Bug 64315 - Web Inspector: audit extensions need a way to link directly to resources
Summary: Web Inspector: audit extensions need a way to link directly to resources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 13:41 PDT by Boris Smus
Modified: 2011-07-20 00:54 PDT (History)
11 users (show)

See Also:


Attachments
Tentative patch for resourceLink FormattedValue (789 bytes, patch)
2011-07-11 13:41 PDT, Boris Smus
no flags Details | Formatted Diff | Diff
Add resourceLink FormattedValue (2.28 KB, patch)
2011-07-17 14:40 PDT, Boris Smus
no flags Details | Formatted Diff | Diff
resourceLink V2 (2.22 KB, patch)
2011-07-18 10:43 PDT, Boris Smus
no flags Details | Formatted Diff | Diff
resourceLink (2.24 KB, patch)
2011-07-18 14:21 PDT, Boris Smus
no flags Details | Formatted Diff | Diff
Patch (3.96 KB, patch)
2011-07-19 15:30 PDT, Boris Smus
no flags Details | Formatted Diff | Diff
Tests now pass and LayoutTests ChangeLog entry (4.74 KB, patch)
2011-07-19 16:12 PDT, Boris Smus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Smus 2011-07-11 13:41:34 PDT
Created attachment 100358 [details]
Tentative patch for resourceLink FormattedValue

What steps will reproduce the problem?
1. Develop a chrome devtools audit extension
2. Try using url FormattedValue to link directly to a resource in the current page that should be opened in the devtools

What is the expected output? What do you see instead?

This is impossible currently. We need a special kind of FormattedValue to reference resources. I propose adding a resourceLink API to provide this functionality.
Comment 1 Andrey Kosyakov 2011-07-13 05:50:49 PDT
Please prepare patch according to WebKit guidelines (see http://www.webkit.org/coding/contributing.html) -- it needs to use proper diff format and a ChangeLog entry.

Also, we use double quotes in the inspector code.
Comment 2 Boris Smus 2011-07-17 14:40:45 PDT
Created attachment 101119 [details]
Add resourceLink FormattedValue

Added ChangeLog
Updated patch to be in sync with latest SVN: r88326
Comment 3 Andrey Kosyakov 2011-07-18 02:22:50 PDT
Comment on attachment 101119 [details]
Add resourceLink FormattedValue

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

> WebCore/inspector/front-end/AuditFormatters.js:95
> +        var title = url.replace(/\\/g,"/").replace( /.*\//, "" ) + ":" + line;

- spaces within parenthesis in a second replace() are redundant;
- // in the second RegExp is likely to confuse MinJS that we use on chromium to minify the inspector code;
- do we really need first replace? I'd put it as something like title = url.replace(/.*[/\\]/, "")

> WebCore/inspector/front-end/AuditFormatters.js:99
> +        a.title = title;

We use the entire URL as a tooltip in other places, is using abbreviated URL here intentional?

> WebCore/inspector/front-end/AuditFormatters.js:100
> +        a.style = "max-width: 100%";

Does this line actually have effect?
Comment 4 Boris Smus 2011-07-18 10:41:55 PDT
(In reply to comment #3)
> (From update of attachment 101119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101119&action=review
> 
> > WebCore/inspector/front-end/AuditFormatters.js:95
> > +        var title = url.replace(/\\/g,"/").replace( /.*\//, "" ) + ":" + line;
> 
> - spaces within parenthesis in a second replace() are redundant;
> - // in the second RegExp is likely to confuse MinJS that we use on chromium to minify the inspector code;
> - do we really need first replace? I'd put it as something like title = url.replace(/.*[/\\]/, "")

Good idea. Simplified to one regex: /.*[\/\\]/ (escaping the "/")

> 
> > WebCore/inspector/front-end/AuditFormatters.js:99
> > +        a.title = title;
> 
> We use the entire URL as a tooltip in other places, is using abbreviated URL here intentional?

Not intentional. Changed.

> 
> > WebCore/inspector/front-end/AuditFormatters.js:100
> > +        a.style = "max-width: 100%";
> 
> Does this line actually have effect?

IIRC in my testing before, it to affect cases where the filename was very long (composed of a GUID). But I haven't been able to repro this, so removed from the patch.
Comment 5 Boris Smus 2011-07-18 10:43:13 PDT
Created attachment 101172 [details]
resourceLink V2
Comment 6 Andrey Kosyakov 2011-07-18 11:23:30 PDT
Comment on attachment 101172 [details]
resourceLink V2

Code LGTM, but the paths in the commit message appear malformed -- these should be relative to WebKit base directory (running prepare-ChangeLog in WebKit root may help).
Also, this should blow the layout tests -- please run LayoutTests/inspector/extensions/extensions-audits-api.html and rebaseline the expectations (also, it would be good idea to add createResourceLink to extensions-audits.html)
Note that extensions tests are not supported by chromium DumpRenderTree at the moment, so you will need WebKit build on either mac, win or Qt to run the tests (mac build is perhaps the easiest to do).
Comment 7 Boris Smus 2011-07-18 14:21:12 PDT
Created attachment 101203 [details]
resourceLink

Fixed ChangeLog paths to be relative to WebCore. Working on the tests.
Comment 8 Boris Smus 2011-07-19 15:30:53 PDT
Created attachment 101400 [details]
Patch
Comment 9 Boris Smus 2011-07-19 16:12:27 PDT
Created attachment 101407 [details]
Tests now pass and LayoutTests ChangeLog entry
Comment 10 Andrey Kosyakov 2011-07-19 16:14:34 PDT
Comment on attachment 101407 [details]
Tests now pass and LayoutTests ChangeLog entry

LGTM. Pavel/Yury, please have a look.
Comment 11 Andrey Kosyakov 2011-07-20 00:54:19 PDT
Comment on attachment 101407 [details]
Tests now pass and LayoutTests ChangeLog entry

Clearing flags on attachment: 101407

Committed r91346: <http://trac.webkit.org/changeset/91346>
Comment 12 Andrey Kosyakov 2011-07-20 00:54:27 PDT
All reviewed patches have been landed.  Closing bug.