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.
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.
Created attachment 101119 [details] Add resourceLink FormattedValue Added ChangeLog Updated patch to be in sync with latest SVN: r88326
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?
(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.
Created attachment 101172 [details] resourceLink V2
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).
Created attachment 101203 [details] resourceLink Fixed ChangeLog paths to be relative to WebCore. Working on the tests.
Created attachment 101400 [details] Patch
Created attachment 101407 [details] Tests now pass and LayoutTests ChangeLog entry
Comment on attachment 101407 [details] Tests now pass and LayoutTests ChangeLog entry LGTM. Pavel/Yury, please have a look.
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>
All reviewed patches have been landed. Closing bug.