Bug 65612

Summary: Web Inspector: draft implementation of source mapping listeners.
Product: WebKit Reporter: Pavel Podivilov <podivilov>
Component: Web Inspector (Deprecated)Assignee: Pavel Podivilov <podivilov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch pfeldman: review+

Description Pavel Podivilov 2011-08-03 05:31:58 PDT 
Web Inspector: draft implementation of source mapping listeners.

Refactor anchors creation: move anchor updating code to ScriptsPanel since we have model.addSourceMappingListener now.
Comment 1 Pavel Podivilov 2011-08-03 05:32:57 PDT 
Created attachment 102768 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-03 05:56:18 PDT 
Comment on attachment 102768 [details]
Patch

Attachment 102768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9304015

New failing tests:
http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html
Comment 3 Pavel Podivilov 2011-08-03 07:19:05 PDT 
Created attachment 102783 [details]
Patch
Comment 4 Pavel Feldman 2011-08-04 03:08:39 PDT 
Comment on attachment 102783 [details]
Patch

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

> Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:125
> +        this._sourceMappingListeners.push(listener);

Nit: You could do addEventListener(sourceId, listener) followed by a dispatchEvent(this.sourceId)...

> Source/WebCore/inspector/front-end/ScriptsPanel.js:572
> +            model.addSourceMappingListener(url, null, formatAnchor);

Could we test anchor update?

> Source/WebCore/inspector/front-end/inspector.js:1323
> +WebInspector._linkifyLocation = function(url, oneBasedLineNumber, oneBasedColumnNumber, classes, tooltipText, preferredPanel)

Call sites should probably do WebInspector.debuggerPresentationModel.linkifyLocation(location), where location is raw 'script' location.

> Source/WebCore/inspector/front-end/inspector.js:1325
> +    if (preferredPanel === "scripts" && !WebInspector.debuggerModel.scriptsForURL(url).length)

Either this code or WebInspector._showAnchorLocation should fall back to "resources".
Comment 5 Pavel Podivilov 2011-08-04 06:04:02 PDT 
Created attachment 102902 [details]
Patch
Comment 6 Pavel Podivilov 2011-08-04 06:11:21 PDT 
(In reply to comment #4)
> (From update of attachment 102783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102783&action=review
> 
> > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:125
> > +        this._sourceMappingListeners.push(listener);
> 
> Nit: You could do addEventListener(sourceId, listener) followed by a dispatchEvent(this.sourceId)...

Unfortunately, sourceId isn't a good id since there may be several scripts linked to one RawSourceCode instance.

> 
> > Source/WebCore/inspector/front-end/ScriptsPanel.js:572
> > +            model.addSourceMappingListener(url, null, formatAnchor);
> 
> Could we test anchor update?

It is tested by testConsoleMessagesForFormattedScripts in script-formatter.html.

> 
> > Source/WebCore/inspector/front-end/inspector.js:1323
> > +WebInspector._linkifyLocation = function(url, oneBasedLineNumber, oneBasedColumnNumber, classes, tooltipText, preferredPanel)
> 
> Call sites should probably do WebInspector.debuggerPresentationModel.linkifyLocation(location), where location is raw 'script' location.

Done.

> 
> > Source/WebCore/inspector/front-end/inspector.js:1325
> > +    if (preferredPanel === "scripts" && !WebInspector.debuggerModel.scriptsForURL(url).length)
> 
> Either this code or WebInspector._showAnchorLocation should fall back to "resources".

Not all anchors are created by linkifyResourceAsNode, some of them don't have preferred_panel attribute. Code in WebInspector._showAnchorLocation is needed for such anchors.
Comment 7 WebKit Review Bot 2011-08-04 06:27:30 PDT 
Comment on attachment 102902 [details]
Patch

Attachment 102902 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9303457

New failing tests:
http/tests/inspector/console-resource-errors.html
http/tests/inspector/console-xhr-logging.html
Comment 8 Pavel Podivilov 2011-08-04 06:46:58 PDT 
Created attachment 102905 [details]
Patch
Comment 9 Pavel Podivilov 2011-08-04 07:07:31 PDT 
Created attachment 102909 [details]
Patch
Comment 10 Pavel Podivilov 2011-08-04 07:08:30 PDT 
Created attachment 102910 [details]
Patch
Comment 11 Pavel Feldman 2011-08-05 08:47:35 PDT 
Comment on attachment 102910 [details]
Patch

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

> Source/WebCore/inspector/front-end/EventListenersSidebarPane.js:240
> +            var url = this.eventListener.location.scriptId;

Please add yet another FIXME on the location structure + use of column.

> Source/WebCore/inspector/front-end/ProfileDataGridTree.js:101
> +            var urlElement = WebInspector.debuggerPresentationModel.linkifyLocation(this.profileNode.url, lineNumber, 0, "profile-node-file");

Do you know what url is?

> Source/WebCore/inspector/front-end/TimelinePanel.js:1128
> +                    return this._linkifyLocation(this.stackTrace[0].url, this.stackTrace[0].lineNumber, this.stackTrace[0].columnNumber);

Extract method receiving ConsoleCallFrame
Comment 12 Pavel Podivilov 2011-08-08 08:51:03 PDT 
Committed r92598: <http://trac.webkit.org/changeset/92598>