Bug 78575 – Web Inspector: Disable dock button when not allowed to dock

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

[Joseph Pecoraro]

Created attachment 126909 [details]
[IMAGE] Inspector cannot dock to Inspector

Surprisingly this was already available in InspectorFrontendHostStub.js, but just not used
anywhere in the frontend. This will be useful to hide the attach button when the frontend
host says that the window cannot be attached.

See attached screenshot. Not allowing an inspector window to attach to another inspector
window was recently added, and that is the example in the photo.

[Timothy Hatcher]

I assume it will also hide when the inspected window is too small? What is the inspected window resizes to have room, will the button show back up?

[Pavel Feldman]

We can't do synchronous canAttach, so we have asynchronous requestAttach instead. We should add InspectorFrontendAPI::setDockAvailable signal that would hide corresponding action instead.

[Joseph Pecoraro]

(In reply to comment #2)
> We can't do synchronous canAttach, so we have asynchronous requestAttach instead.
> We should add InspectorFrontendAPI::setDockAvailable signal that would hide
> corresponding action instead.

That sounds good. What domain should this be in? Inspector? Page? I can look at
this tomorrow!

[Pavel Feldman]

InspectorFrontendAPI is called from within the WebKit layer, i.e. it bypasses the protocol and talks directly Embedder -> Frontend View -> Frontend Page. Check out usages of other methods there (the Develop menu plumbing)

[Joseph Pecoraro]

 (In reply to comment #2)
> We can't do synchronous canAttach, so we have asynchronous requestAttach instead.
> We should add InspectorFrontendAPI::setDockAvailable signal that would hide
> corresponding action instead.

Does the asynchronous nature matter? If the WebView being inspected and the inspector
WebView are in two difference processes, if you check the size of the inspected WebView's
main frame it could change by the time that you've gotten the value.


(In reply to comment #4)
> InspectorFrontendAPI is called from within the WebKit layer, i.e. it bypasses the
> protocol and talks directly Embedder -> Frontend View -> Frontend Page. Check
> out usages of other methods there (the Develop menu plumbing)

canAttachWindow and attachWindow check or adjust the inspected page's main frame's
FrameView. As far as I can tell, that FrameView's size can change with layout.
The InspectorFrontendAPI seems to be very port specific (InspectorFrontendClient).
If I add API to this, would I have to update each port in some way in didLayout?

My original plan was to have the trigger be at the end FrameView::performPostLayoutTasks.
There is already "resize" code there, and it is shared by all ports. If I trigger the frontend
API from here it looks pretty ugly:

  - to get to the InspectorFrontendClient I could go through InspectorController + InspectorClient,
    but going through InspectorController like this is frowned on.

Your asynchronous comment was probably about the unreliability of fetching that state?
If so, would it be fine to instead have a setDockingAvailable(bool) message in the Inspector
domain? As long as the events arrive in the order they are sent the this looks okay to me.

[Joseph Pecoraro]

Created attachment 130237 [details]
[PATCH] Go through InspectorFrontendAPI - setDockingAvailability

• cq- to make sure the bots build this on other platforms.
• I will update this bug title (and the ChangeLog as well) to more accurately reflect what we're doing now.

[WebKit Review Bot]

Attachment 130237 [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/WebCore/inspector/InspectorInstrumentation.cpp:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 21 files


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

[Joseph Pecoraro]

> Source/WebCore/inspector/InspectorInstrumentation.cpp:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Fixed locally.

[Timothy Hatcher]

Comment on attachment 130237 [details]
[PATCH] Go through InspectorFrontendAPI - setDockingAvailability

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

> Source/WebCore/page/FrameView.cpp:2336
> +            InspectorInstrumentation::didResizeMainFrame(m_frame.get());

This might not be the main frame. Do you need a check here?

if (Page* page = m_frame->page()) {
    if (page->mainFrame() == m_frame) { … }
}

[Joseph Pecoraro]

Comment on attachment 130237 [details]
[PATCH] Go through InspectorFrontendAPI - setDockingAvailability

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

>> Source/WebCore/page/FrameView.cpp:2336
>> +            InspectorInstrumentation::didResizeMainFrame(m_frame.get());
> 
> This might not be the main frame. Do you need a check here?
> 
> if (Page* page = m_frame->page()) {
>     if (page->mainFrame() == m_frame) { … }
> }

Ahh, you're correct. I actually had that and lost it when I rebased this from last week. Good catch.

[WebKit Review Bot]

Comment on attachment 130237 [details]
[PATCH] Go through InspectorFrontendAPI - setDockingAvailability

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

[Joseph Pecoraro]

> cause the following virtual functions are pure within 'WebKit::InspectorFrontendClientImpl':
> Source/WebCore/inspector/InspectorFrontendClient.h:69: note: 	virtual bool WebCore::InspectorFrontendClient::canAttachWindow()
> Source/WebCore/inspector/InspectorFrontendClient.h:70: note: 	virtual void WebCore::InspectorFrontendClient::setDockingAvailable(bool)

I can provide empty implementations for these (canAttachWindow returning true).

[Joseph Pecoraro]

Created attachment 130252 [details]
[PATCH] For Bots

[Joseph Pecoraro]

This recent patch flipped the state to be setDockingUnavailable.
That way if the value was undefined for Chromium this should still
work properly. Letting the bots take a look at this one.

[Joseph Pecoraro]

Landed in r109858:
<http://trac.webkit.org/changeset/109858>

[Pavel Feldman]

Comment on attachment 130252 [details]
[PATCH] For Bots

Guys, this is all very wrong. Could you roll it out while I am commenting on the change?

[Joseph Pecoraro]

(In reply to comment #16)
> (From update of attachment 130252 [details])
> Guys, this is all very wrong. Could you roll it out while I am commenting on the change?

Sure. I'll revert this.

[Pavel Feldman]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 130252 [details] [details])
> > Guys, this is all very wrong. Could you roll it out while I am commenting on the change?
> 
> Sure. I'll revert this.

Wait, I think it can be "fixed" with less effort. I see that you did use InspectorFrontendAPI, while that was my major concern.

[Joseph Pecoraro]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 130252 [details] [details] [details])
> > > Guys, this is all very wrong. Could you roll it out while I am commenting on the change?
> > 
> > Sure. I'll revert this.
> 
> Wait, I think it can be "fixed" with less effort. I see that you did use InspectorFrontendAPI, while that was my major concern.

Arg, I managed to roll out already.:
<http://trac.webkit.org/changeset/109860>

I thought you were going to request some sweeping changes.

I suspect you are going to want this to be entirely handled up in WebKit, when the Window changes size?

[Pavel Feldman]

Comment on attachment 130252 [details]
[PATCH] For Bots

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

> Source/WebCore/inspector/InspectorClient.h:49
> +    virtual void updateDockingAvailability() { }

InspectorClient should not be aware of docking, it is not inspector's business. I'd much rather have ChromeClient::didResiseMainFrame that would convert into what you need in WebKit layer. But I can see that this method allows you getting the information straight to where you need it, so it is probably Ok. I would rename it to didResiseMainFrame though.

> Source/WebCore/inspector/InspectorController.cpp:368
> +    m_inspectorClient->updateDockingAvailability();

The reason I don't like it is that InspectorController represents inspector backend, it should not be chrome-aware. It should not know what dock / undock means.

> Source/WebCore/inspector/InspectorFrontendClient.h:69
> +    virtual bool canAttachWindow() { return true; }

This is what confused me initially: you ask whether you can undock and command that docking is unavailable into the same instance. You call it differently (attach and dock), but this is essentially the same thing. You probably don't want canAttachWindow in this interface.

> Source/WebCore/inspector/InspectorFrontendClientLocal.h:79
> +    virtual bool canAttachWindow();

Why did it get virtual? Probably should not.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:271
> +void InspectorInstrumentation::didResizeMainFrameImpl(Frame* frame)

Inspector instrumentation is only used for the instrumentation (things that get into agents / reported to the front-end).

> Source/WebCore/page/FrameView.cpp:2338
> +                    InspectorInstrumentation::didResizeMainFrame(m_frame.get());

As mentioned above, you should do page->inspectorController()->didResizeMainFrame. In fact, I would even expose InspectorClient on InspectorController and call didResizeMainFrame on the client directly. InspectorClient::didResizeMainFrame.

[Pavel Feldman]

Comment on attachment 130252 [details]
[PATCH] For Bots

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

> Source/WebCore/inspector/front-end/inspector.js:581
> +    this._dockToggleButton.visible = this.attached ? true : !this._isDockingUnavailable;

Changing the visibility of the button might be a bad idea - we sometimes rely upon the number of buttons (to position view's statusbar when resizing the sidebar). could you disable it instead?

[Pavel Feldman]

> Changing the visibility of the button might be a bad idea - we sometimes rely upon the number of buttons (to position view's statusbar when resizing the sidebar). could you disable it instead?

Sorry for crying out loud. It is just that canAttach and setDockingUnavailable in the same interface made me go wild.

[Pavel Feldman]

(In reply to comment #22)
> > Changing the visibility of the button might be a bad idea - we sometimes rely upon the number of buttons (to position view's statusbar when resizing the sidebar). could you disable it instead?
> 
> Sorry for crying out loud. It is just that canAttach and setDockingUnavailable in the same interface made me go wild.

To clarify: canAttachWindow in the interface meant that we still do synchronous calls with return valus (from the front-end into the embedder). Which was my major concern expressed in the comment#2. It turned out that Joe's latest solution did not need really it since he was already using direct calls on the InspectorFrontendAPI as I was suggesting. I already raised the flag by the time I reached the InspectorFrontendAPI part in my review.

[Joseph Pecoraro]

Created attachment 130308 [details]
[PATCH] Revised Patch

This should address all of Pavel's comments!

[Joseph Pecoraro]

I will also remove canAttachWindow from InspectorFrontendHostStub.js,
since it is stale and not correct anyways.

[Pavel Feldman]

Comment on attachment 130308 [details]
[PATCH] Revised Patch

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

One small comment, otherwise looks good. Thanks for following up.

> Source/WebCore/inspector/InspectorFrontendClient.h:69
> +    virtual void setDockingUnavailable(bool) { }

I think all the ports that are going to call this methods inherit from InspectorFrontendClientLocal, so you actually don't need it here. It also does not have to be virtual, it is just a helper function.

[Joseph Pecoraro]

(In reply to comment #26)
> (From update of attachment 130308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130308&action=review
> 
> One small comment, otherwise looks good. Thanks for following up.
> 
> > Source/WebCore/inspector/InspectorFrontendClient.h:69
> > +    virtual void setDockingUnavailable(bool) { }
> 
> I think all the ports that are going to call this methods inherit from InspectorFrontendClientLocal,
> so you actually don't need it here. It also does not have to be virtual, it is just a helper function.

Done. Thanks for the extra comments. The patch feels better.

[Joseph Pecoraro]

Landed in r109939: <http://trac.webkit.org/changeset/109939>

[Joseph Pecoraro]

This broke the Qt minimal bot:
> ../../../../Source/WebCore/page/FrameView.cpp: In member function 'void WebCore::FrameView::performPostLayoutTasks()':
> ../../../../Source/WebCore/page/FrameView.cpp:2342: error: 'class WebCore::Page' has no member named 'inspectorController'

Looks like this needs to be wrapped in ENABLE(INSPECTOR) or something similar. I'll take a look.

[Joseph Pecoraro]

Build Fix for minimal build landed in r109954:
<http://trac.webkit.org/changeset/109954>