In many places we use linkifyLocation function to produce a link node which will updates automatically when the source file was changed on the fly. Such changes happen when we use pretty print or another operation that changes the source code somehow. linkifyLocation associates a new instance of updateAnchor closure with the each link node and add the closure to the SourceMappingUpdated event's list. As result the node<->closure pairs wouldn't be collected until reset the UI and DebuggerPresentationModel
Created attachment 109396 [details] Patch
Comment on attachment 109396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109396&action=review > Source/WebCore/inspector/front-end/TimelinePanel.js:1181 > + return WebInspector.debuggerPresentationModel.linkifyLocation(WebInspector.TimelinePanel.linkedNodesOwnerName, url, lineNumber, columnNumber, "timeline-details"); I would form it a bit differently to improve readability. I would introduce DebuggerPresentationModel.Linkifier and do this: this._linkifier = DebuggerPresentationModel.createLinkifier(); this._linkifier.linkifyLocation(url, lineNumber, ...) this._linkifier.reset() (or delete this._linkifier);
(In reply to comment #2) > (From update of attachment 109396 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109396&action=review > > > Source/WebCore/inspector/front-end/TimelinePanel.js:1181 > > + return WebInspector.debuggerPresentationModel.linkifyLocation(WebInspector.TimelinePanel.linkedNodesOwnerName, url, lineNumber, columnNumber, "timeline-details"); > > I would form it a bit differently to improve readability. > > I would introduce DebuggerPresentationModel.Linkifier and do this: > > this._linkifier = DebuggerPresentationModel.createLinkifier(); > this._linkifier.linkifyLocation(url, lineNumber, ...) > this._linkifier.reset() (or delete this._linkifier); Panel: WebInspector.TimelinePanel.linkifier = DebuggerPresentationModel.createLinkifier(); lalalalNode: WebInspector.TimelinePanel.linkifier.linkifyLocation(url, lineNumber, ...) panel.reset: WebInspector.TimelinePanel.linkifier.reset() (or delete this._linkifier);
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 109396 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=109396&action=review > > > > > Source/WebCore/inspector/front-end/TimelinePanel.js:1181 > > > + return WebInspector.debuggerPresentationModel.linkifyLocation(WebInspector.TimelinePanel.linkedNodesOwnerName, url, lineNumber, columnNumber, "timeline-details"); > > > > I would form it a bit differently to improve readability. > > > > I would introduce DebuggerPresentationModel.Linkifier and do this: > > > > this._linkifier = DebuggerPresentationModel.createLinkifier(); > > this._linkifier.linkifyLocation(url, lineNumber, ...) > > this._linkifier.reset() (or delete this._linkifier); > > Panel: > WebInspector.TimelinePanel.linkifier = DebuggerPresentationModel.createLinkifier(); you'll need to create this on instance of dpm. I would do WebInspector.debuggerPresentationModel.createLinkifier from within TimelinePanel constructor. > > lalalalNode: > WebInspector.TimelinePanel.linkifier.linkifyLocation(url, lineNumber, ...) should be defined on instance (this._linkifier ...) > > panel.reset: > WebInspector.TimelinePanel.linkifier.reset() (or delete this._linkifier);
Comment on attachment 109396 [details] Patch r- as per comments in the bug
Created attachment 109457 [details] Patch
Comment on attachment 109457 [details] Patch Attachment 109457 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9937004 New failing tests: http/tests/inspector/network/network-disabling-check-no-memory-leak.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/network/network-cachedresources-with-same-urls.html http/tests/inspector/console-cd.html http/tests/inspector/network/network-disable-cache-memory.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network/network-disable-cache-xhrs.html http/tests/inspector/network/download.html http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector/network-preflight-options.html http/tests/inspector/network/network-content-replacement-xhr.html http/tests/inspector/console-cd-completions.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html http/tests/inspector/change-iframe-src.html
Created attachment 109477 [details] Patch
Comment on attachment 109477 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109477&action=review > Source/WebCore/WebCore.gypi:6237 > + 'inspector/front-end/Linkifier.js', Do not forget about .vcproj! > Source/WebCore/inspector/front-end/EventListenersSidebarPane.js:59 > + WebInspector.EventListenersSidebarPane._linkifier = new WebInspector.Linkifier(); you should install these on instance. Installing on class in constructor does not make sense - it leaks linkifier anyways. > Source/WebCore/inspector/front-end/Linkifier.js:46 > + var rawSourceCode = WebInspector.debuggerPresentationModel._rawSourceCodeForScript(sourceURL); Extracting linkifier in its own class does not remote dependency from the DPM. Not sure what you were trying to achieve here. > Source/WebCore/inspector/front-end/ProfileDataGridTree.js:101 > + var urlElement = WebInspector.ProfilesPanel.linkifier.linkifyLocation(this.profileNode.url, lineNumber, 0, "profile-node-file"); Why not to create linkifier on the tree instead? > Source/WebCore/inspector/front-end/TimelinePanel.js:112 > + WebInspector.TimelinePanel._linkifier = new WebInspector.Linkifier(); ditto
Created attachment 109483 [details] Patch
Comment on attachment 109483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109483&action=review > Source/WebCore/inspector/front-end/Linkifier.js:36 > + this._model = model; _model -> _debuggerPresentationModel, alternatively you may add corresponding type annotation.
Committed r96500: <http://trac.webkit.org/changeset/96500>
Created attachment 109628 [details] Patch
Comment on attachment 109628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109628&action=review > LayoutTests/inspector/console/console-preserve-log.html:8 > + WebInspector.console.addMessage(WebInspector.consoleView.createTextMessage("PASS")); You should always use factory method when creating messages. > LayoutTests/inspector/debugger/linkifier.html:37 > + DummyPresentationModel.prototype = { Something is wrong with indentation. > Source/WebCore/inspector/front-end/ConsoleMessage.js:37 > + * @param {WebInspector.DebuggerPresentationModel.Linkifier=} linkifier move this parameter to after "message", make it non-optional > Source/WebCore/inspector/front-end/ConsoleModel.js:126 > + var msgCopy = WebInspector.consoleView.createMessage(msg.source, msg.type, msg.level, msg.line, msg.url, count - prevRepeatCount, msg._messageText, msg._parameters, msg._stackTrace, msg._request); ditto > Source/WebCore/inspector/front-end/ConsoleModel.js:177 > +WebInspector.ConsoleMessage.create = function(source, type, level, line, url, repeatCount, message, parameters, stackTrace, request, linkifier) These should not have linkifier parameter. > Source/WebCore/inspector/front-end/ConsoleModel.js:187 > +WebInspector.ConsoleMessage.createTextMessage = function(text, level, linkifier) ditto > Source/WebCore/inspector/front-end/ConsoleView.js:123 > + createMessage: function(source, type, level, line, url, repeatCount, message, parameters, stackTrace, request) WebInspector.ConsoleMessage.create should be defined in the bottom of this class. You will add WebInspector.consoleView.linkifier() into each of the created ConsoleMessageImpls. > Source/WebCore/inspector/front-end/ConsoleView.js:129 > + { createTextMessage > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:404 > + rawSourceCodeForScriptWithURL: function(sourceURL) This can't be exposed. > Source/WebCore/inspector/front-end/ExtensionServer.js:333 > + var consoleMessage = WebInspector.consoleView.createMessage( ditto > Source/WebCore/inspector/front-end/NetworkManager.js:-130 > - WebInspector.console.addMessage(WebInspector.ConsoleMessage.create(WebInspector.ConsoleMessage.MessageSource.Other, ditt > Source/WebCore/inspector/front-end/ProfileView.js:490 > + this._linkiier.reset(); This should not work! > Source/WebCore/inspector/front-end/inspector.js:1012 > + var msg = WebInspector.consoleView.createMessage( ditto
Created attachment 109634 [details] Patch
Comment on attachment 109634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109634&action=review > LayoutTests/inspector/debugger/linkifier.html:49 > + console.log("--- linkify fakeURL-1 first time ---"); console.log -> InspectorTest.addResult > Source/WebCore/inspector/front-end/ConsoleView.js:701 > + WebInspector.ConsoleCommandResult = function(result, wasThrown, originatingCommand, linkifier) Weird indent
Comment on attachment 109634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109634&action=review >> Source/WebCore/inspector/front-end/ConsoleView.js:701 >> + WebInspector.ConsoleCommandResult = function(result, wasThrown, originatingCommand, linkifier) > > Weird indent it was tab
Created attachment 109639 [details] Patch
Comment on attachment 109634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109634&action=review Looks good. > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:565 > + this._anchorsForURL = {}; We should be able to linkify console messages for anonymous scripts as well. Could you store anchors by rawSourceCode.id instead so we can easily support anonymous scripts? > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:609 > + if (!anchors) How could this happen?
Comment on attachment 109639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109639&action=review Overall looks good. Could you update it and let Pavel P. take a look? > LayoutTests/inspector/debugger/linkifier.html:21 > + DummyPresentationModel = function() You should define classes according the the style guidelines. > LayoutTests/inspector/debugger/linkifier.html:25 > + this.addEventListener = function(eventName, callback, object) ditto > LayoutTests/inspector/debugger/linkifier.html:37 > + DummyPresentationModel.prototype = { ditto > Source/WebCore/inspector/front-end/ConsoleView.js:701 > + WebInspector.ConsoleCommandResult = function(result, wasThrown, originatingCommand, linkifier) Weird space added. It is clear that it has been added, not removed.
Created attachment 109763 [details] Patch
the landed patch was rolled out. reopen the bug.
Comment on attachment 109763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109763&action=review Looks good. > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:565 > + this._anchorsForScriptId = {}; Please rename to anchorsForRawSourceCode or anchorsForRawSourceCodeId, because rawSourceCode.id is not a scriptId. > Source/WebCore/inspector/front-end/DebuggerPresentationModel.js:599 > + if (rawSourceCode) This check is not needed. You add entries in this._anchorsForScriptId only when rawSourceCode exists, and model._rawSourceCode is cleared only on navigation.
Created attachment 109765 [details] Patch
Comment on attachment 109765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109765&action=review > LayoutTests/inspector/debugger/linkifier.html:45 > + _rawSourceCodeForScript: function(id) nit: it should be "script".
Committed r96694: <http://trac.webkit.org/changeset/96694>