[webkit-reviews] review denied: [Bug 69146] Web Inspector: debuggerPresentatioModel.linkifyLocation leaks updateAnchor closure instances. : [Attachment 109628] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 69146: Web Inspector: debuggerPresentatioModel.linkifyLocation leaks
updateAnchor closure instances.
https://bugs.webkit.org/show_bug.cgi?id=69146

Attachment 109628: Patch
https://bugs.webkit.org/attachment.cgi?id=109628&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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("PAS
S"));

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