[Webkit-unassigned] [Bug 40425] Web Inspector: provide API for content scripts to interact with the inspector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40425





--- Comment #17 from Andrey Kosyakov <caseq at chromium.org>  2010-07-28 10:02:35 PST ---
(In reply to comment #9)
> (From update of attachment 62568 [details])
> 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 ;

Fixed.

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

Let's do this later -- whatever JSDoc we may have in ExtensionsAPI.js would not map directly on API that is visible to user because of xxxImpl classes we use to hide internal properties in closures and because callback parameters are often formed on the server sides and passed by the API side as is.


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

Dropped it. The idea was that we may want to get extension panels in other extensions or in other iframes of the same extension, but we agreed that we don't want this for the moment.

> 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.
Done.

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

Why -- it doesn't cost us much?

> 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)

Fixed.

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

Let's make it a separate change -- it's a refactoring, is logically independent and will touch every panel.

> 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.

Removed attach, addPanelToolbarIcon is one layer up for consistency with other panels.

> 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.
Fixed.

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

Replaced magic strings with functions, i.e. this._stauts.E_NOTFOUND(message.panel);

> 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).

Fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.