Bug 40425 – Web Inspector: provide API for content scripts to interact with the inspector

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Kosyakov]

Rationale for supporting extensions:
- Integration for existing development tools into WebInspector
- Stuff that is not generic enough to be a part of WebKit (audits/validators, support for specific server frameworks or client libraries)
- Stuff with incompatible licenses, lack of browser support etc.

Proposed integration points:
- UI integration: be able to create a pane or add a sidebar to an existing pane, request inspector to display particular resource etc.
- access inspector objects associated with the inspected page, such as resource records, console messages, potentially scripts etc.
- ability to evaluate JS code on inspected page (to access DOM and JS state);

[Andrey Kosyakov]

Created attachment 58376 [details]
Extension API draft

This is very preliminary, just to get discussion started.

[Andrey Kosyakov]

Created attachment 58377 [details]
A demo of extension in action

This one is pretty useless, but gives an idea of what is currently supported.

[Andrey Kosyakov]

Created attachment 62567 [details]
patch

[WebKit Review Bot]

Attachment 62567 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/front-end/ExtensionServer.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/inspector/front-end/ExtensionRegistryStub.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/inspector/front-end/ExtensionAPI.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 646 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrey Kosyakov]

Created attachment 62568 [details]
patch (EOLs fixed)

[Andrey Kosyakov]

Created attachment 62578 [details]
updated API draft

[Pavel Feldman]

Comment on attachment 62568 [details]
patch (EOLs fixed)

I started it, but got distracted. Will continue a bit later.

