[devtools] Remove trailing fragment identifier from script urls

[devtools] Remove trailing fragment identifier from script urls

Scripts should be identified by resource and text range.
Fragement identifiers on urls may cause URLs for the same
resource to not match. Therefore: if the URL supplied to ScriptSourceCode has a fragment identifier, remove it.
    
Add LayoutTest for inlined-script breakpoints with fragment id.


BUG=306239
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162138

[johnjbarton, 2013-10-11 19:02:09]

Feedback please. I'll look into a test if this fix is close.

[abarth-chromium, 2013-10-11 20:20:41]

https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
Source/bindings/v8/ScriptDebugServer.cpp:508: size_t hashCharPos =
rawScriptURL.find("#");
This seems like the wrong layer for this work.  The job of the bindings layer is
just to do type conversions.  All "application logic" should be handled outside
the bindings.

Also, shouldn't we use a real URL parser?

https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
Source/bindings/v8/ScriptDebugServer.cpp:513: }
This branch isn't needed.  substring(0, kNotFound) will just return the whole
string without making a copy.

[johnjbarton, 2013-10-11 21:31:16]

Advice on trying to fix this in WebScriptSource?

https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
Source/bindings/v8/ScriptDebugServer.cpp:508: size_t hashCharPos =
rawScriptURL.find("#");
On 2013/10/11 20:20:41, abarth wrote:
> This seems like the wrong layer for this work.  The job of the bindings layer
is
> just to do type conversions.  All "application logic" should be handled
outside
> the bindings.

The debugger layer (listener here) gets a const Script&. I could change
didParseSource() to take Script&, but that seems wrong.

The other place I can fix it is before we compile. Two classes represent JS it
seems, one is ScriptSourceCode but that is still in bindings. The other is
WebScriptSource. Construction of WebScriptSource from a string and a URL could
remove the fragment identifier.

> Also, shouldn't we use a real URL parser?

WebScriptSource has a WebURL with a path to GURL and GURL::PathForRequest()
which computes the string we would want to use for scripts.

https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
Source/bindings/v8/ScriptDebugServer.cpp:513: }
On 2013/10/11 20:20:41, abarth wrote:
> This branch isn't needed.  substring(0, kNotFound) will just return the whole
> string without making a copy.

If I understand you, I could have written:
   script.url = script.url.substring(0, script.url.find("#"));
?

[abarth-chromium, 2013-10-13 05:11:24]

On 2013/10/11 21:31:16, johnjbarton wrote:
> If I understand you, I could have written:
>    script.url = script.url.substring(0, script.url.find("#"));
> ?

Correct, but you shouldn't be writing that code at all.  Instead of parsing URLs
with string operations, we should use a real URL parser.

[johnjbarton, 2013-10-16 18:03:57]

Adam, PTAL bindings code.

Still needs a test and then a review from devtools team. Test is blocked by
another bug.

On 2013/10/11 20:20:41, abarth wrote:
>
https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
> File Source/bindings/v8/ScriptDebugServer.cpp (right):
> 
>
https://codereview.chromium.org/26538007/diff/1/Source/bindings/v8/ScriptDebu...
> Source/bindings/v8/ScriptDebugServer.cpp:508: size_t hashCharPos =
> rawScriptURL.find("#");
> This seems like the wrong layer for this work.  The job of the bindings layer
is
> just to do type conversions.  All "application logic" should be handled
outside
> the bindings.

The new patch looks more like a type conversion. The URL returned by
ScriptSourceCode.url() is now correct for scripts. 

> 
> Also, shouldn't we use a real URL parser?

Done.

[aandrey, 2013-10-16 18:34:02]

John, please re-upload the patch base files.

[johnjbarton, 2013-10-16 19:01:14]

On 2013/10/16 18:34:02, aandrey wrote:
> John, please re-upload the patch base files.

PTAL

[abarth-chromium, 2013-10-17 01:06:55]

Thanks, this is much better.  One remaining issue.

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptSourceCode.h:54: m_url.removeFragmentIdentifier();
Ah, much better.

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptSourceCode.h:77: return m_url;
Why not store the data in m_url instead of in ScriptResource?  We already have
storage for it here.  There's no reason to bloat the memory cache with yet
another KURL member.

https://codereview.chromium.org/26538007/diff/26001/Source/core/fetch/ScriptR...
File Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/26538007/diff/26001/Source/core/fetch/ScriptR...
Source/core/fetch/ScriptResource.cpp:93: return m_url;
Why do we need to store this data?  Is it really too expensive to recompute it
each time?

[johnjbarton, 2013-10-17 02:30:11]

Adam, I answered your questions as I understand them, I see one addition
improvement I can make, but please check my reasoning.

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptSourceCode.h:77: return m_url;
On 2013/10/17 01:06:55, abarth wrote:
> Why not store the data in m_url instead of in ScriptResource?  We already have
> storage for it here.  There's no reason to bloat the memory cache with yet
> another KURL member.

