Bug 62985

Summary: Web Inspector: introduce InspectorFrontendAPI for actions initiated from the application menu.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: noel.gordon, rniwa, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 63099    
Bug Blocks: 73962    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch to land
none
Patch
none
Patch
none
Patch
none
Patch working both on Mac and Windows.
yurys: review-
[PATCH] Patch for ews: bots and yurys.
none
[PATCH] Patch for ews: bots only.
none
Patch none

Description Pavel Feldman 2011-06-20 06:45:39 PDT
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.
Comment 1 Pavel Feldman 2011-06-20 06:52:17 PDT
Created attachment 97795 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-20 07:01:49 PDT
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
Comment 3 WebKit Review Bot 2011-06-20 07:01:54 PDT
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
Comment 4 Yury Semikhatsky 2011-06-20 07:13:00 PDT
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.
Comment 5 Pavel Feldman 2011-06-20 08:48:27 PDT
Created attachment 97806 [details]
Patch
Comment 6 WebKit Review Bot 2011-06-20 08:58:17 PDT
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
Comment 7 WebKit Review Bot 2011-06-20 08:58:22 PDT
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
Comment 8 Pavel Feldman 2011-06-20 09:31:03 PDT
Created attachment 97810 [details]
Patch to land
Comment 9 Pavel Feldman 2011-06-20 11:15:26 PDT
Created attachment 97826 [details]
Patch
Comment 10 Pavel Feldman 2011-06-20 13:18:05 PDT
Created attachment 97846 [details]
Patch
Comment 11 Pavel Feldman 2011-06-20 23:40:20 PDT
Created attachment 97935 [details]
Patch
Comment 12 Pavel Feldman 2011-06-21 05:08:31 PDT
Created attachment 97964 [details]
Patch working both on Mac and Windows.
Comment 13 Yury Semikhatsky 2011-06-21 06:31:04 PDT
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().
Comment 14 Pavel Feldman 2011-06-21 06:48:38 PDT
Created attachment 97980 [details]
[PATCH] Patch for ews: bots and yurys.
Comment 15 Pavel Feldman 2011-06-21 07:08:24 PDT
Created attachment 97983 [details]
[PATCH] Patch for ews: bots only.
Comment 16 Pavel Feldman 2011-06-21 07:32:42 PDT
Committed r89354: <http://trac.webkit.org/changeset/89354>
Comment 17 Ryosuke Niwa 2011-06-21 14:38:38 PDT
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
Comment 18 Ryosuke Niwa 2011-06-21 14:43:45 PDT
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
Comment 19 Ryosuke Niwa 2011-06-21 15:32:31 PDT
I'm sorry I rolled this patch out because it was making harder to track other regressions.
Comment 20 Pavel Feldman 2011-08-05 06:10:18 PDT
(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?
Comment 21 Eric Seidel (no email) 2011-09-06 15:28:16 PDT
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.
Comment 22 Pavel Feldman 2011-11-21 04:01:11 PST
Created attachment 116062 [details]
Patch
Comment 23 Pavel Feldman 2011-11-21 04:55:32 PST
Committed r100903: <http://trac.webkit.org/changeset/100903>
Comment 24 noel gordon 2011-11-21 05:21:54 PST
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'
Comment 25 noel gordon 2011-11-21 05:25:28 PST
Pavel is on it.
Comment 26 Ryosuke Niwa 2011-11-21 09:53:03 PST
(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.