Bug 62985 – Web Inspector: introduce InspectorFrontendAPI for actions initiated from the application menu.

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

[Pavel Feldman]

Both: inspector protocol and WebCore/InspectorController have a number of unnecessary methods for plumbing the menu action handlers through the WebKit and WebCore. Like "Start Debugging JavaScript" starts in the WebKit's inspector code, dives into the WebCore backend, goes to the WebCore front-end and enables debugging from there.

Considering remote debugging scenario, it is clear that inspector's actions under the "Develop" menu in Safari are the ones contributed by the front-end, not the inspector backend. What they should do is "open the frontend -> do an action". Such as "open the frontend -> enable debugger" in the Start Debugging JavaScript case. Hence, I intend to remove this menu support from the protocol and WebCore/InspectorController API. I am starting with exposing the new front-end API in the WebCore and using it in the WebKit/mac case. Then i'll port WebKit/win and WebKit2.

[Pavel Feldman]

Created attachment 97795 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97795 [details]
Patch

Attachment 97795 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8910519

New failing tests:
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/resource-tree/resource-tree-document-url.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/network/network-xhr-sync.html
http/tests/inspector/network/network-iframe-load-and-delete.html
http/tests/inspector/network/download.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector/network/ping.html
http/tests/inspector/network/network-open-load-reopen.html
http/tests/inspector/network/network-xhr-async.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/network/network-close-load-open.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/network/x-frame-options-deny.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html
http/tests/inspector/network/network-clear-after-disabled.html

[WebKit Review Bot]

Created attachment 97797 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yury Semikhatsky]

Comment on attachment 97795 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:221
> +    ScriptValue value = m_frontendPage->mainFrame()->script()->executeScript(expression);

Some ifs are missing.

> Source/WebKit/mac/WebInspector/WebInspector.mm:78
>          page->inspectorController()->showConsole();

This should go through inspector frontend API.

[Pavel Feldman]

Created attachment 97806 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97806 [details]
Patch

Attachment 97806 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8914535

New failing tests:
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/resource-tree/resource-tree-document-url.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/network/network-xhr-sync.html
http/tests/inspector/network/network-iframe-load-and-delete.html
http/tests/inspector/network/download.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector/network/ping.html
http/tests/inspector/network/network-open-load-reopen.html
http/tests/inspector/network/network-xhr-async.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/network/network-close-load-open.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/network/x-frame-options-deny.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html
http/tests/inspector/network/network-clear-after-disabled.html

[WebKit Review Bot]

Created attachment 97807 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Created attachment 97810 [details]
Patch to land

[Pavel Feldman]

Created attachment 97826 [details]
Patch

[Pavel Feldman]

Created attachment 97846 [details]
Patch

[Pavel Feldman]

Created attachment 97935 [details]
Patch

[Pavel Feldman]

Created attachment 97964 [details]
Patch working both on Mac and Windows.

[Yury Semikhatsky]

Comment on attachment 97964 [details]
Patch working both on Mac and Windows.

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

> Source/WebCore/ChangeLog:36
> +   warning: LF will be replaced by CRLF in Source/WebCore/WebCore.vcproj/WebCore.vcproj.

Something wrong with this changelog entry, please fix.

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:175
> +    m_frontendClient = new WebInspectorFrontendClient(m_inspectedWebView, m_inspectedWebViewHwnd, frontendHwnd, frontendWebView, frontendWebViewHwnd, this, createFrontendSettings());

Plseas use adoptPtr(new WebInspectorFrontendClient(...)) and .leakPtr() below.

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.h:71
> +    void releaseFrontendClient();

Can we use one releaseFrontendXXX method instead?

> Source/WebKit/win/WebView.cpp:2642
> +    m_inspectorClient = new WebInspectorClient(this);

There should be adoptPtr().

[Pavel Feldman]

Created attachment 97980 [details]
[PATCH] Patch for ews: bots and yurys.

[Pavel Feldman]

Created attachment 97983 [details]
[PATCH] Patch for ews: bots only.

[Pavel Feldman]

Committed r89354: <http://trac.webkit.org/changeset/89354>

[Ryosuke Niwa]

This broke a whole bunch of tests on Snow Leopard and others:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r89354%20(30586)/results.html

[Ryosuke Niwa]

It also broke Leopard:
http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/33362

I'm getting less patient about all these inspector changesets breaking tests and left on the tree.  If you're landing a patch, please respect what's written on "Keeping the tree green" section on http://www.webkit.org/coding/contributing.html

[Ryosuke Niwa]

I'm sorry I rolled this patch out because it was making harder to track other regressions.

[Pavel Feldman]

(In reply to comment #18)
> It also broke Leopard:
> http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Tests%29/builds/33362
> 
> I'm getting less patient about all these inspector changesets breaking tests and left on the tree.  If you're landing a patch, please respect what's written on "Keeping the tree green" section on http://www.webkit.org/coding/contributing.html

Sorry, my bad. It was a fairly large change. I checked inspector/ tests after landing and was not expecting profiler/ to fail.

@rniwa: "all these inspector changesets breaking tests and left on the tree" sounds a bit harsh though. Are you saying that inspector contributors are less careful with the tree?

[Eric Seidel (no email)]

Comment on attachment 97980 [details]
[PATCH] Patch for ews: bots and yurys.

Cleared Yury Semikhatsky's review+ from obsolete attachment 97980 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Pavel Feldman]

Created attachment 116062 [details]
Patch

[Pavel Feldman]

Committed r100903: <http://trac.webkit.org/changeset/100903>

[noel gordon]

Did this change brake the windows build?  From http://queues.webkit.org/results/10534245

4>..\WebInspector.cpp(216) : error C2039: 'isJavaScriptProfilingEnabled' : is not a member of 'WebInspectorFrontendClient'
4>        c:\cygwin\home\buildbot\WebKit\Source\WebKit\win\WebCoreSupport\WebInspectorClient.h(95) : see declaration of 'WebInspectorFrontendClient'

[noel gordon]

Pavel is on it.

[Ryosuke Niwa]

(In reply to comment #20)
> @rniwa: "all these inspector changesets breaking tests and left on the tree" sounds a bit harsh though. Are you saying that inspector contributors are less careful with the tree?

Oh I didn't meant like that. Apologize if I offended you. It's just that there were a bunch of inspector-related test failures / build failures around that time.