Handle "//# sourceURL" comments in the Parser instead of the JS.

Handle "//# sourceURL" comments in the Parser instead of the JS.

BUG=v8:2948
LOG=N
R=svenpanne@chromium.org, yurys@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22137

[yurys, 2014-06-05 13:13:38]

https://codereview.chromium.org/316173002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/316173002/diff/1/src/objects.h#newcode6842
src/objects.h:6842: kEvalFrominstructionsOffsetOffset + kPointerSize;
This should be kFlagsOffset + kPointerSize

[marja, 2014-06-05 13:19:25]

https://codereview.chromium.org/316173002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/316173002/diff/1/src/objects.h#newcode6842
src/objects.h:6842: kEvalFrominstructionsOffsetOffset + kPointerSize;
On 2014/06/05 13:13:39, yurys wrote:
> This should be kFlagsOffset + kPointerSize

Oops, done.

[yurys, 2014-06-05 13:55:20]

https://codereview.chromium.org/316173002/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/316173002/diff/20001/src/parser.cc#newcode3853
src/parser.cc:3853: while (name_start < data.length() && data[name_start] == '
') {
This won't work for \tname=. Also the original pattern assumed exactly one
whitespace character between @ and sourceURL

https://codereview.chromium.org/316173002/diff/20001/src/parser.cc#newcode3862
src/parser.cc:3862: while (value_start < data.length() && data[value_start] == '
') {
Same here, the original pattern was [\040\t]sourceURL=[\040\t]*

https://codereview.chromium.org/316173002/diff/20001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/316173002/diff/20001/src/parser.h#newcode788
src/parser.h:788: void ParseSourceMapComment();
The name is a bit misleading as we actually parse sourceURL not
sourceMappingURL.

https://codereview.chromium.org/316173002/diff/20001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/316173002/diff/20001/src/scanner.cc#newcode477
src/scanner.cc:477: token = SkipSourceMapComment();
I believe we should consider only sourceURL that are at the end of file. If
there is something like

function foo(){}
//@ sourceURL = url.js
function bar(){}

we should skip the url as far as I remember.

[vsevik, 2014-06-05 14:21:59]

> I believe we should consider only sourceURL that are at the end of file. If
> there is something like
> 
> function foo(){}
> //@ sourceURL = url.js
> function bar(){}
> 
> we should skip the url as far as I remember.
This is not quite so. For example we want to support a case when the script has
both sourceURL and sourceMappingURL annotations (it is obviously not possible
for both of them to be on the last line).
Actually is it possible to parse sourceMappingURL as well in this patch?


Also one thing that is probably different (i am not sure) in our implementations
is that we deny whitespace after sourceURL, see
https://codereview.chromium.org/256543002

FYI We have a test for sourceURL:
LayoutTests/inspector/sources/debugger/source-url-comment.html

[marja, 2014-06-05 15:28:03]

yurys@: the "has source url" bool is set to false in Scanner::Next(), so this
only accepts the magic comment if there are no tokens after it.

vsevik: what do you want to do in the case where we have both sourceURL and
sourceMappingURL? do you want script.source_url and script.source_mapping_url
separately?

(Btw, this CL is not working yet, some plumbing is missing... will fix.)

[vsevik, 2014-06-05 16:05:29]

On 2014/06/05 15:28:03, marja wrote:
> yurys@: the "has source url" bool is set to false in Scanner::Next(), so this
> only accepts the magic comment if there are no tokens after it.
Could you please clarify whether this would work for code like

function foo() {}
//@ sourceURL=foo.js
function bar() {}

?
I don't like this syntax and would prefer if we didn't support it, but I we
support it in our current implementation
and it is possible that some of our users would be upset if we change that.
We can still change it later (I hope the diff wouldn't be big)


> vsevik: what do you want to do in the case where we have both sourceURL and
> sourceMappingURL?
I'd like to have both of them

> do you want script.source_url and script.source_mapping_url
> separately?
Yes

[marja, 2014-06-06 11:15:35]

The latest patch set parses both sourceURL and sourceMappingURL, and allows them
in the middle of the script too (the later one overrides the previous one, if
there are several instances of the same value).

I'm not sure if you'd need more plumbing than this, for example,
source_mapping_url is not plumbed via StackFrame. Is it enough that Script has
the source_mapping_url accessor?

[vsevik, 2014-06-06 14:03:23]

> The latest patch set parses both sourceURL and sourceMappingURL, and allows
them
> in the middle of the script too (the later one overrides the previous one, if
> there are several instances of the same value).
Great, thank you!

> I'm not sure if you'd need more plumbing than this, for example,
> source_mapping_url is not plumbed via StackFrame. Is it enough that Script has
> the source_mapping_url accessor?
Should be enough. I can write up blink-side plumbing so that we could check that
inspector tests are passing as well.

Do you plan to remove the older javascript-based sourceURL parsing?
This should be probably done in a separate patch so that v8 roll doesn't break
the build but other than that I don't think it is still needed.

[vsevik, 2014-06-06 17:14:45]

FYI: Here is a blink side patch. It seems to pass all the tests.
https://codereview.chromium.org/323523004/

[marja, 2014-06-10 08:30:33]

On 2014/06/06 14:03:23, vsevik wrote:
> Do you plan to remove the older javascript-based sourceURL parsing?
> This should be probably done in a separate patch so that v8 roll doesn't break
> the build but other than that I don't think it is still needed.

You mean in messages.js? This one removes it, and makes ScriptNameOrSourceURL
return this.source_url in some cases. When Blink has migrated, that function can
be removed altogether, I suppose.

[yurys, 2014-06-10 09:28:48]

https://codereview.chromium.org/316173002/diff/120001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/316173002/diff/120001/src/accessors.cc#newcod...
src/accessors.cc:601: Object* source =
Script::cast(JSValue::cast(object)->value())->source_url();
source -> url

https://codereview.chromium.org/316173002/diff/120001/src/accessors.cc#newcod...
src/accessors.cc:636: Object* source =
ditto

https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc#newcode318
src/scanner.cc:318: // Magic comments are of the form \s*name\s*=\s*value\s*.*
and this function
We might need to allow only single whitespace between @ and sourceURL= and no
space between sourceURL and =:

...@\ssourceURL=\s*...

Personally I'd prefer \s+sourceURL=\s* but it wouldn't match what I see at
https://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/

Vsevolod, what do you think?

https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc#newcode335
src/scanner.cc:335: } else if
(name_literal.IsEqualTo(STATIC_ASCII_VECTOR("sourceMappingURL"))) {
Since we add support for sourceMappingURL we need some tests for it too (at the
moment we have test coverage only for sourceURL).

[vsevik, 2014-06-10 11:49:30]

https://codereview.chromium.org/316173002/diff/120001/src/messages.js
File src/messages.js (left):

https://codereview.chromium.org/316173002/diff/120001/src/messages.js#oldcode561
src/messages.js:561: if (this.hasCachedNameOrSourceURL) {
This should be done in a separate patch, otherwise v8 roll is going to break
blink (blink can not migrate to new way of getting source_url before the roll).

[vsevik, 2014-06-10 11:50:23]

>
https://codereview.chromium.org/316173002/diff/120001/src/messages.js#oldcode561
> src/messages.js:561: if (this.hasCachedNameOrSourceURL) {
> This should be done in a separate patch, otherwise v8 roll is going to break
> blink (blink can not migrate to new way of getting source_url before the
roll).

Just to clarify: blink has layout tests checking sourceURL in different cases
that will be broken by this patch.

[yurys, 2014-06-10 11:55:28]

https://codereview.chromium.org/316173002/diff/120001/src/messages.js
File src/messages.js (left):

https://codereview.chromium.org/316173002/diff/120001/src/messages.js#oldcode561
src/messages.js:561: if (this.hasCachedNameOrSourceURL) {
On 2014/06/10 11:49:29, vsevik wrote:
> This should be done in a separate patch, otherwise v8 roll is going to break
> blink (blink can not migrate to new way of getting source_url before the
roll). 

The result seems unchanged why the tests would break?

[vsevik, 2014-06-10 12:14:58]

> The result seems unchanged why the tests would break?
Oh, I didn't get it, you are right, this is fine.

[vsevik, 2014-06-10 12:26:12]

>
https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc#newcode318
> src/scanner.cc:318: // Magic comments are of the form \s*name\s*=\s*value\s*.*
> and this function
> We might need to allow only single whitespace between @ and sourceURL= and no
> space between sourceURL and =:
> 
> ...@\ssourceURL=\s*...
> 
> Personally I'd prefer \s+sourceURL=\s* but it wouldn't match what I see at
> https://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/
> 
> Vsevolod, what do you think?
We are currently supporting single whitespace only and I would prefer not change
this.
Actually we have very strict regex for the comment line: //[#@][
\t]sourceURL=[^\s]* which (roughly) matches Source Map Revision 3 Proposal.
So we try to prohibit any extra spaces.

[marja, 2014-06-10 13:45:54]

On 2014/06/10 12:26:12, vsevik wrote:
> >
>
https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc#newcode318
> > src/scanner.cc:318: // Magic comments are of the form
\s*name\s*=\s*value\s*.*
> > and this function
> > We might need to allow only single whitespace between @ and sourceURL= and
no
> > space between sourceURL and =:
> > 
> > ...@\ssourceURL=\s*...
> > 
> > Personally I'd prefer \s+sourceURL=\s* but it wouldn't match what I see at
> > https://blog.getfirebug.com/2009/08/11/give-your-eval-a-name-with-sourceurl/
> > 
> > Vsevolod, what do you think?
> We are currently supporting single whitespace only and I would prefer not
change
> this.
> Actually we have very strict regex for the comment line: //[#@][
> \t]sourceURL=[^\s]* which (roughly) matches Source Map Revision 3 Proposal.
> So we try to prohibit any extra spaces.

The latest patch set matches what was in messages.js before and what is in
findMagicComment in ContentSearchUtils.cpp, that is:

//[#@][ \t]sourceURL=[ \t]*<value>

What do you mean by "we have a very strict regex"; where?

[marja, 2014-06-10 13:46:09]

& thanks for comments!

https://codereview.chromium.org/316173002/diff/120001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/316173002/diff/120001/src/accessors.cc#newcod...
src/accessors.cc:601: Object* source =
Script::cast(JSValue::cast(object)->value())->source_url();
On 2014/06/10 09:28:47, yurys wrote:
> source -> url

Done.

https://codereview.chromium.org/316173002/diff/120001/src/accessors.cc#newcod...
src/accessors.cc:636: Object* source =
On 2014/06/10 09:28:47, yurys wrote:
> ditto

Done.

https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/316173002/diff/120001/src/scanner.cc#newcode335
src/scanner.cc:335: } else if
(name_literal.IsEqualTo(STATIC_ASCII_VECTOR("sourceMappingURL"))) {
On 2014/06/10 09:28:47, yurys wrote:
> Since we add support for sourceMappingURL we need some tests for it too (at
the
> moment we have test coverage only for sourceURL).

Done.

[yurys, 2014-06-10 14:41:04]

https://codereview.chromium.org/316173002/diff/140001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/316173002/diff/140001/include/v8.h#newcode949
include/v8.h:949: // Data read from magic sourceURL and sourceMappingURL
comments.
Please use /** */ style annotations.

https://codereview.chromium.org/316173002/diff/140001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/316173002/diff/140001/src/scanner.cc#newcode345
src/scanner.cc:345: while (c0_ >= 0 &&
!unicode_cache_->IsWhiteSpaceOrLineTerminator(c0_)) {
This will stop at first whitepace while ContentSearchUtils.cpp would parse till
the end of line. Not sure which one we should prefer though.

//# sourceURL=foo bar \n

[yurys, 2014-06-10 14:46:54]

https://codereview.chromium.org/316173002/diff/140001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/316173002/diff/140001/src/scanner.cc#newcode345
src/scanner.cc:345: while (c0_ >= 0 &&
!unicode_cache_->IsWhiteSpaceOrLineTerminator(c0_)) {
On 2014/06/10 14:41:04, yurys wrote:
> This will stop at first whitepace while ContentSearchUtils.cpp would parse
till
> the end of line. Not sure which one we should prefer though.
> 
> //# sourceURL=foo bar \n

As Vsevolod pointed out sourceURLs that contain whitspaces will be ignored by
current implementation. So we should probably preserve this behavior.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[marja, 2014-06-10 15:02:56]

Yeah okay, let's match the ContentSearchUtils.cpp. The latest patch set does
that.

[yurys, 2014-06-10 15:18:16]

On 2014/06/10 15:02:56, marja wrote:
> Yeah okay, let's match the ContentSearchUtils.cpp. The latest patch set does
> that.

ContentSearchUtils.cpp would return empty string if there is a whitespace inside
url as if there were no source url at all while implementation in PS#9 will
accept that and return that value with trimmed whitespaces.

[marja, 2014-06-10 15:21:13]

On 2014/06/10 15:18:16, yurys wrote:
> On 2014/06/10 15:02:56, marja wrote:
> > Yeah okay, let's match the ContentSearchUtils.cpp. The latest patch set does
> > that.
> 
> ContentSearchUtils.cpp would return empty string if there is a whitespace
inside
> url as if there were no source url at all while implementation in PS#9 will
> accept that and return that value with trimmed whitespaces.

Ah true, I missed that. Will fix tomorrow. Oh well, it has other "disallowed
chars" too, and this will be a can of worms if we start to discuss what chars
exactly we want to allow here...

[marja, 2014-06-11 07:40:53]

The latest patch set matches the disallowed characters and whitespace handling
of ContentSearchUtils, even though it's pretty arbitrary regarding what
characters it allows in the URL and what not (disallows ' which is a valid URL
character, allows ^ which is not a valid URL character).

[yurys, 2014-06-11 11:32:18]

LGTM

On 2014/06/11 07:40:53, marja wrote:
> The latest patch set matches the disallowed characters and whitespace handling
> of ContentSearchUtils, even though it's pretty arbitrary regarding what
> characters it allows in the URL and what not (disallows ' which is a valid URL
> character, allows ^ which is not a valid URL character).

I'm not a fan of that logic as well but I guess it is safe to match existing
behavior when switching implementation and fix possible issue with the pattern
we use in a separate change if need to.

[marja, 2014-06-11 13:17:31]

svenpanne@, ulan@, could you do the V8 side review?

svenpanne: API additions
ulan: the rest

[Sven Panne, 2014-06-13 10:37:50]

LGTM with nits.

https://codereview.chromium.org/316173002/diff/170001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/316173002/diff/170001/src/messages.js#newcode566
src/messages.js:566: $Array("source", "name", "source_url",
"source_mapping_url", "line_ends", "line_offset", "column_offset"),
80 columns...

https://codereview.chromium.org/316173002/diff/170001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/316173002/diff/170001/src/parser.cc#newcode3841
src/parser.cc:3841: void Parser::HandleMagicComments() {
Can we use a better name for this? I'm quite sure that in 2 weeks nobody will
have the slightest clue what "magic comments" are...

https://codereview.chromium.org/316173002/diff/170001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/316173002/diff/170001/src/vector.h#newcode103
src/vector.h:103: bool IsEqualTo(Vector<T> other) {
Perhaps a const ref nicer? Furthermore, operator== is a better name, the method
is just lifting the underlying operator to the Vector after all.

[marja, 2014-06-13 11:15:29]

Thanks for review!

ulan@, since svenpanne@ reviewed everything, I'm not expecting you to review too
(though, feel free to have a look if you want to). The same for vsevik@, since
yurys@ has already reviewed.

I'm not going to land this until https://codereview.chromium.org/314603004/
lands, and I'm going to need to rebase on top of that (though, it should be
trivial).

https://codereview.chromium.org/316173002/diff/170001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/316173002/diff/170001/src/messages.js#newcode566
src/messages.js:566: $Array("source", "name", "source_url",
"source_mapping_url", "line_ends", "line_offset", "column_offset"),
On 2014/06/13 10:37:49, Sven Panne wrote:
> 80 columns...

Done (wut, why didn't the upload script whine??)

https://codereview.chromium.org/316173002/diff/170001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/316173002/diff/170001/src/parser.cc#newcode3841
src/parser.cc:3841: void Parser::HandleMagicComments() {
On 2014/06/13 10:37:49, Sven Panne wrote:
> Can we use a better name for this? I'm quite sure that in 2 weeks nobody will
> have the slightest clue what "magic comments" are...

Renamed to HandleSourceURLComments. (Though, the term "magic comment" is used on
ContentSearchUtils in Inspector, that's why it was originally named like that.)

https://codereview.chromium.org/316173002/diff/170001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/316173002/diff/170001/src/vector.h#newcode103
src/vector.h:103: bool IsEqualTo(Vector<T> other) {
On 2014/06/13 10:37:49, Sven Panne wrote:
> Perhaps a const ref nicer? Furthermore, operator== is a better name, the
method
> is just lifting the underlying operator to the Vector after all.

Done.

[marja, 2014-06-18 14:01:51]

(V8 is not accepting commits atm, and I need to get my
delay-string-internalization CL re-landed before this... can take a while.)

https://codereview.chromium.org/316173002/diff/210001/src/scanner.h
File src/scanner.h (right):

https://codereview.chromium.org/316173002/diff/210001/src/scanner.h#newcode581
src/scanner.h:581: Token::Value SkipMagicComment();
Note to self: Should also rename these before landing.

[marja, 2014-07-01 09:20:56]

Update: I rebased this on top of the string-internalization-delay changes, and
changed the rest of "magic" names.

The V8 tree is throttled now, and I'll land this as soon as it opens.

[marja, 2014-07-01 09:21:15]

(Also, wanted to say that no action from anybody else than me is needed.)

[aandrey, 2014-07-01 09:49:12]

FYI

https://codereview.chromium.org/316173002/diff/250001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/316173002/diff/250001/src/vector.h#newcode103
src/vector.h:103: bool operator==(const Vector<T>& other) {
bool operator==() const {

https://codereview.chromium.org/316173002/diff/250001/src/vector.h#newcode104
src/vector.h:104: if (length_ != other.length_) return false;
if (this == &other) return true; ?

[marja, 2014-07-01 11:33:58]

Btw, one thing turned out during the test run...

Now that the parser takes care of the sourceURL, we naturally don't have it
before we have parsed the script.

So the sourceURL is not available in the "before compile" debug event.

See the test fix. aandrey@, is this okay from the dev tools side? (yurys@ said
previously that the dev tools side should be okay as long as cctest passes, and
it does now).

https://codereview.chromium.org/316173002/diff/250001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/316173002/diff/250001/src/vector.h#newcode103
src/vector.h:103: bool operator==(const Vector<T>& other) {
On 2014/07/01 09:49:11, aandrey wrote:
> bool operator==() const {

Done.

https://codereview.chromium.org/316173002/diff/250001/src/vector.h#newcode104
src/vector.h:104: if (length_ != other.length_) return false;
On 2014/07/01 09:49:11, aandrey wrote:
> if (this == &other) return true; ?

I think this is even better:
if (start_ == other.start_) return true;

(Two vectors can point to the other same data. This check is inserted after the
length check, so here we know that the lengths are equal.)

[marja, 2014-07-01 11:52:24]

> (Two vectors can point to the other same data. 

Wut, I meant to say: two different vector objects can point to the same data.

[vsevik, 2014-07-01 13:05:09]

> So the sourceURL is not available in the "before compile" debug event.
> 
> See the test fix. aandrey@, is this okay from the dev tools side? (yurys@ said
> previously that the dev tools side should be okay as long as cctest passes,
and
> it does now).
AFAIK DevTools only uses BeforeCompile event for script preprocessor.
While script preprocessor receives a URL as a parameter I think we should be
fine
if it does not provide sourceURL at this point as script preprocessor is going
to parse the script anyway and could extract the sourceURL annotation and
probably even change it.

[marja, 2014-07-01 13:29:07]

On 2014/07/01 13:05:09, vsevik wrote:
> > So the sourceURL is not available in the "before compile" debug event.
> > 
> > See the test fix. aandrey@, is this okay from the dev tools side? (yurys@
said
> > previously that the dev tools side should be okay as long as cctest passes,
> and
> > it does now).
> AFAIK DevTools only uses BeforeCompile event for script preprocessor.
> While script preprocessor receives a URL as a parameter I think we should be
> fine
> if it does not provide sourceURL at this point as script preprocessor is going
> to parse the script anyway and could extract the sourceURL annotation and
> probably even change it.

Alright, thanks. I conclude this CL (with the test fix) is safe to commit now.

[marja, 2014-07-02 07:01:45]

Committed patchset #18 manually as r22137 (presubmit successful).