Summary: | Web Inspector: [Chromium] Expose extension API to select a node in WebInspector | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, fishd, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2010-11-18 05:22:49 PST
Created attachment 74228 [details]
patch
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.
Created attachment 74231 [details]
patch
Fixed style.
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.
(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(). 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. Created attachment 74863 [details]
patch
Moved chrome.experimental.devTools.inspect(node) to chrome, exposed WebDevToolsAgent::inspectNode() from WebKit.
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 Manually committed r72810: http://trac.webkit.org/changeset/72810 |