Bug 157715 – Web Inspector: use a consistent prefix for injected scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

Proposing __InjectedScript_FooBar.js. This way all the injected scripts show up in the resources sidebar at the top but they still have the full source filename for easy grepping.

[Radar WebKit Bug Importer]

<rdar://problem/26287188>

[Blaze Burg]

Created attachment 278947 [details]
Proposed Fix

[Timothy Hatcher]

Comment on attachment 278947 [details]
Proposed Fix

You need to change this function in Web Inspector to match, otherwise these get seen by end-users.

function isWebKitInternalScript(url)
{
    if (isWebInspectorConsoleEvaluationScript(url))
        return false;
    return url && url.startsWith("__Web") && url.endsWith("__");
}

Why isn't the current __Web prefix good enough?

[Blaze Burg]

(In reply to comment #3)
> Comment on attachment 278947 [details]
> Proposed Fix
> 
> You need to change this function in Web Inspector to match, otherwise these
> get seen by end-users.
> 
> function isWebKitInternalScript(url)
> {
>     if (isWebInspectorConsoleEvaluationScript(url))
>         return false;
>     return url && url.startsWith("__Web") && url.endsWith("__");
> }
> 
> Why isn't the current __Web prefix good enough?

I want the original source file in the repository to be the suffix of the sourceURL. __InjectedScript_{foo}.js seemed better than __Web{foo}__ because you can easily guess what filename to search for.

[Blaze Burg]

Created attachment 278955 [details]
Proposed Fix

[Build Bot]

Comment on attachment 278955 [details]
Proposed Fix

Attachment 278955 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1323252

New failing tests:
inspector/debugger/sourceURLs.html
inspector/debugger/scriptParsed.html
transitions/default-timing-function.html

[Build Bot]

Created attachment 278956 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278955 [details]
Proposed Fix

Attachment 278955 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1323259

New failing tests:
inspector/debugger/sourceURLs.html
inspector/debugger/scriptParsed.html

[Build Bot]

Created attachment 278957 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 278955 [details]
Proposed Fix

Attachment 278955 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1323264

New failing tests:
inspector/debugger/sourceURLs.html
inspector/debugger/scriptParsed.html

[Build Bot]

Created attachment 278958 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Timothy Hatcher]

Comment on attachment 278955 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=278955&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1315
>      return url && url.startsWith("__Web") && url.endsWith("__");

Is this still a case we need to check?

[Joseph Pecoraro]

Comment on attachment 278955 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=278955&action=review

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1315
>>      return url && url.startsWith("__Web") && url.endsWith("__");
> 
> Is this still a case we need to check?

I think so, for appendWebInspectorSourceURL.

[Blaze Burg]

(In reply to comment #13)
> Comment on attachment 278955 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278955&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1315
> >>      return url && url.startsWith("__Web") && url.endsWith("__");
> > 
> > Is this still a case we need to check?
> 
> I think so, for appendWebInspectorSourceURL.

What he said. We don't want to persist breakpoints in one-shot __WebInspectorInternal__ and console evaluation scripts.

[Blaze Burg]

Created attachment 279390 [details]
Proposed Fix (fix tests)

[Blaze Burg]

Committed r201168: <http://trac.webkit.org/changeset/201168>

[Alexey Proskuryakov]

What's the status of this bug? Is the "fix tests" patch still relevant?

inspector/debugger/scriptParsed.html fails most of the time on the bots: <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdebugger%2FscriptParsed.html>.

[Blaze Burg]

(In reply to comment #17)
> What's the status of this bug? Is the "fix tests" patch still relevant?
> 
> inspector/debugger/scriptParsed.html fails most of the time on the bots:
> <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fdebugger%2FscriptParsed.html>.

I landed the most recent patch (the one with test fixes) because it worked locally and EWS was green. I will look into the scriptParsed.html failures. Right now, the flakiness dashboard is not loading for me, probably due to my internet connection.

[Alexey Proskuryakov]

Failures look like this:

https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/r201175%20(6186)/inspector/debugger/scriptParsed-diff.txt

[Blaze Burg]

(In reply to comment #18)
> (In reply to comment #17)
> > What's the status of this bug? Is the "fix tests" patch still relevant?
> > 
> > inspector/debugger/scriptParsed.html fails most of the time on the bots:
> > <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=inspector%2Fdebugger%2FscriptParsed.html>.
> 
> I landed the most recent patch (the one with test fixes) because it worked
> locally and EWS was green. I will look into the scriptParsed.html failures.
> Right now, the flakiness dashboard is not loading for me, probably due to my
> internet connection.

Looking at the test, it seems that the check for an InjectedScript is winning over the check for CommandLineAPIModuleSource, since the latter is now also classified as the former. I think it should be sufficient to switch the two checks so we try the more specific match first.

[Blaze Burg]

Reopening to attach new patch.

[Blaze Burg]

Created attachment 279422 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279422 [details]
Patch

Clearing flags on attachment: 279422

Committed r201181: <http://trac.webkit.org/changeset/201181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.