Bug 49727 – Web Inspector: [Chromium] Expose extension API to select a node in WebInspector

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

[Andrey Kosyakov]

A WebInspector extension may need to interact with the Elements panel -- e.g. change or request currently selected node. Currently, we only provide an ability to call inspect() from the script being evaluated in the inspected page context. Using this interface is dangerous, as the code an extension evaluates is not isolated from page context. A more natural way for an extension to access inspector's DOM API would be via a content script, since it both provides DOM access and isolates extension JS context from the inspected page.
The proposed patch exposes chrome.experimental.devTools.inspect() to content script API.

[Andrey Kosyakov]

Created attachment 74228 [details]
patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/WebKit.gyp', u'WebKit/chromium/public/WebDevToolsExtension.h', u'WebKit/chromium/src/WebDevToolsExtensionImpl.cpp']" exit_code: 1
WebKit/chromium/src/WebDevToolsExtensionImpl.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebDevToolsExtensionImpl.cpp:40:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/WebDevToolsExtensionImpl.cpp:52:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/public/WebDevToolsExtension.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 4 files


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

[Andrey Kosyakov]

Created attachment 74231 [details]
patch

Fixed style.

[Pavel Feldman]

Comment on attachment 74231 [details]
patch

Adding a static void WebScriptController::registerDevToolsExtensions() instead would reduce the API surface. There is no need to expose extension from WebKit in order to pass it into WebKit within the next statement.

[Andrey Kosyakov]

(In reply to comment #4)
> (From update of attachment 74231 [details])
> Adding a static void WebScriptController::registerDevToolsExtensions() instead would reduce the API surface. There is no need to expose extension from WebKit in order to pass it into WebKit within the next statement.

The idea is that the actual decision on whether the binding should be exposed is made on the renderer side (see RenderThread::AllowScriptExtension). For WebDevToolsExtension, we could put the decision logic to WebKit's FrameLoaderClientImpl, but given this binding should only be exposed to content script (extensionGroup == 1) and we don't seem to know the content script group id on the webkit side, I'd suggest we expose the extension via WebKit and register it the way other extensions are registered in RenderThread::EnsureWebKitInitialized().

[Darin Fisher (:fishd, Google)]

Comment on attachment 74231 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74231&action=review

> WebKit/chromium/src/WebDevToolsExtensionImpl.cpp:95
> +    "chrome = chrome || {};"

WebKit should not be modifying "window.chrome"

That belongs to Chromium, and we setup that in chrome/renderer.
Perhaps you should move this extension there?

Also, WebKit/chromium/src is not the place to be defining "web platform APIs."
That should be done in WebCore.  The only exception is window.chrome, which is
for UA specific things, and therefore does not belong in the WebKit repository.

[Andrey Kosyakov]

Created attachment 74863 [details]
patch

Moved chrome.experimental.devTools.inspect(node) to chrome, exposed WebDevToolsAgent::inspectNode() from WebKit.

[Darin Fisher (:fishd, Google)]

Comment on attachment 74863 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74863&action=review

> WebKit/chromium/public/WebDevToolsAgent.h:38
> +class Context;

nit: looks like the forward declaration of Context is not needed

[Andrey Kosyakov]

Manually committed r72810: http://trac.webkit.org/changeset/72810