Maybe my C++ is rusty, but the method here is const so we can't mutate the m_url
to update it with m_resource scriptURL()

https://codereview.chromium.org/26538007/diff/26001/Source/core/fetch/ScriptR...
File Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/26538007/diff/26001/Source/core/fetch/ScriptR...
Source/core/fetch/ScriptResource.cpp:93: return m_url;
On 2013/10/17 01:06:55, abarth wrote:
> Why do we need to store this data?  

I believe we need to store this data so the const KURL& has something to point
to.

> Is it really too expensive to recompute it
> each time?

No, the only time we need to store is if we call scriptURL (already true) and if
we have a fragment id. So I could check the response().url() for a fragment,
update in that case, else return the response().url(). That way the
ScriptResource.m_url is an empty KURL except in the case we are using # URLs.

[abarth-chromium, 2013-10-17 02:39:04]

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptSourceCode.h:77: return m_url;
On 2013/10/17 02:30:11, johnjbarton wrote:
> On 2013/10/17 01:06:55, abarth wrote:
> > Why not store the data in m_url instead of in ScriptResource?  We already
have
> > storage for it here.  There's no reason to bloat the memory cache with yet
> > another KURL member.
> 
> Maybe my C++ is rusty, but the method here is const so we can't mutate the
m_url
> to update it with m_resource scriptURL()

You can mark the m_url member as |mutable|.

https://codereview.chromium.org/26538007/diff/26001/Source/core/fetch/ScriptR...
File Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/26538007/diff/26001/Source/core/fetch/ScriptR...
Source/core/fetch/ScriptResource.cpp:93: return m_url;
On 2013/10/17 02:30:11, johnjbarton wrote:
> On 2013/10/17 01:06:55, abarth wrote:
> > Why do we need to store this data?  
> 
> I believe we need to store this data so the const KURL& has something to point
> to.

We can return a KURL rather than a const KURL&

> > Is it really too expensive to recompute it
> > each time?
> 
> No, the only time we need to store is if we call scriptURL (already true) and
if
> we have a fragment id. So I could check the response().url() for a fragment,
> update in that case, else return the response().url(). That way the
> ScriptResource.m_url is an empty KURL except in the case we are using # URLs.

[johnjbarton, 2013-10-23 17:48:15]

PTAL

On 2013/10/17 02:39:04, abarth wrote:
>
https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
> File Source/bindings/v8/ScriptSourceCode.h (right):
> 
>
https://codereview.chromium.org/26538007/diff/26001/Source/bindings/v8/Script...
> Source/bindings/v8/ScriptSourceCode.h:77: return m_url;
> On 2013/10/17 02:30:11, johnjbarton wrote:
> > On 2013/10/17 01:06:55, abarth wrote:
> > > Why not store the data in m_url instead of in ScriptResource?  We already
> have
> > > storage for it here.  There's no reason to bloat the memory cache with yet
> > > another KURL member.
> > 
> > Maybe my C++ is rusty, but the method here is const so we can't mutate the
> m_url
> > to update it with m_resource scriptURL()
> 
> You can mark the m_url member as |mutable|.

Done.

[johnjbarton, 2013-10-24 17:05:12]

LayoutTest added, the patch is ready for review.

[vsevik, 2013-11-05 13:51:28]

Adam, could you please have another look?

Layout test lgtm.

https://chromiumcodereview.appspot.com/26538007/diff/94002/LayoutTests/inspec...
File LayoutTests/inspector/debugger/debug-inlined-scripts-fragment-id.html
(right):

https://chromiumcodereview.appspot.com/26538007/diff/94002/LayoutTests/inspec...
LayoutTests/inspector/debugger/debug-inlined-scripts-fragment-id.html:34: // Use
fake resources to make sure that scripts will be loaded from JavaScript VM.
I don't think you need this.

[johnjbarton, 2013-11-11 18:50:18]

On 2013/11/05 13:51:28, vsevik wrote:
> Adam, could you please have another look?
> 
> Layout test lgtm.

Thanks.

> 
>
https://chromiumcodereview.appspot.com/26538007/diff/94002/LayoutTests/inspec...
> File LayoutTests/inspector/debugger/debug-inlined-scripts-fragment-id.html
> (right):
> 
>
https://chromiumcodereview.appspot.com/26538007/diff/94002/LayoutTests/inspec...
> LayoutTests/inspector/debugger/debug-inlined-scripts-fragment-id.html:34: //
Use
> fake resources to make sure that scripts will be loaded from JavaScript VM.
> I don't think you need this.

Done.

[pfeldman, 2013-11-15 20:36:15]

lgtm

[commit-bot: I haz the power, 2013-11-15 20:38:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/26538007/174001

[commit-bot: I haz the power, 2013-11-15 21:50:57]

Change committed as 162138