WebCore/inspector/InspectorController.cpp:461
 +      if (m_scriptsToEvaluateOnLoad.size()) {
Why did this move? Looks fragile in case something depends on such a move internally.

WebCore/inspector/InspectorFrontendHost.cpp:195
 +  void InspectorFrontendHost::setExtensionAPI(const String& script)
Here and below: we should refer to extensions api as to plural. Also, we might want to make this functionality generic, no need to explicitly mention "extension" here.



WebCore/inspector/front-end/ElementsPanel.js:61
 +              WebInspector.extensionServer.notifyObjectSelected("elements", "DOMNode");
extensionsServer?

WebCore/inspector/front-end/ElementsPanel.js:61
 +              WebInspector.extensionServer.notifyObjectSelected("elements", "DOMNode");
What does "elements" stand for? Should it be a const?


WebCore/inspector/front-end/ExtensionAPI.js:31
 +  var extensionAPI = function(InjectedScriptHost, inspectedWindow, injectedScriptId)
extensionsAPI

WebCore/inspector/front-end/ExtensionAPI.js:31
 +  var extensionAPI = function(InjectedScriptHost, inspectedWindow, injectedScriptId)
This looks as if it is injected into the target page. You don't use InjectedScriptHost, inspectWindow and injectedScriptID, do you?

WebCore/inspector/front-end/ExtensionAPI.js:55
 +                  listeners[i] = listeners[listeners.length - 1];
splice?

[Andrey Kosyakov]

(In reply to comment #7)
> (From update of attachment 62568 [details])
> I started it, but got distracted. Will continue a bit later.
> 
> WebCore/inspector/InspectorController.cpp:461
>  +      if (m_scriptsToEvaluateOnLoad.size()) {
> Why did this move? Looks fragile in case something depends on such a move internally.

It just happened to be put at the wrong place initially -- it gets injected too late in certain cases (namely, for iframes that get attached to a a detached tree initially, and then the tree is attached to the document). The only thing that depends on this is workers debugging and it still works.

'
> 
> WebCore/inspector/InspectorFrontendHost.cpp:195
>  +  void InspectorFrontendHost::setExtensionAPI(const String& script)
> Here and below: we should refer to extensions api as to plural. 

Will fix.

> Also, we might want to make this functionality generic, no need to explicitly mention "extension" here.

Well, I guess these other APIs would still qualify as extensions, wouldn't they?

> WebCore/inspector/front-end/ElementsPanel.js:61
>  +              WebInspector.extensionServer.notifyObjectSelected("elements", "DOMNode");
> extensionsServer?
> 
> WebCore/inspector/front-end/ElementsPanel.js:61
>  +              WebInspector.extensionServer.notifyObjectSelected("elements", "DOMNode");
> What does "elements" stand for? Should it be a const?

It's a panel id. Agree, will make a const.

> WebCore/inspector/front-end/ExtensionAPI.js:31
>  +  var extensionAPI = function(InjectedScriptHost, inspectedWindow, injectedScriptId)
> extensionsAPI
> 
> WebCore/inspector/front-end/ExtensionAPI.js:31
>  +  var extensionAPI = function(InjectedScriptHost, inspectedWindow, injectedScriptId)
> This looks as if it is injected into the target page. You don't use InjectedScriptHost, inspectWindow and injectedScriptID, do you?
> 

It is injected into the front-end page, but otherwise it's the same as if it were a tatget page (the means used to do injection are the same). injectedScriptId is actually used to derive unique object ids on the client side.

> WebCore/inspector/front-end/ExtensionAPI.js:55
>  +                  listeners[i] = listeners[listeners.length - 1];
> splice?

Yes!

[Pavel Feldman]

Comment on attachment 62568 [details]
patch (EOLs fixed)

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

[Pavel Feldman]

We are going to land this with the provision status. It is experimental, by no means it is final. Tim, Joe, we'd like to open discussion about this can you look at it and post comments / chat over IRC / we could talk using Skype.

[Timothy Hatcher]

A few comments:

* The access point should be webInspector or WebInspector, not webinspector.
* The use of Chrome style event listeners makes me sad. I think event sinks are ugly (the onFoo naming) and prefer the DOM addEventListener design.
* webinspector.panels.setExpanded(boolean expandedState) is listed under ExtensionSidebarPane, but that line of code is using the panels object not a sidebar. is that correct?
* webinspector.panels.revealAndSelect(string type, any id) does this select the right panel? Where do I get the id to use?
* All the "The callback parameter should specify a function that looks like this" examples are code snippets that are not valid JavaScript.

[Andrey Kosyakov]

(In reply to comment #11)
> A few comments:
> 
> * The access point should be webInspector or WebInspector, not webinspector.

Agree, will rename to webInspector.

> * The use of Chrome style event listeners makes me sad. I think event sinks are ugly (the onFoo naming) and prefer the DOM addEventListener design.

There are couple of practical reasons (aside from mimicking Chrome :)) I thought it's going to be better this way:
- We don't rely on string constants, so whatever typos may be in event type, these are going to be quickly caught in the runtime (i.e. "Cannot call method 'addListener' of undefined", vs. quietly swallowing invalid event types as DOM does). We're generally trying to get rid of any magic strings in the interface for similar reasons. And those with smart IDEs (or using our console) are going to enjoy name completion ;)
- These aren't really DOM events, so any similarity may actually be confusing. There's also no support for bubbling, so third argument to addEventListener() wouldn't is not applicable either.

If you're still feeling bad because it's not consistent with Safari extension API style, we can have some wrappers, so that webInspector.foo.addEventListener(bar, func) translates into webInspector.foo.onBar.addListener(func).

> * webinspector.panels.setExpanded(boolean expandedState) is listed under ExtensionSidebarPane, but that line of code is using the panels object not a sidebar. is that correct?

Oops, sorry -- this is a bug in template that is used to render the API schema (the schema itself is correct). I borrowed template from Chrome extension API docs, which is somewhat too specific for Chrome API conventions. I fixed some stuff there to fit our API  better, but some bugs still remain. I'm only publishing it this way because it's better than nothing -- we need some API reference for convenience of review, but this is not appropriate for the end users. I guess we may want to supply just the JSON schema as part of WebCore, and rendering it to HTML would be specific for the browser and in accordance with browser's documentation conventions.

> * webinspector.panels.revealAndSelect(string type, any id) does this select the right panel? 

Again, sorry for the poor rendering -- this is the method of the panel object, so it's mean to switch to given panel and cause the panel to display specified object.

> Where do I get the id to use?

This is the ids you get in onCreated events or with getAll methods. We only have this for resources so far, but the plan is to have similar methods for console messages and scripts.

> * All the "The callback parameter should specify a function that looks like this" examples are code snippets that are not valid JavaScript.

So are most of other function "signatures" -- obviously, we have to choose between showing valid JS and being able to specify types and other possible attributes. Again, this is just the template that I borrowed and I realize the rendering may be questionable.

[Joseph Pecoraro]

> webinspector.panels.onSelectionChanged
> Fired when an objects (script, resource, DOM element etc) is selected in the panel.

I don't see the usefulness of this. Wouldn't you know when someone is selected
in your own panel? Was this added to handle a specific use case?

Or maybe I'm misreading this and it means when an element is selected in a
native different Panel, like the Elements or Storage panel.


> webinspector.panels.onShown

There is an onShown but not an onHidden? If an inspector extension wants to do
something really wild, they may need to detect when a panel change is made.


> webinspector.consoleMessages

I think webInspector.console seems more appropriate. It would change the events
to "addMessage" and "onMessageAdded", but it opens the door to future console
related tasks.


> webinspector.consoleMessages.add(ConsoleMessage message)

Directly using the WebInspector.ConsoleMessage type is nice, but it means that
changing the internal ConsoleMessage type in the future could break extensions.
Should there be an intermediate representation for conversion?

This concern goes for all places where internal objects would be used directly.

[Joseph Pecoraro]

Comment on attachment 62568 [details]
patch (EOLs fixed)

> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,107 @@
> [phone]  Andrey Kosyakov  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Web Inspector: basic support of WebInspector extensions API.
> +        https://bugs.webkit.org/show_bug.cgi?id=40425
> +        Test: inspector/extensions.html
> +
>          ...

This is pretty bare. I think it would go a long way to add at least
a comment that this is experimental. Also, I always recommend giving
a brief description of new files in the ChangeLog.


> +            'inspector/front-end/ExtensionAPI.js',
> +            'inspector/front-end/ExtensionPanel.js',
> +            'inspector/front-end/ExtensionRegistryStub.js',
> +            'inspector/front-end/ExtensionServer.js',

New inspector files are typically added to WebCore/inspector/front-end/WebKit.qrc.
There is a helper script to generate it, but I actually have never used it:
WebKitTools/Scripts/generate-qt-inspector-resource



> Index: WebCore/inspector/InspectorController.cpp
> ===================================================================
> +    if (m_scriptsToEvaluateOnLoad.size()) {
> +        ScriptState* scriptState = mainWorldScriptState(frame);
> +        for (Vector<String>::iterator it = m_scriptsToEvaluateOnLoad.begin();
> +             it != m_scriptsToEvaluateOnLoad.end(); ++it) {
> +            m_injectedScriptHost->injectScript(*it, scriptState);
> +        }
> +    }
> +
>      if (!enabled() || !m_frontend || frame != m_inspectedPage->mainFrame())
>          return;

Seems weird to inject scripts before checking if the Web Inspector is
enabled() or not. I don't think this is correct.


> Index: WebCore/inspector/front-end/ExtensionAPI.js
> ===================================================================
> +function bind(func, thisObject)
> +{
> +    var args = Array.prototype.slice.call(arguments, 2);
> +    return function() { return func.apply(thisObject, args.concat(Array.prototype.slice.call(arguments, 0))); };
> +}

Can you use the utilities.js's Function.prototype.bind?


> Index: WebCore/inspector/front-end/ExtensionServer.js
> ===================================================================

> +    initExtensions: function(extensions)
> +    {
> +        InspectorFrontendHost.setExtensionAPI("(" + extensionAPI + ")");
> +        InspectorExtensionRegistry.getExtensions();
> +    },

This could use a comment because extensionAPI is defined in a different file,
and its actually a function. I think it would be nice to see toString() on it.
Also, the parameter, "extensions" is not used.


> +    _addExtension: function(extension)
> +    {
>          ...
> +        } catch (e) {
> +            console.log("Failed to initialize extension " + extension.startPage + ":" + e);
> +        }
> +    },

Typically we make these console.error(). They show up in red in the inspector's
inspector which makes them more noticeable.


> Index: WebCore/inspector/front-end/InjectedScript.js
> ===================================================================

> +        if (value != null && typeof value !== "string")

We strive for === as much as possible, but this might be acceptable.
"value != null" is only true when value is null or undefined. But
it still takes a bit of memorization.


This code looked pretty solid too me. Huge patch though, so I skimmed
quite a bit.

[Andrey Kosyakov]

(In reply to comment #13)
> > webinspector.panels.onSelectionChanged
> > Fired when an objects (script, resource, DOM element etc) is selected in the panel.
> 
> I don't see the usefulness of this. Wouldn't you know when someone is selected
> in your own panel? Was this added to handle a specific use case?
> 
> Or maybe I'm misreading this and it means when an element is selected in a
> native different Panel, like the Elements or Storage panel.

Exactly -- this is meant to let the extension know when inspector object is selected in inspector's panel, not extension panel. The primary use case for that is to update contents of sidebar panes or any other UI that depends on a currently selected item.

> > webinspector.panels.onShown
> 
> There is an onShown but not an onHidden? If an inspector extension wants to do
> something really wild, they may need to detect when a panel change is made.
> 

Yes, I'll add that. The intent of onShown was primarily to let the panel do some expensive stuff lazily only when it is shown, but I think having onHidden is good for consistency.

> > webinspector.consoleMessages
> 
> I think webInspector.console seems more appropriate. It would change the events
> to "addMessage" and "onMessageAdded", but it opens the door to future console
> related tasks.

This also makes sense. Anyway, console functionality only exists in the API draft at the moment, we need some more time to think over it.


> > webinspector.consoleMessages.add(ConsoleMessage message)
> 
> Directly using the WebInspector.ConsoleMessage type is nice, but it means that
> changing the internal ConsoleMessage type in the future could break extensions.
> Should there be an intermediate representation for conversion?

Sure, there definitely going to be an intermediate representation.

> This concern goes for all places where internal objects would be used directly.

We already do that for the only object type we support (which is resource).

[Andrey Kosyakov]

(In reply to comment #14)
> (From update of attachment 62568 [details])
> > +++ WebCore/ChangeLog	(working copy)
> > @@ -1,3 +1,107 @@
> > [phone]  Andrey Kosyakov  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Web Inspector: basic support of WebInspector extensions API.
> > +        https://bugs.webkit.org/show_bug.cgi?id=40425
> > +        Test: inspector/extensions.html
> > +
> >          ...
> 
> This is pretty bare. I think it would go a long way to add at least
> a comment that this is experimental. Also, I always recommend giving
> a brief description of new files in the ChangeLog.

Yup, will fix.

> > +            'inspector/front-end/ExtensionAPI.js',
> > +            'inspector/front-end/ExtensionPanel.js',
> > +            'inspector/front-end/ExtensionRegistryStub.js',
> > +            'inspector/front-end/ExtensionServer.js',
> 
> New inspector files are typically added to WebCore/inspector/front-end/WebKit.qrc.
> There is a helper script to generate it, but I actually have never used it:
> WebKitTools/Scripts/generate-qt-inspector-resource

Thanks for spotting this, I used to know that but keep forgetting :)

> > Index: WebCore/inspector/InspectorController.cpp
> > ===================================================================
> > +    if (m_scriptsToEvaluateOnLoad.size()) {
> > +        ScriptState* scriptState = mainWorldScriptState(frame);
> > +        for (Vector<String>::iterator it = m_scriptsToEvaluateOnLoad.begin();
> > +             it != m_scriptsToEvaluateOnLoad.end(); ++it) {
> > +            m_injectedScriptHost->injectScript(*it, scriptState);
> > +        }
> > +    }
> > +
> >      if (!enabled() || !m_frontend || frame != m_inspectedPage->mainFrame())
> >          return;
> 
> Seems weird to inject scripts before checking if the Web Inspector is
> enabled() or not. I don't think this is correct.

The only way to add scripts to m_scriptsToEvaluateOnLoad is via inspector, so it's going to be empty unless it's enabled. Adding an extra check won't hurt, though.


> > Index: WebCore/inspector/front-end/ExtensionAPI.js
> > ===================================================================
> > +function bind(func, thisObject)
> > +{
> > +    var args = Array.prototype.slice.call(arguments, 2);
> > +    return function() { return func.apply(thisObject, args.concat(Array.prototype.slice.call(arguments, 0))); };
> > +}
> 
> Can you use the utilities.js's Function.prototype.bind?

No -- this is an injected script, so utilities.js is not there, and Function.prototype is visible in client context, so we don't want to touch it.

> > +    initExtensions: function(extensions)
> 
> This could use a comment because extensionAPI is defined in a different file,
> and its actually a function. I think it would be nice to see toString() on it.

It looks like it's a common pattern now, given that we also do this for InjectedScript and InjectedFakeWorkers. I don't mind adding this, though. Just thought it could be renamed to injectedExtensionAPI or even injectedExtensionAPIConstructor for consistency with other two cases.

> Also, the parameter, "extensions" is not used.
> > +    _addExtension: function(extension)
> > +    {
> >          ...
> > +        } catch (e) {
> > +            console.log("Failed to initialize extension " + extension.startPage + ":" + e);
> > +        }
> > +    },
> 
> Typically we make these console.error(). They show up in red in the inspector's
> inspector which makes them more noticeable.

Thanks, fixed both of the above!

> > Index: WebCore/inspector/front-end/InjectedScript.js
> > ===================================================================
> 
> > +        if (value != null && typeof value !== "string")
> 
> We strive for === as much as possible, but this might be acceptable.
> "value != null" is only true when value is null or undefined. But
> it still takes a bit of memorization.

value != null was there on purpose, as I actually wanted this to cover both null and undefined. This will go away, though, as we decided just to do a JSON.stringify() on the resulting object unconditionally.

[Andrey Kosyakov]

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

[Andrey Kosyakov]

Created attachment 62843 [details]
patch

- Removed magic strings when sending notifications on server side and when creating statuses
- Do not attach panel upon creation
- Invoke callbacks right after client iframes are created, not in their onload (to support detached panels; may need ExtensionPanel.onLoad and ExtensionSidebarPane.onLoad)
- Hidden internal fields in API classes in closures
- Added dumping API fields in layout tests
- JSON.stringify everything resulting from eval in inspected page
- Removed webInspector.panels.get()

[Pavel Feldman]

Comment on attachment 62843 [details]
patch

LayoutTests/inspector/extensions-expected.txt:50
 +              createSidebarPane : <function>
resources panel does not have sidebar. r- for this.

LayoutTests/inspector/extensions-expected.txt:56
 +              createSidebarPane : <function>
ditto.

LayoutTests/inspector/extensions-expected.txt:59
 +              createSidebarPane : <function>
ditto.

LayoutTests/inspector/extensions-expected.txt:62
 +              createSidebarPane : <function>
ditto.

LayoutTests/inspector/extensions-expected.txt:65
 +              createSidebarPane : <function>
ditto.

LayoutTests/inspector/extensions-expected.txt:68
 +              createSidebarPane : <function>
ditto.

For LayoutTests/inspector/extensions-expected.txt:30
 +          onLoaded : {
Lets mimic these with reflective addEventListener("load") as Timothy suggested. r- for this.

LayoutTests/inspector/extensions-expected.txt:73
 +          onCreated : {
onFinished?

LayoutTests/inspector/extensions-expected.txt:91
 +  >>> test_resourceNotification
>>> will be used by the conflict resolve tool as well. Change to something else?

LayoutTests/inspector/extensions-expected.txt:5
 +  >>> test_createPanel
Each test should be in its own file. Or at least separate panel tests from APIs from inspectedWindow's and such. This could be done in a follow-up.

[Andrey Kosyakov]

(In reply to comment #19)

> For LayoutTests/inspector/extensions-expected.txt:30
>  +          onLoaded : {
> Lets mimic these with reflective addEventListener("load") as Timothy suggested. r- for this.

This is possible, but comes at a cost: presently, "chrome-style" event handles have arbitrary signatures -- i.e. they may have a number of parameters. With DOM-style handlers, these parameters would have to be converted to named properties of a DOM event. So for every event type we'd have to maintain a wrapper that converts the arguments from array to object with named property (or vice-versa, depending on how we send them). This will lead to some code bloat, obviously.

[Andrey Kosyakov]

(In reply to comment #19)
> (From update of attachment 62843 [details])
> LayoutTests/inspector/extensions-expected.txt:50
>  +              createSidebarPane : <function>
> resources panel does not have sidebar. r- for this.
> 
> LayoutTests/inspector/extensions-expected.txt:56
>  +              createSidebarPane : <function>
> ditto.
> 
> LayoutTests/inspector/extensions-expected.txt:59
>  +              createSidebarPane : <function>
> ditto.
> 
> LayoutTests/inspector/extensions-expected.txt:62
>  +              createSidebarPane : <function>
> ditto.
> 
> LayoutTests/inspector/extensions-expected.txt:65
>  +              createSidebarPane : <function>
> ditto.
> 
> LayoutTests/inspector/extensions-expected.txt:68
>  +              createSidebarPane : <function>
> ditto.
> 

I'm dropping support for panels other than scripts and elements now, will add it back later when we have some other methods (e.g. revealAndSelect) supported.

> For LayoutTests/inspector/extensions-expected.txt:30
>  +          onLoaded : {
> Lets mimic these with reflective addEventListener("load") as Timothy suggested. r- for this.

Will do later, filed https://bugs.webkit.org/show_bug.cgi?id=43251 for that.

> 
> LayoutTests/inspector/extensions-expected.txt:73
>  +          onCreated : {
> onFinished?

Done.

> 
> LayoutTests/inspector/extensions-expected.txt:91
>  +  >>> test_resourceNotification
> >>> will be used by the conflict resolve tool as well. Change to something else?
>

Done.
 
> LayoutTests/inspector/extensions-expected.txt:5
>  +  >>> test_createPanel
> Each test should be in its own file. Or at least separate panel tests from APIs from inspectedWindow's and such. This could be done in a follow-up.

I've changed the extension tests to make splitting the tests easier and separated the API dump. The other tests are still together, but Im going to split them when more tests are introduced.

[Andrey Kosyakov]

Created attachment 63061 [details]
patch

[Pavel Feldman]

Comment on attachment 63061 [details]
patch

Please land by hand.

[Joseph Pecoraro]

Comment on attachment 63061 [details]
patch

> Index: WebCore/inspector/InspectorController.cpp
> ===================================================================
> -    if (!enabled() || !m_frontend || frame != m_inspectedPage->mainFrame())
> +    if (!enabled())
> +        return;
> +
> +    if (m_scriptsToEvaluateOnLoad.size()) {
> +        ScriptState* scriptState = mainWorldScriptState(frame);
> +        for (Vector<String>::iterator it = m_scriptsToEvaluateOnLoad.begin();
> +             it != m_scriptsToEvaluateOnLoad.end(); ++it) {
> +            m_injectedScriptHost->injectScript(*it, scriptState);
> +        }
> +    }
> +
> +    if (!m_frontend || frame != m_inspectedPage->mainFrame())
>          return;
>      m_injectedScriptHost->discardInjectedScripts();
>  }

Minor point, which shouldn't prevent landing.
This still doesn't look right to me. It looks like:

  1. if its disabled leave.
  2. inject scripts.
  3. if there is no frontend, leave.
  4. discard injected scripts map.

It could be we just don't need the injected scripts any more,
but if that is the case shouldn't we discard in the early return
as well?

[Andrey Kosyakov]

(In reply to comment #24)
> (From update of attachment 63061 [details])
> > Index: WebCore/inspector/InspectorController.cpp
> > ===================================================================
> > -    if (!enabled() || !m_frontend || frame != m_inspectedPage->mainFrame())
> > +    if (!enabled())
> > +        return;
> > +
> > +    if (m_scriptsToEvaluateOnLoad.size()) {
> > +        ScriptState* scriptState = mainWorldScriptState(frame);
> > +        for (Vector<String>::iterator it = m_scriptsToEvaluateOnLoad.begin();
> > +             it != m_scriptsToEvaluateOnLoad.end(); ++it) {
> > +            m_injectedScriptHost->injectScript(*it, scriptState);
> > +        }
> > +    }
> > +
> > +    if (!m_frontend || frame != m_inspectedPage->mainFrame())
> >          return;
> >      m_injectedScriptHost->discardInjectedScripts();
> >  }
> 
> Minor point, which shouldn't prevent landing.
> This still doesn't look right to me. It looks like:
> 
>   1. if its disabled leave.
>   2. inject scripts.
>   3. if there is no frontend, leave.
>   4. discard injected scripts map.
> 
> It could be we just don't need the injected scripts any more,
> but if that is the case shouldn't we discard in the early return
> as well?

The discardInjectedScripts() logic does not change with this patch. We used to skip it if ether (a) inspector is not enabled, (b) frontend is not attached, or (c) the event comes for a subframe. I'm just inserting my code between (a) and (b). Actually, I think (a) is redundant both for injection of scriptsToEvaluateOnLoad and discardInjectedScripts(), as both are supposed to be a NOP in this case -- so we _could_ discard in early return as well (or remove the check altogether), but my idea was to leave the discardInjectedScripts() logic intact -- just in case.

[Andrey Kosyakov]

> The discardInjectedScripts() logic does not change with this patch. We used to skip it if ether (a) inspector is not enabled, (b) frontend is not attached, or (c) the event comes for a subframe. I'm just inserting my code between (a) and (b). Actually, I think (a) is redundant both for injection of scriptsToEvaluateOnLoad and discardInjectedScripts(), as both are supposed to be a NOP in this case -- so we _could_ discard in early return as well (or remove the check altogether), but my idea was to leave the discardInjectedScripts() logic intact -- just in case.

Ugh, sorry, I realized that you refer to second return as early return and imply that we may be clearing the ids of injected scripts we just created under certain circumstances. Well, this should not cause problems as the ids of this sort of injected scripts are never used, but I agree it would be better to do this consistently. I think we'll change this to

if (enabled()) {
    if (m_frontend && frame == m_inspectedPage->mainFrame())
        m_injectedScriptHost->discardInjectedScripts();

    // inject m_scriptsToEvaluateOnLoad() here.
}

[Andrey Kosyakov]

Created attachment 63199 [details]
patch to land

- Fixed API script injection
- Fixed tests

[WebKit Review Bot]

http://trac.webkit.org/changeset/64458 might have broken Qt Linux Release

[Timothy Hatcher]

Just to make sure:

Was this landed as experimental behing a flag? Before it leaves experimental status we need to have API review with the broader WebKit community.

[Pavel Feldman]

(In reply to comment #29)
> Just to make sure:
> 
> Was this landed as experimental behing a flag? Before it leaves experimental status we need to have API review with the broader WebKit community.

This change has been landed, but extensions can't yet be instantiated: registry that would do that is not ready. We'd like to silently (with people on Web Inspector list involved) iterate over the feature until we feel it is ready for broader WebKit community. There are no plans on making extensions available / announcing / blog posing on then any time soon.

Wrt API review by WebKit community, whom would you like to involve? I'd like to see Web Inspector, Extensions and FireBug extensions people discussing it. I'd really prefer to keep this feature small and unimportant so that we could iterate over it rapidly. Otherwise, if we feel review process is slowing us down, we might have to drop it...

[Ilya Tikhonovsky]

Committed r64458
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.gypi
	M	WebCore/inspector/InspectorFrontendHost.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorFrontendHost.h
	A	WebCore/inspector/front-end/ExtensionAPI.js
	M	WebCore/inspector/front-end/InspectorFrontendHostStub.js
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/InjectedScript.js
	A	WebCore/inspector/front-end/ExtensionPanel.js
	M	WebCore/inspector/front-end/ElementsPanel.js
	A	WebCore/inspector/front-end/ExtensionServer.js
	A	WebCore/inspector/front-end/ExtensionRegistryStub.js
	M	WebCore/inspector/front-end/InjectedScriptAccess.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/InspectorFrontendHost.idl
	M	LayoutTests/http/tests/inspector/inspector-test.js
	M	LayoutTests/platform/chromium/test_expectations.txt
	A	LayoutTests/inspector/extensions-test.js
	A	LayoutTests/inspector/extensions-api-expected.txt
	A	LayoutTests/inspector/extensions-api.html
	A	LayoutTests/inspector/extensions-expected.txt
	A	LayoutTests/inspector/extensions.html
	A	LayoutTests/inspector/resources/extension-main.html
	A	LayoutTests/inspector/resources/extension-sidebar.html
	A	LayoutTests/inspector/resources/extension-main.js
	A	LayoutTests/inspector/resources/extension-panel.html
	M	LayoutTests/ChangeLog
r64458 = 6a07f07ffe5acc63ca14e52486c2bd1162f8bea6 (refs/remotes/trunk)

[Ilya Tikhonovsky]

Web Inspector: Extensions API tests fail on Qt & GTK
https://bugs.webkit.org/show_bug.cgi?id=43344

[Patrick Mueller]

(In reply to comment #30)
> Wrt API review by WebKit community, whom would you like to involve? I'd like to see Web Inspector, Extensions and FireBug extensions people discussing it. I'd really prefer to keep this feature small and unimportant so that we could iterate over it rapidly. Otherwise, if we feel review process is slowing us down, we might have to drop it...

I chatted with John Barton from FireBug, and he would be interested in discussing.  Perhaps we could arrange a call, or IRC, or notes?

One thing John mentioned, was that there might be a desire to have Web Inspector support running FireBug extensions, but I don't think that's quite the shape of where this is going, but I haven't looked that deeply.  That certainly sounds like an interesting goal!

It would be good to state what the goal of having FireBug actually involved is; is there a suggestion that FireBug might support the Web Inspector Extension API, for instance?

[Timothy Hatcher]

I doubt we will ever be able to run Firebug extensions because XUL is (almost always) used.

[Timothy Hatcher]

Iterating rapidly is fine, but before flipping the switch to be something we need to support and keep from breaking, we need to have a good review of the API. Keeping it initially to ourselves is fine for now, but we need to solicit feedback from developers and other WebKit developers before making it all official. Otherwise there might be resentment about the design, after the point where it can't change.

[Christoph Dorn]

(In reply to comment #34)
> I doubt we will ever be able to run Firebug extensions because XUL is (almost always) used.

That is not the case. Firefox extensions do rely on XUL a lot, but Firebug extensions primarily leverage the Firebug API and domplate template system which is pure JS/HTML/CSS.

I believe the goal for Firebug is to move towards dynamically loadable pure JS/HTML/CSS extensions possibly conforming to CommonJS and/or Jetpacks.

This kind of setup makes Firebug extensions an ideal candidate to run in other browsers as well.

[Pavel Feldman]

I don't think we should aim API compatibility with FB. We have different UI, a bit different lifetime of things and such. In either case, extension authors would be able to split their code into the core, ui and client (host interaction). Core will be reused. It is much more important that we provide sufficient API for features that add value. When I was saying I'd involve "Firebug extensions" people in the discussion, I meant extensions' authors, not the framework owners. But having framework owners on board is surely great.

[Steven Roussey]

It is difficult to discuss an API without using it. How does one build Chromium or WebKit with this enabled?

[Andrey Kosyakov]

(In reply to comment #38)
> It is difficult to discuss an API without using it. How does one build Chromium or WebKit with this enabled?

This is already part of the build in the default configuration. For Chromium, however, you need to pass --enable-experimental-extension-apis flag when starting the browser. The layout tests (inspector/extensions-*.html) may be used as a reference for the current state of the API.

[Steven Roussey]

Thanks. I already run with --enable-experimental-extension-apis though, and I have an extension with "experimental" added to the permissions of the manifest.json file, yet webInspector is still not defined.

[Andrey Kosyakov]

(In reply to comment #40)
> Thanks. I already run with --enable-experimental-extension-apis though, and I have an extension with "experimental" added to the permissions of the manifest.json file, yet webInspector is still not defined.

This isn't exposed to extension pages other than designated Web Inspector / DevTools extensions. You need to have "devtools_page": "devtools.html" in your manifest.json, which will cause devtools.html to be loaded as a hidden iframe into DevTools page and will get the webInspector.* API exposed. Sorry we don't have any public documentation yet (still working on it).

[Steven Roussey]

I only need help getting to "hello world". After that, I understand I am on my own until docs are made and that won't happen until the API is stable, and that is a ways away...

So I created a chrome extension and run chrome with the specified command line switch. I used the files below, but nothing happens. No iframe gets added (I checked with the inspector on the inspector). Using Chrom dev channel.

I could just get to step one...


manifest.json:

{
  "name": "test",
  "version": "1.0.0",
  "description": "test...",
  "permissions": ["experimental", "tabs", "http://*/*", "https://*/*"],
  "devtools_page": "illumination_devtools.html"
}


devtools.html:

<!DOCTYPE html>
<html>
  <head>
  </head>
  <body>
    <script>
    webInspector.panels.create("Test Panel", "extension-panel.html", "extension-panel.png", function(){});
    webInspector.panels.scripts.createSidebarPane("Test Sidebar", "extension-sidebar.html", function(){});
    console.log("hello");
    </script>
  </body>
</html>

[Steven Roussey]

Scratch that, I'm an idiot and misspelled my how html file (usually chrome complains if a file in the manifest does not exist, so *that* ought to be an issue perhaps).