Issue 1801203002: Parser: Make skipping HTML comments optional.

Issue 1801203002: Parser: Make skipping HTML comments optional. (Closed)

Created:
4 years, 9 months ago by vogelheim

Modified:
4 years, 9 months ago

Reviewers:
jochen (gone - plz use gerrit), rossberg

CC:
ulan, v8-reviews_googlegroups.com

Base URL:
https://chromium.googlesource.com/v8/v8.git@master

Target Ref:
refs/pending/heads/master

Project:
v8

Visibility:
Public.

More Reviews

Description

Parser: Make skipping HTML comments optional. API change: This adds a new flag skip_html_comments to v8::ScriptOriginOptions. This flag controls whether V8 will attempt to honour HTML-style comments in JS sources. (That is: Gracefully ignore  in JS sources, which was a popular technique in the early days of JavaScript, to prevent non-JS-enabled browsers from displaying script sources to uses.) The flag defaults to 'true' when using v8::ScriptOrigin constructor, which preserves the existing behaviour. Embedders which are happy with the existing behaviour will thus not need any changes. BUG=chromium:573887 LOG=Y Committed: https://crrev.com/91d344288aa51ed03eaaa1cb3e368ac1e82f0173 Cr-Commit-Position: refs/heads/master@{#34904}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Cleanly expose the decision at the API. #

Total comments: 2

Patch Set 3 : Removed prefix; set default only on v8::ScriptOrigin constructor. #

Total comments: 2

Patch Set 4 : Andreas' feedback: [skip|ignore] -> allow #

Patch Set 5 : Clearer comment. #

Patch Set 6 : Rebase. #

Created: 4 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+115 lines, -37 lines)			Patch
M	include/v8.h	View	1 2 3 4 5	4 chunks	+21 lines, -15 lines	0 comments	Download
M	src/api.cc	View	1 2 3 4 5	1 chunk	+2 lines, -1 line	0 comments	Download
M	src/objects.h	View	1 2 3 4 5	1 chunk	+3 lines, -2 lines	0 comments	Download
M	src/parsing/parser.h	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/parsing/parser.cc	View	1 2 3 4 5	1 chunk	+1 line, -2 lines	0 comments	Download
M	src/parsing/scanner.h	View	1 2 3 4 5	2 chunks	+4 lines, -1 line	0 comments	Download
M	src/parsing/scanner.cc	View	1 2 3 4 5	2 chunks	+3 lines, -2 lines	0 comments	Download
M	test/cctest/test-api.cc	View	1	1 chunk	+37 lines, -0 lines	0 comments	Download
M	test/cctest/test-parsing.cc	View	1 2 3 4 5	15 chunks	+40 lines, -14 lines	0 comments	Download

Messages

Total messages: 29 (12 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

vogelheim

Not quite done, but I need two opinions: - How should this be triggered? I ...

4 years, 9 months ago (2016-03-15 18:22:59 UTC) #2

jochen (gone - plz use gerrit)

https://codereview.chromium.org/1801203002/diff/1/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/1801203002/diff/1/src/compiler.cc#newcode1669 src/compiler.cc:1669: bool ignore = script_name.is_null() || On 2016/03/15 at 18:22:59, ...

4 years, 9 months ago (2016-03-15 18:36:08 UTC) #4

vogelheim

@jochen: Please take another look. Is v8::ScriptOriginOptions the right location for this? @ulan: on cc:, ...

4 years, 9 months ago (2016-03-18 10:33:23 UTC) #8

jochen (gone - plz use gerrit)

lgtm with comments https://codereview.chromium.org/1801203002/diff/60001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1801203002/diff/60001/include/v8.h#newcode1009 include/v8.h:1009: bool is_skip_html_comments = true) can you ...

4 years, 9 months ago (2016-03-18 10:50:29 UTC) #9

vogelheim

.. let me know if this works. https://codereview.chromium.org/1801203002/diff/60001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1801203002/diff/60001/include/v8.h#newcode1009 include/v8.h:1009: bool is_skip_html_comments ...

4 years, 9 months ago (2016-03-18 11:19:53 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801203002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801203002/80001

4 years, 9 months ago (2016-03-18 12:52:13 UTC) #14

commit-bot: I haz the power

Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12585)

4 years, 9 months ago (2016-03-18 12:56:05 UTC) #16

rossberg

https://codereview.chromium.org/1801203002/diff/80001/src/parsing/scanner.cc File src/parsing/scanner.cc (right): https://codereview.chromium.org/1801203002/diff/80001/src/parsing/scanner.cc#newcode487 src/parsing/scanner.cc:487: } else if (c0_ == '!' && ignore_html_comments_) { ...

4 years, 9 months ago (2016-03-18 13:14:08 UTC) #19

vogelheim

4 years, 9 months ago (2016-03-18 13:56:01 UTC) #20

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801203002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801203002/140001

4 years, 9 months ago (2016-03-18 17:20:55 UTC) #24

commit-bot: I haz the power

Patchset 6 (id:??) landed as https://crrev.com/91d344288aa51ed03eaaa1cb3e368ac1e82f0173 Cr-Commit-Position: refs/heads/master@{#34904}

4 years, 9 months ago (2016-03-18 17:24:31 UTC) #28

vogelheim

4 years, 9 months ago (2016-03-21 16:59:05 UTC) #29

Message was sent while issue was closed.

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/1817163003/ by vogelheim@chromium.org.

The reason for reverting is: Violates ES6 spec (crbug.com/4850), and
implementation was over-eager. Will revert for now..

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages