Code review - Issue 6741065: Derive DOM element tamings from specification data.https://codereview.appspot.com/2012-11-13T18:14:09+00:00rietveld
Message from unknown
2012-10-23T20:05:59+00:00kpreid2urn:md5:1ea97201640e98b6799a64bd9ecaf258
Message from kpreid.switchb.org@gmail.com
2012-10-23T20:06:06+00:00kpreid2urn:md5:7ae2cf64a9e86ed9c0767bb6ed68e0e1
Message from unknown
2012-10-23T21:44:07+00:00kpreid2urn:md5:6482b05f6de437a5629328245d7a2f13
Message from kpreid.switchb.org@gmail.com
2012-10-23T21:45:16+00:00kpreid2urn:md5:8ee7d94a5d352adbbb1981b797f60afc
Updated to fix ES5/3 compatibility due to a freeze ordering problem.
Message from unknown
2012-11-01T23:18:02+00:00kpreid2urn:md5:80274d5a6b16764c852c1de3e5d813c2
Message from jasvir@gmail.com
2012-11-06T04:52:25+00:00Jasvirurn:md5:8062a7c4e197d18a877e536071627ded
LGTM
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json
File src/com/google/caja/lang/html/html4-elements-defs.json (right):
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json#newcode342
src/com/google/caja/lang/html/html4-elements-defs.json:342: "interface": "HTMLElement" },
HTMLIsIndexElement?
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json#newcode385
src/com/google/caja/lang/html/html4-elements-defs.json:385: "interface": "HTMLMetaElement" },
HTMLMenuElement. Copy and paste?
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json#newcode602
src/com/google/caja/lang/html/html4-elements-defs.json:602: "description": "instance of a variable or program argument" }
Missing interface?
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/plugin/domado.js#newcode3555
src/com/google/caja/plugin/domado.js:3555: if (!isVirtualized !== !shouldBeVirtualized) {
A little !too !clever. ;)
Message from unknown
2012-11-06T19:03:55+00:00kpreid2urn:md5:c0396c58c5b2fc5a4f354bafa61ecf69
Message from kpreid.switchb.org@gmail.com
2012-11-06T19:19:54+00:00kpreid2urn:md5:f9d7ceae6397005ba31079aac23b5b39
Added HTMLUnknownElement per spec; please review that aspect of the code. No changes to schema data other than those mentioned below.
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json
File src/com/google/caja/lang/html/html4-elements-defs.json (right):
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json#newcode342
src/com/google/caja/lang/html/html4-elements-defs.json:342: "interface": "HTMLElement" },
On 2012/11/06 04:52:25, Jasvir wrote:
> HTMLIsIndexElement?
http://www.whatwg.org/specs/web-apps/current-work/multipage/obsolete.html#other-elements,-attributes-and-apis
“The bgsound, isindex, multicol, nextid, rb, and spacer elements must use the HTMLUnknownElement interface.”
Hm, I neglected the existence of HTMLUnknownElement — we use HTMLElement as the default for unknown elements. Fixed that.
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json#newcode385
src/com/google/caja/lang/html/html4-elements-defs.json:385: "interface": "HTMLMetaElement" },
On 2012/11/06 04:52:25, Jasvir wrote:
> HTMLMenuElement. Copy and paste?
That or misreading the table / where I'm typing. Call me a loose cannon already.
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/lang/html/html4-elements-defs.json#newcode602
src/com/google/caja/lang/html/html4-elements-defs.json:602: "description": "instance of a variable or program argument" }
On 2012/11/06 04:52:25, Jasvir wrote:
> Missing interface?
Done.
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
https://codereview.appspot.com/6741065/diff/6001/src/com/google/caja/plugin/domado.js#newcode3555
src/com/google/caja/plugin/domado.js:3555: if (!isVirtualized !== !shouldBeVirtualized) {
On 2012/11/06 04:52:25, Jasvir wrote:
> A little !too !clever. ;)
This is a standard formulation of boolean xor. Do you want me to change it? Would !!isVirtualized !== !!shouldBeVirtualized be better, to emphasize the coerce-to-boolean intent?
Message from jasvir@gmail.com
2012-11-09T22:35:32+00:00Jasvirurn:md5:0826732972bfb1bc31d628a59638fffb
For fun I ran a check on Chrome using these html4 and html5 defs.
xxx.forEach(function(x) {
var el = document.createElement(x.key);
var cand = Object.prototype.toString.call(el);
if (cand.indexOf(x.interface) == -1) {
print(x.key + " => " + cand + ", not " + x.interface);
}
});
From html4:
APPLET => [object HTMLAppletElement], not HTMLElement
BASEFONT => [object HTMLBaseFontElement], not HTMLElement
DIR => [object HTMLDirectoryElement], not HTMLElement
FONT => [object HTMLFontElement], not HTMLElement
FRAME => [object HTMLFrameElement], not HTMLElement
FRAMESET => [object HTMLFrameSetElement], not HTMLElement
TD => [object HTMLTableCellElement], not HTMLTableDataCellElement
TH => [object HTMLTableCellElement], not HTMLTableHeaderCellElement
From html5:
COMMAND => [object HTMLElement], not HTMLCommandElement
DATA => [object HTMLUnknownElement], not HTMLElement
DIALOG => [object HTMLUnknownElement], not HTMLDialogElement
TIME => [object HTMLUnknownElement], not HTMLTimeElement
None of these seem significant differences -- they're essentially ways in which Chrome varies from the html5 spec.
Message from jasvir@gmail.com
2012-11-09T23:08:54+00:00Jasvirurn:md5:ccbf4d6c920a4a532405128bc2299e4f
https://codereview.appspot.com/6741065/diff/11001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
https://codereview.appspot.com/6741065/diff/11001/src/com/google/caja/plugin/domado.js#newcode5900
src/com/google/caja/plugin/domado.js:5900: 'HTMLTableCellElement',
So should this become HTMLTableDataCell now? How does allDomNodeClasses interact with the element defs?
Message from unknown
2012-11-12T20:28:57+00:00kpreid2urn:md5:c0a54d2865f83eca0caae1a1ff070db6
Message from kpreid.switchb.org@gmail.com
2012-11-12T20:29:10+00:00kpreid2urn:md5:621ba5b5018bb1ab5edeed963f886272
New snapshot.
https://codereview.appspot.com/6741065/diff/11001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
https://codereview.appspot.com/6741065/diff/11001/src/com/google/caja/plugin/domado.js#newcode5900
src/com/google/caja/plugin/domado.js:5900: 'HTMLTableCellElement',
On 2012/11/09 23:08:54, Jasvir wrote:
> So should this become HTMLTableDataCell now?
No. HTMLTableCellElement is the superclass of HTMLTableDataCellElement and HTMLTableHeaderCellElement.
> How does allDomNodeClasses interact with the element defs?
This *was* a *complete* list of standard classes (it is no longer complete due to HTML5 etc.); the ones in this list but not otherwise implemented are made into aliases for HTMLElement.
It occurs to me that we could instead generate this list from all interfaces mentioned in the schema. Comparison of the results this would have pointed out a few mistakes in the schema data and Domado; after fixing those, the only difference that results is that we would lose HTMLIsIndexElement and HTMLNavElement. Both are not specified by HTML5.
Therefore, I have gone ahead and done so; this list is now gone.
Message from jasvir@gmail.com
2012-11-13T17:58:08+00:00Jasvirurn:md5:c27c0482ca18c1299d8e1317d4f66e91
With the caveat to doublecheck the use of gOPN, LGTM
https://codereview.appspot.com/6741065/diff/14001/src/com/google/caja/plugin/html-schema.js
File src/com/google/caja/plugin/html-schema.js (right):
https://codereview.appspot.com/6741065/diff/14001/src/com/google/caja/plugin/html-schema.js#newcode176
src/com/google/caja/plugin/html-schema.js:176: table[ELEMENT_DOM_INTERFACES[el]] = true;
Note - it's safe to use object as a dictionary here since ELEMENT_DOM_INTERFACES is known not to contain elements like toString and valueOf.
https://codereview.appspot.com/6741065/diff/14001/src/com/google/caja/plugin/html-schema.js#newcode179
src/com/google/caja/plugin/html-schema.js:179: scriptInterfacesCache = cajaVM.def(Object.getOwnPropertyNames(table));
Double check - this file gets run cajoled (in which case Object.getOwnPropertyNames is shimmed in) or in SES in which case gOPN is browser provided).
Message from kpreid.switchb.org@gmail.com
2012-11-13T18:14:09+00:00kpreid2urn:md5:f8e2d3e4cf5f8ee8aadd4c04bf81dddd
https://codereview.appspot.com/6741065/diff/14001/src/com/google/caja/plugin/html-schema.js
File src/com/google/caja/plugin/html-schema.js (right):
https://codereview.appspot.com/6741065/diff/14001/src/com/google/caja/plugin/html-schema.js#newcode176
src/com/google/caja/plugin/html-schema.js:176: table[ELEMENT_DOM_INTERFACES[el]] = true;
On 2012/11/13 17:58:08, Jasvir wrote:
> Note - it's safe to use object as a dictionary here since
> ELEMENT_DOM_INTERFACES is known not to contain elements
> like toString and valueOf.
I'm not sure which object (ELEMENT_DOM_INTERFACES or table) you're referring to here. Either way, this is safe even if that is not the case: we iterate over own properties of ELEMENT_DOM_INTERFACES, and the only thing we ever do with 'table' is get its own properties. This is just a deduplicator.
https://codereview.appspot.com/6741065/diff/14001/src/com/google/caja/plugin/html-schema.js#newcode179
src/com/google/caja/plugin/html-schema.js:179: scriptInterfacesCache = cajaVM.def(Object.getOwnPropertyNames(table));
On 2012/11/13 17:58:08, Jasvir wrote:
> Double check - this file gets run cajoled (in which case
> Object.getOwnPropertyNames is shimmed in) or in SES
> in which case gOPN is browser provided).
Yes. This is part of the bundle which includes Domado. Note that it uses cajaVM.def which only exists in Caja environments, regardless of browser ES5 support.