Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(120)

Issue 8446: Felix' mega patch

Can't Edit
Can't Publish+Mail
Start Review
Created:
17 years, 8 months ago by ben
Modified:
16 years, 11 months ago
Reviewers:
MikeSamuel
CC:
google-caja-discuss_googlegroups.com
Base URL:
http://google-caja.googlecode.com/svn/trunk/
Visibility:
Public.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+452 lines, -29 lines) Patch
M src/com/google/caja/plugin/domita.js View 22 chunks +452 lines, -29 lines 14 comments Download

Messages

Total messages: 1
MikeSamuel
17 years, 8 months ago (2008-11-05 22:54:18 UTC) #1
http://codereview.appspot.com/8446/diff/1/2
File src/com/google/caja/plugin/domita.js (left):

http://codereview.appspot.com/8446/diff/1/2#oldcode486
Line 486: 
Needs documentation.

http://codereview.appspot.com/8446/diff/1/2#oldcode988
Line 988: if (!this.editable___) { throw new Error(NOT_EDITABLE); }
I disagree.  Stopping review here.  Will finish later.

http://codereview.appspot.com/8446/diff/1/2
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/8446/diff/1/2#newcode474
Line 474: case 'textarea':
These are incorrect.

HTMLTextAreaElement is not a subclass of HTMLInputElement acoording to
http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-24874179 and neither are the
others.

http://codereview.appspot.com/8446/diff/1/2#newcode599
Line 599: 'ownerDocument',
This seems like a really bad idea.

http://codereview.appspot.com/8446/diff/1/2#newcode681
Line 681: return imports.document;
Why not use the ownerDocument field?
How is this not a massive expansion of authority?

http://codereview.appspot.com/8446/diff/1/2#newcode758
Line 758: 'offsetLeft', 'offsetTop', 'offsetWidth', 'offsetHeight'm
m -> ,

http://codereview.appspot.com/8446/diff/1/2#newcode814
Line 814: return this.node___.hasAttribute( name );
Please remove spaces inside parentheses

http://codereview.appspot.com/8446/diff/1/2#newcode844
Line 844: this.node___.removeAttribute( name );
This is insufficient.  If elementPolicies would add the attribute, it can't
safely be removed.

http://codereview.appspot.com/8446/diff/1/2#newcode856
Line 856: TameElement.prototype.setTitle = function (classes) {
Please rename param.

http://codereview.appspot.com/8446/diff/1/2#newcode863
Line 863: TameElement.prototype.setDir = function (classes) {
Please rename param.

http://codereview.appspot.com/8446/diff/1/2#newcode944
Line 944: };
Does access to scroll positions leak information if the scrollParent is
inaccessible?

http://codereview.appspot.com/8446/diff/1/2#newcode1009
Line 1009: };
Violation of DOMado rules.  Please remove.

http://codereview.appspot.com/8446/diff/1/2#newcode1024
Line 1024: exportFields(this, ['action', 'elements', 'enctype', 'method',
'target']);
setting enctype is disallowed in the HTML schema.

http://codereview.appspot.com/8446/diff/1/2#newcode1083
Line 1083: 'options', 'selected', 'selectedIndex',
selectedIndex,multiple, et al are not members of HTMLInputElement.

Does tabIndex leak information about the page?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b