Bug 78575 - Web Inspector: Disable dock button when not allowed to dock
Summary: Web Inspector: Disable dock button when not allowed to dock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-13 22:38 PST by Joseph Pecoraro
Modified: 2012-03-06 13:12 PST (History)
12 users (show)

See Also:


Attachments
[IMAGE] Inspector cannot dock to Inspector (356.79 KB, image/png)
2012-02-13 22:38 PST, Joseph Pecoraro
no flags Details
[PATCH] Go through InspectorFrontendAPI - setDockingAvailability (17.90 KB, patch)
2012-03-05 17:01 PST, Joseph Pecoraro
timothy: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Bots (17.95 KB, patch)
2012-03-05 17:56 PST, Joseph Pecoraro
pfeldman: review-
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Revised Patch (15.68 KB, patch)
2012-03-05 23:56 PST, Joseph Pecoraro
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2012-02-13 22:38:49 PST
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.
Comment 1 Timothy Hatcher 2012-02-13 22:41:11 PST
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?
Comment 2 Pavel Feldman 2012-02-13 23:55:14 PST
We can't do synchronous canAttach, so we have asynchronous requestAttach instead. We should add InspectorFrontendAPI::setDockAvailable signal that would hide corresponding action instead.
Comment 3 Joseph Pecoraro 2012-02-14 00:06:33 PST
(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!
Comment 4 Pavel Feldman 2012-02-14 00:11:12 PST
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)
Comment 5 Joseph Pecoraro 2012-02-14 16:11:35 PST
 (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.
Comment 6 Joseph Pecoraro 2012-03-05 17:01:17 PST
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.
Comment 7 WebKit Review Bot 2012-03-05 17:04:25 PST
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.
Comment 8 Joseph Pecoraro 2012-03-05 17:09:45 PST
> 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.
Comment 9 Timothy Hatcher 2012-03-05 17:11:38 PST
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) { … }
}
Comment 10 Joseph Pecoraro 2012-03-05 17:16:37 PST
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.
Comment 11 WebKit Review Bot 2012-03-05 17:28:34 PST
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
Comment 12 Joseph Pecoraro 2012-03-05 17:31:06 PST
> 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).
Comment 13 Joseph Pecoraro 2012-03-05 17:56:39 PST
Created attachment 130252 [details]
[PATCH] For Bots
Comment 14 Joseph Pecoraro 2012-03-05 17:58:15 PST
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.
Comment 15 Joseph Pecoraro 2012-03-05 22:03:48 PST
Landed in r109858:
<http://trac.webkit.org/changeset/109858>
Comment 16 Pavel Feldman 2012-03-05 22:07:56 PST
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?
Comment 17 Joseph Pecoraro 2012-03-05 22:09:19 PST
(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.
Comment 18 Pavel Feldman 2012-03-05 22:16:05 PST
(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.
Comment 19 Joseph Pecoraro 2012-03-05 22:18:40 PST
(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?
Comment 20 Pavel Feldman 2012-03-05 22:25:39 PST
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.
Comment 21 Pavel Feldman 2012-03-05 22:33:27 PST
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?
Comment 22 Pavel Feldman 2012-03-05 22:38:34 PST
> 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.
Comment 23 Pavel Feldman 2012-03-05 22:43:27 PST
(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.
Comment 24 Joseph Pecoraro 2012-03-05 23:56:01 PST
Created attachment 130308 [details]
[PATCH] Revised Patch

This should address all of Pavel's comments!
Comment 25 Joseph Pecoraro 2012-03-05 23:58:35 PST
I will also remove canAttachWindow from InspectorFrontendHostStub.js,
since it is stale and not correct anyways.
Comment 26 Pavel Feldman 2012-03-06 02:04:37 PST
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.
Comment 27 Joseph Pecoraro 2012-03-06 11:33:31 PST
(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.
Comment 28 Joseph Pecoraro 2012-03-06 11:33:58 PST
Landed in r109939: <http://trac.webkit.org/changeset/109939>
Comment 29 Joseph Pecoraro 2012-03-06 12:58:52 PST
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.
Comment 30 Joseph Pecoraro 2012-03-06 13:12:30 PST
Build Fix for minimal build landed in r109954:
<http://trac.webkit.org/changeset/109954>