Bug 65493 – WebKit2: Web Inspector always starts in undocked mode

• Component: WebKit2
• Status: RESOLVED FIXED

[Brian Weinstein]

In WebKit2, the web inspector always starts in undocked mode. It then can be docked, but it shouldn't take two clicks to do this.

<rdar://problem/9353114>

[Brian Weinstein]

Created attachment 102575 [details]
[PATCH] Fix

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:211:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:212:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 17 files


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

[Brian Weinstein]

Tabs are fixed locally.

[WebKit Review Bot]

Comment on attachment 102575 [details]
[PATCH] Fix

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

[Andy Estes]

Comment on attachment 102575 [details]
[PATCH] Fix

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

> Source/WebCore/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

Oops indeed! You already have this, so this line can be removed.

> Source/WebKit2/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

Ditto.

[Brian Weinstein]

(In reply to comment #5)
> (From update of attachment 102575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102575&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Need a short description and bug URL (OOPS!)
> 
> Oops indeed! You already have this, so this line can be removed.

Fixed. I meant to remove this, and keep the Reviewed by NOBODY line. OOPS!

> 
> > Source/WebKit2/ChangeLog:3
> > +        Need a short description and bug URL (OOPS!)
> 
> Ditto.

(In reply to comment #5)
> (From update of attachment 102575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102575&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Need a short description and bug URL (OOPS!)
> 
> Oops indeed! You already have this, so this line can be removed.
> 
> > Source/WebKit2/ChangeLog:3
> > +        Need a short description and bug URL (OOPS!)
> 
> Ditto.

Ditto for the fix.

[Jeff Miller]

Comment on attachment 102575 [details]
[PATCH] Fix

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

> Source/WebKit2/UIProcess/WebInspectorProxy.h:59
> +enum UpdateAttachedPreferenceOrNot { DoNotUpdateAttachedPreference, UpdateAttachedPreference };

Is this enum used anywhere?

> Source/WebKit2/UIProcess/WebInspectorProxy.h:119
> +    void platformOpen(bool willOpenAttached);

Would an enum be more clear here?

> Source/WebKit2/UIProcess/WebInspectorProxy.h:132
> +    void didLoadInspectorPage(bool canStartAttached);

Would an enum be more clear here?

[Brian Weinstein]

Comment on attachment 102575 [details]
[PATCH] Fix

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

>> Source/WebKit2/UIProcess/WebInspectorProxy.h:59
>> +enum UpdateAttachedPreferenceOrNot { DoNotUpdateAttachedPreference, UpdateAttachedPreference };
> 
> Is this enum used anywhere?

Nope. Removed. This was from a previous iteration of the code.

>> Source/WebKit2/UIProcess/WebInspectorProxy.h:119
>> +    void platformOpen(bool willOpenAttached);
> 
> Would an enum be more clear here?

I don't think it would be, because we never pass just true or false, we're always passing a value that makes it clear.

>> Source/WebKit2/UIProcess/WebInspectorProxy.h:132
>> +    void didLoadInspectorPage(bool canStartAttached);
> 
> Would an enum be more clear here?

This value is sent in a message from web process -> UI process, and is never passed as true/false, so I don't think an enum is necessary.

[Alexey Proskuryakov]

*** Bug 65107 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Comment on attachment 102575 [details]
[PATCH] Fix

Attachment 102575 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9287531

[Eric Caldwell]

When will this patch make it to a nightly.  As of r92131, the bug is still there.

Thx!

[Brian Weinstein]

(In reply to comment #11)
> When will this patch make it to a nightly.  As of r92131, the bug is still there.
> 
> Thx!

The patch needs to be reviewed and landed, and then it will be put in a nightly, when it is landed, I will note the revision number in this bug so you know what nightly to look for.

[Eric Caldwell]

@Brian,

Thanks!!  We're a web design and development company and this is driving us nuts in Safari which is our primary tool.

[Adam Roben (:aroben)]

Comment on attachment 102575 [details]
[PATCH] Fix

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

> Source/WebCore/inspector/InspectorController.cpp:518
> +void InspectorController::requestAttachWindow()
> +{
> +    if (!m_inspectorFrontendClient)
> +        return;
> +
> +    m_inspectorFrontendClient->requestAttachWindow();
> +}

Why isn't this function needed in WebKit1?

[Brian Weinstein]

(In reply to comment #14)
> (From update of attachment 102575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102575&action=review
> 
> > Source/WebCore/inspector/InspectorController.cpp:518
> > +void InspectorController::requestAttachWindow()
> > +{
> > +    if (!m_inspectorFrontendClient)
> > +        return;
> > +
> > +    m_inspectorFrontendClient->requestAttachWindow();
> > +}
> 
> Why isn't this function needed in WebKit1?

WebKit1 calls through to functions in the InspectorFrontendClient directly. In WebKit2 we can't do that (because the proxy is in a different process than the InspectorFrontendClient), and so all calls need to go through the InspectorController.

[Adam Roben (:aroben)]

Comment on attachment 102575 [details]
[PATCH] Fix

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

>>> Source/WebCore/inspector/InspectorController.cpp:518
>>> +}
>> 
>> Why isn't this function needed in WebKit1?
> 
> WebKit1 calls through to functions in the InspectorFrontendClient directly. In WebKit2 we can't do that (because the proxy is in a different process than the InspectorFrontendClient), and so all calls need to go through the InspectorController.

OK. I guess WebKit::WebInspector can't call through to the InspectorFrontendClient directly?

[Brian Weinstein]

(In reply to comment #16)
> (From update of attachment 102575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102575&action=review
> 
> >>> Source/WebCore/inspector/InspectorController.cpp:518
> >>> +}
> >> 
> >> Why isn't this function needed in WebKit1?
> > 
> > WebKit1 calls through to functions in the InspectorFrontendClient directly. In WebKit2 we can't do that (because the proxy is in a different process than the InspectorFrontendClient), and so all calls need to go through the InspectorController.
> 
> OK. I guess WebKit::WebInspector can't call through to the InspectorFrontendClient directly?

Right, it has to go through the InspectorController.

[Brian Weinstein]

Created attachment 102940 [details]
[PATCH] Fix + Chromium Build Fix

[Brian Weinstein]

Landed in r92384.

[Pavel Feldman]

This change is wrong, please use webkit.org/new-inspector-bug for filing inspector bugs or make sure active inspector contributors are included into the CC. I do realize how painful this issue is and am ready to help resolving it.

The reasons it is wrong are following:

1) Source/WebCore/inspector/InspectorFrontendClient.h can't have synchronous canAttachWindow() since in the multiprocess environment, you can't immediately get back to it from the host. That's why it has asynchronous requestAttachWindow instead (that can well fail).

2) You are introducing a strange loop here: embedder (WebKit) asks InspectorController (WebCore) on whether it can dock front-end window. InspectorController (WebCore) gets back into WebKit (via InspectorFrontendClient interface), so you are effectively asking yourself a question.

You were probably confused by the InspectorFrontendClientLocal which is a in-WebCore helper class used in most traditional in-process WebKit ports to shortcut this scenario. But WebKit2 should not do that.

(It aslo adds a FIXME into the Chromium code with not heads up to the code maintainers)

[Pavel Feldman]

It did not have me in the CC, it does not have WebInspector component. It flew entirely under my radar. Adam, could you please make sure you include people contributing to the code most while reviewing inspector patches?

[Brian Weinstein]

(In reply to comment #20)
> This change is wrong, please use webkit.org/new-inspector-bug for filing inspector bugs or make sure active inspector contributors are included into the CC. I do realize how painful this issue is and am ready to help resolving it.

I should have CC'd you guys, it was originally handled more at the WebKit2 level, but I had to do more in WebCore than I thought.

> 
> The reasons it is wrong are following:
> 
> 1) Source/WebCore/inspector/InspectorFrontendClient.h can't have synchronous canAttachWindow() since in the multiprocess environment, you can't immediately get back to it from the host. That's why it has asynchronous requestAttachWindow instead (that can well fail).
> 

The canAttachWindow logic is all done in our WebProcess. I'm not sure what the issue is here. I have a better question below, however.

> 2) You are introducing a strange loop here: embedder (WebKit) asks InspectorController (WebCore) on whether it can dock front-end window. InspectorController (WebCore) gets back into WebKit (via InspectorFrontendClient interface), so you are effectively asking yourself a question.
>

Is there a better way to go from WebInspectorClient -> WebInspectoFrontendClient? Tim seemed to think I had to go through the InspectorController, and he signed off on this approach as well.
 
> You were probably confused by the InspectorFrontendClientLocal which is a in-WebCore helper class used in most traditional in-process WebKit ports to shortcut this scenario. But WebKit2 should not do that.

What should WebKit2 do? Is the best solution to start the window undocked, and have it flash in the undocked mode until requestAttachWindow returns? This patch does better than that, and I don't want to have the undocked window flash. How does Chromium handle this?

> 
> (It aslo adds a FIXME into the Chromium code with not heads up to the code maintainers)

The FIXME was for something that shouldn't need to be implemented, I could have made it a non-pure virtual method, and then wouldn't have had to touch Chromium.

Sorry for not CC'ing you guys - let me know if you have an idea how to make this fix more correct.

[Pavel Feldman]

> The canAttachWindow logic is all done in our WebProcess. I'm not sure what the issue is here. I have a better question below, however.
> 

I still need to learn how WebKit2 works (was avoiding it before). But on the high level, we think of it as of three worlds that can't talk to each other. It helps us keeping the architecture sane and allows remote debugging. [We kinda relaxed on the way we've wired it in traditional WebKit and as a result, we are facing this confusion].

The worlds I am talking about are:

(1) Inspected Window that has InspectorController (which hosts internal WebCore agents)
(2) Front-end Window that does not really need to have InspectorController. It does, but we leave it behind the scenes
(3) Embedder that hosts both windows (browser)

We make interaction between theses 3 asynchronous (with the exception of InspectorFrontendClientLocal in WebKit that violates it all for the sake of simplicity of in-process implementation).

Here is the life of the "request attach":

A button is clicked in the front-end (world 2), it hits the InspectorFrontendHost in WebCore that delegates the question to the InspectorFrontendClient::requestAttach. We need to ask our window manager (browser from world (3)) whether dock can be performed. Browser can't answer any questions synchronously (different world), but it can try docking the window.

> Is there a better way to go from WebInspectorClient -> WebInspectoFrontendClient? Tim seemed to think I had to go through the InspectorController, and he signed off on this approach as well.

Again, InspectorFrontendClientLocal hack made you think that this transition should go through InspectorController, while it should not. You should not inherit WebInspectoFrontendClient from InspectorFrontendClientLocal (we should have named it InspectorFrontendClientForWebKit1). Once you remove this inheritance, it'll be way more clear on how you should proceed with wiring these on the WebKit level.

> > You were probably confused by the InspectorFrontendClientLocal which is a in-WebCore helper class used in most traditional in-process WebKit ports to shortcut this scenario. But WebKit2 should not do that.
> 
> What should WebKit2 do? Is the best solution to start the window undocked, and have it flash in the undocked mode until requestAttachWindow returns? This patch does better than that, and I don't want to have the undocked window flash. How does Chromium handle this?
> 

WebKit2 is responsible for windows layout (managing docked / undocked). When inspector is opened at first, WebKit2 receives a signal. It creates a view for the front-end. Depending on the setting stored on the WebKit2 level, it should check the windows dimensions and place front-end view accordingly (no flickering no nothing). There should be no roundtrips on the initial open.

The only place where Chromium is handling is differently (and I am not sure it is relevant to this very topic) is that we don't open front-end from within InspectorController::show. Again, InspectorController::show is in world (1), front-end is in world (2). For better understanding, you can imagine that in case of remote debugging, InspectorController::show can't do much to the outside remote client. So what we do is upon inspect element / inspector shortcut, we start the front-end and wire it to the InspectorController via calling connectFrontend(). At that point we establish explicit WebInspectorClient / WebInspectorFrontendClient binding in terms of WebKit. And our InspectorClient::openInspectorFrontend is empty.

> > 
> > (It aslo adds a FIXME into the Chromium code with not heads up to the code maintainers)
> 
> The FIXME was for something that shouldn't need to be implemented, I could have made it a non-pure virtual method, and then wouldn't have had to touch Chromium.
> 
> Sorry for not CC'ing you guys - let me know if you have an idea how to make this fix more correct.

Lets sort this one out. While looking at it I might get a better idea on how to fix the Develop menu implementation that suffers from the same crossing-worlds problems.

[Pavel Feldman]

> At that point we establish explicit WebInspectorClient / WebInspectorFrontendClient binding in terms of WebKit. And our InspectorClient::openInspectorFrontend is empty.

This is probably the most important piece: WebKit2 should maintain WebInspector / WebInspectorFrontend binding on its own. It should dispatch the messages between the two and make sure they reach the addressee.

[Pavel Feldman]

Filed a bug for this: https://bugs.webkit.org/show_bug.cgi?id=65756

[Adam Roben (:aroben)]

(In reply to comment #21)
> Adam, could you please make sure you include people contributing to the code most while reviewing inspector patches?

Yes, I'll do that in the future. Sorry for the trouble.

[Eric Caldwell]

The latest nightly is looking better!  The inspector now starts attached.  The only visual bug I can see right now is the inspector windows starts at 2/3 the height of the browser window.

[drew covi]

looks like we're closer to half in fullscreen, but it could definitely start at around 30-40 percent. Have there been any discussions on this? Thanks to everyone who tackled this bug!

(In reply to comment #27)
> The latest nightly is looking better!  The inspector now starts attached.  The only visual bug I can see right now is the inspector windows starts at 2/3 the height of the browser window.