[webkit-reviews] review granted: [Bug 40425] Web Inspector: provide API for content scripts to interact with the inspector : [Attachment 62568] patch (EOLs fixed)

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 40425: Web Inspector: provide API for content scripts to interact with the
inspector
https://bugs.webkit.org/show_bug.cgi?id=40425

Attachment 62568: patch (EOLs fixed)
https://bugs.webkit.org/attachment.cgi?id=62568&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
WebCore/inspector/front-end/ExtensionAPI.js:34
 +  function EventSink(type)
Add comment claiming that it is not externally visible.

WebCore/inspector/front-end/ExtensionAPI.js:66
 +	    for (listener in this._listeners)
Never fire while iterating over collection.

WebCore/inspector/front-end/ExtensionAPI.js:69
 +  };
no need for ;

WebCore/inspector/front-end/ExtensionAPI.js:92
 +	getAll: function(callback)
Lets start using JSDoc here.

WebCore/inspector/front-end/ExtensionAPI.js:123
 +	get: function(id)
I am not sure we need this accessor.

WebCore/inspector/front-end/ExtensionAPI.js:146
 +	    extensionServer.sendRequest({ command: "createPanel", id: id,
label: label, url: expandURL(pageURL), icon: expandURL(iconURL) }, callback &&
bind(callbackWrapper, this));
i'd start formatting these vertically.

WebCore/inspector/front-end/ExtensionAPI.js:174
 +	this.onSearch = new EventSink("panel-search-" + id);
Lets save onSearch until v5? :)

WebCore/inspector/front-end/ExtensionAPI.js:159
 +	    var id = "extension-sidebar-" + extensionServer.nextObjectId;
make this a function

WebCore/inspector/front-end/ExtensionAPI.js:193
 +	setExpansion: function(state)
setExpanded(expanded)

WebCore/inspector/front-end/ExtensionPanel.js:33
 +	this.toolbarItemLabel = label;
Lets pull this property up and add it into the Panel's constructor.

WebCore/inspector/front-end/ExtensionPanel.js:36
 +	this.attach(); // Extension panel has to be attached so that extension
can see it in DOM tree
You only need to call WebInspector.addPanelToolbarIcon here.

WebCore/inspector/front-end/ExtensionServer.js:125
 +	    WebInspector.addPanelToolbarIcon(toolbarElement, panel,
lastToolbarItem);
Ok.

WebCore/inspector/front-end/ExtensionServer.js:126
 +	    WebInspector.panels[id] = panel;
This can kill all our code.

WebCore/inspector/front-end/ExtensionServer.js:134
 +		return this._status("E_NOTFOUND", message.panel);
Nice magic strings!

WebCore/inspector/front-end/ExtensionServer.js:226
 +	    InspectorExtensionRegistry.getExtensions();
getExtensionsAsync

WebCore/inspector/front-end/ExtensionServer.js:235
 +	_addExtension: function(extension)
inline ?

WebCore/inspector/front-end/InjectedScript.js:246
 +	    if (value != null && typeof value !== "string")
I'd do try / catch JSON.stringify.

WebCore/inspector/front-end/inspector.js:1208
 +	    this.extensionServer.notifyObjectCreated("resource",
WebInspector.extensionServer.convertResource(resource));
here and below, only use constants for notifyObjectCreated and postEvent. Or do
postResetEvent.

WebCore/inspector/front-end/inspector.js:1606
 +  WebInspector.addExtensions = function(extensions)
You can define this method in ExtensionServer.js and made server's one private
to the unit.

General comment on tests: you should add a test for extension-facing API that
would enumerate and whitelist all the accessible properties (see window.*
tests).