Web Inspector: audit extensions need a way to link directly to resources

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Boris Smus]

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.

[Andrey Kosyakov]

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.

[Boris Smus]

Created attachment 101119 [details]
Add resourceLink FormattedValue

Added ChangeLog
Updated patch to be in sync with latest SVN: r88326

[Andrey Kosyakov]

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?

[Boris Smus]

(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.

[Boris Smus]

Created attachment 101172 [details]
resourceLink V2

[Andrey Kosyakov]

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).

[Boris Smus]

Created attachment 101203 [details]
resourceLink

Fixed ChangeLog paths to be relative to WebCore. Working on the tests.

[Boris Smus]

Created attachment 101400 [details]
Patch

[Boris Smus]

Created attachment 101407 [details]
Tests now pass and LayoutTests ChangeLog entry

[Andrey Kosyakov]

Comment on attachment 101407 [details]
Tests now pass and LayoutTests ChangeLog entry

LGTM. Pavel/Yury, please have a look.

[Andrey Kosyakov]

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>

[Andrey Kosyakov]

All reviewed patches have been landed.  Closing bug.