Bug 157715 - Web Inspector: use a consistent prefix for injected scripts
Summary: Web Inspector: use a consistent prefix for injected scripts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-14 14:24 PDT by BJ Burg
Modified: 2016-05-19 14:10 PDT (History)
12 users (show)

See Also:


Attachments
Proposed Fix (10.97 KB, patch)
2016-05-14 14:38 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (11.80 KB, patch)
2016-05-14 20:19 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.09 MB, application/zip)
2016-05-14 21:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (933.26 KB, application/zip)
2016-05-14 21:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.47 MB, application/zip)
2016-05-14 21:25 PDT, Build Bot
no flags Details
Proposed Fix (fix tests) (14.25 KB, patch)
2016-05-19 08:32 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch (2.23 KB, patch)
2016-05-19 13:28 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-05-14 14:24:33 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-05-14 14:24:43 PDT
<rdar://problem/26287188>
Comment 2 BJ Burg 2016-05-14 14:38:28 PDT
Created attachment 278947 [details]
Proposed Fix
Comment 3 Timothy Hatcher 2016-05-14 14:52:46 PDT
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?
Comment 4 BJ Burg 2016-05-14 20:08:03 PDT
(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.
Comment 5 BJ Burg 2016-05-14 20:19:46 PDT
Created attachment 278955 [details]
Proposed Fix
Comment 6 Build Bot 2016-05-14 21:04:32 PDT
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
Comment 7 Build Bot 2016-05-14 21:04:35 PDT
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
Comment 8 Build Bot 2016-05-14 21:08:00 PDT
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
Comment 9 Build Bot 2016-05-14 21:08:03 PDT
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
Comment 10 Build Bot 2016-05-14 21:25:40 PDT
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
Comment 11 Build Bot 2016-05-14 21:25:43 PDT
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
Comment 12 Timothy Hatcher 2016-05-16 11:17:40 PDT
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?
Comment 13 Joseph Pecoraro 2016-05-16 13:15:06 PDT
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.
Comment 14 BJ Burg 2016-05-16 15:21:58 PDT
(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.
Comment 15 BJ Burg 2016-05-19 08:32:53 PDT
Created attachment 279390 [details]
Proposed Fix (fix tests)
Comment 16 BJ Burg 2016-05-19 09:47:32 PDT
Committed r201168: <http://trac.webkit.org/changeset/201168>
Comment 17 Alexey Proskuryakov 2016-05-19 12:16:20 PDT
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>.
Comment 18 BJ Burg 2016-05-19 13:16:43 PDT
(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.
Comment 20 BJ Burg 2016-05-19 13:25:39 PDT
(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.
Comment 21 BJ Burg 2016-05-19 13:28:13 PDT
Reopening to attach new patch.
Comment 22 BJ Burg 2016-05-19 13:28:16 PDT
Created attachment 279422 [details]
Patch
Comment 23 WebKit Commit Bot 2016-05-19 14:10:01 PDT
Comment on attachment 279422 [details]
Patch

Clearing flags on attachment: 279422

Committed r201181: <http://trac.webkit.org/changeset/201181>
Comment 24 WebKit Commit Bot 2016-05-19 14:10:06 PDT
All reviewed patches have been landed.  Closing bug.