Issue 8783043: [APICHANGE] HtmlEmitter: Emit real <script> elements in ES5 mode.

Issue 8783043: [APICHANGE] HtmlEmitter: Emit real <script> elements in ES5 mode. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 11 months ago by kpreid2

Modified:
12 years, 11 months ago

Reviewers:
ihab.awad

CC:
google-caja-discuss_googlegroups.com

Base URL:
http://google-caja.googlecode.com/svn/trunk/

Visibility:
Public.

More Reviews

Description

Changes: * <script> element processing (including execution) happens within insertion mode logic rather than before. This means that an implicit <html> and <head> will be constructed if they have not (fixes issue <https://code.google.com/p/google-caja/issues/detail?id=1628>). * <script> elements will appear as DOM nodes as well as being executed. Their text content is not yet preserved. Impact: * <script> elements are now present (in ES5 mode) where they were discarded before (in all modes), which may confuse code written for prior versions of Caja which manipulates the DOM from within a <script> element. * Some unexplained regressions in jQuery and jQuery UI tests. @r5358

Patch Set 1 #

Total comments: 10

Patch Set 2 : [APICHANGE] HtmlEmitter: Emit real <script> elements in ES5 mode. #

Created: 12 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+209 lines, -77 lines)			Patch
M	src/com/google/caja/plugin/domado.js	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	src/com/google/caja/plugin/html-emitter.js	View	1	10 chunks	+139 lines, -43 lines	0 comments	Download
M	tests/com/google/caja/plugin/JQueryTest.java	View	1	5 chunks	+5 lines, -5 lines	0 comments	Download
M	tests/com/google/caja/plugin/JQueryUITest.java	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	tests/com/google/caja/plugin/es53-test-domado-dom-guest.html	View	1	7 chunks	+34 lines, -10 lines	0 comments	Download
M	tests/com/google/caja/plugin/es53-test-domado-global.js	View	1	2 chunks	+27 lines, -16 lines	0 comments	Download

Messages

Total messages: 5

Expand All Messages | Collapse All Messages

ihab.awad

lgtm https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html-emitter.js File src/com/google/caja/plugin/html-emitter.js (right): https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html-emitter.js#newcode517 src/com/google/caja/plugin/html-emitter.js:517: // TODO(kpreid): This is pulled out as part ...

12 years, 11 months ago (2013-04-17 00:00:59 UTC) #2

kpreid2

Changes: * <script> element processing (including execution) happens within insertion mode logic rather than before. ...

12 years, 11 months ago (2013-04-17 17:08:28 UTC) #3

kpreid2

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html-emitter.js File src/com/google/caja/plugin/html-emitter.js (right): https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html-emitter.js#newcode517 src/com/google/caja/plugin/html-emitter.js:517: // TODO(kpreid): This is pulled out as part of ...

12 years, 11 months ago (2013-04-17 17:08:39 UTC) #4

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:517: // TODO(kpreid): This is pulled
out as part of the WIP refactoring to use
On 2013/04/17 00:00:59, ihab.awad wrote:
> What is WIP?

Work-in-progress. I have rewritten the entire comment.

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:521: var isScript = cdataContentType
=== CDATA_SCRIPT;
On 2013/04/17 00:00:59, ihab.awad wrote:
> Would it be more defensive to have scripts, styles and "other"?

This code is unchanged, just moved. This variable, and in fact this function, is
going away, as noted in (the new version of) the above TODO.

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:527: res, marker, continuation,
pendingDelayed);
On 2013/04/17 00:00:59, ihab.awad wrote:
> Nit: continuation indent = 4

Done.

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:775: // src attribute? Can we make
this another "slow path attribute"
On 2013/04/17 00:00:59, ihab.awad wrote:
> What's a slow path attribute?

normalInsert normally directly writes sanitized attributes to the host DOM. In
the cases where we need side effects/special handling — in particular, event
handlers — it instead invokes Domado's tame setAttribute. I've updated this
comment to point at normalInsert.

https://codereview.appspot.com/8783043/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:948: // ...
On 2013/04/17 00:00:59, ihab.awad wrote:
> What's with the ellipsis comments? :)

They refer to steps of the HTML spec which I am not bothering to quote here.

lgtm

Expand All Messages | Collapse All Messages