[webkit-reviews] review granted: [Bug 120576] Web Inspector: Breakpoint Actions : [Attachment 210236] [PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS Editor

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 120576: Web Inspector: Breakpoint Actions
https://bugs.webkit.org/show_bug.cgi?id=120576

Attachment 210236: [PATCH] 3 - JS Runtime Completion in Breakpoint Actions JS
Editor
https://bugs.webkit.org/attachment.cgi?id=210236&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210236&action=review


> Source/WebInspectorUI/ChangeLog:13
> +	   The logic was inside of JavaScriptLogViewController but was already
> +	   entirely independent. Factor it out into its own class and plug it
into
> +	   CodeMirrorCompletionController as a "CompletionsProvider".

Nice!

> Source/WebInspectorUI/ChangeLog:56
> +2013-09-01  Joseph Pecoraro	<pecoraro at apple.com>
> +
> +	   Web Inspector: Breakpoint Actions
> +	   https://bugs.webkit.org/show_bug.cgi?id=120576
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

Oops!

> Source/WebInspectorUI/UserInterface/BreakpointActionView.js:160
> +	       completionController.setExtendedCompletionProvider("javascript",
WebInspector.JavaScriptRuntimeCompletionProvider.instance);

I think WebInspector.JavaScriptRuntimeCompletionProvider.instance is odd. I
would expect WebInspector.globalJavaScriptRuntimeCompletionProvider or
something.

> Source/WebInspectorUI/UserInterface/CodeMirrorCompletionController.js:95
> +    setExtendedCompletionProvider: function(modeName, provider)
> +    {
> +	   this._extendedCompletionProviders[modeName] = provider;
> +    },

I would almost call this "addExtendedCompletionProvider" so it can be multiple.
But that is just me wanting to avoid "set" which makes me want this to be a
property.