| Summary: | Web Inspector: provide API for content scripts to interact with the inspector | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, bweinstein, christoph, eric, joepeck, keishi, loislo, masterov, pfeldman, pmuellr, rik, sroussey, timothy, webkit.review.bot, yurys | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Andrey Kosyakov
2010-06-10 09:03:23 PDT
Created attachment 58376 [details]
Extension API draft
This is very preliminary, just to get discussion started.
Created attachment 58377 [details]
A demo of extension in action
This one is pretty useless, but gives an idea of what is currently supported.
Created attachment 62567 [details]
patch
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.
Created attachment 62568 [details]
patch (EOLs fixed)
Created attachment 62578 [details]
updated API draft
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?
(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! 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).
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. 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. (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. > 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. Comment on attachment 62568 [details] patch (EOLs fixed) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,107 @@ > +2010-07-26 Andrey Kosyakov <caseq@chromium.org> > + > + 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. (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). (In reply to comment #14) > (From update of attachment 62568 [details]) > > +++ WebCore/ChangeLog (working copy) > > @@ -1,3 +1,107 @@ > > +2010-07-26 Andrey Kosyakov <caseq@chromium.org> > > + > > + 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. (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. 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()
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. (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. (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. Created attachment 63061 [details]
patch
Comment on attachment 63061 [details]
patch
Please land by hand.
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? (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. > 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.
}
Created attachment 63199 [details]
patch to land
- Fixed API script injection
- Fixed tests
http://trac.webkit.org/changeset/64458 might have broken Qt Linux Release 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. (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... 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) Web Inspector: Extensions API tests fail on Qt & GTK https://bugs.webkit.org/show_bug.cgi?id=43344 (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? I doubt we will ever be able to run Firebug extensions because XUL is (almost always) used. 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. (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. 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. It is difficult to discuss an API without using it. How does one build Chromium or WebKit with this enabled? (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. 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. (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). 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>
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). |