Bug 36944 - Web inspector: Docking should not rely on synchronous canAttachWindow() call
Summary: Web inspector: Docking should not rely on synchronous canAttachWindow() call
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-01 01:13 PDT by Mattias Nissler
Modified: 2010-04-06 06:45 PDT (History)
2 users (show)

See Also:


Attachments
patch (1.22 KB, patch)
2010-04-01 01:20 PDT, Mattias Nissler
no flags Details | Formatted Diff | Diff
Patch (2.03 KB, patch)
2010-04-01 01:32 PDT, Mattias Nissler
no flags Details | Formatted Diff | Diff
patch (2.14 KB, patch)
2010-04-01 02:33 PDT, Mattias Nissler
no flags Details | Formatted Diff | Diff
patch (9.33 KB, patch)
2010-04-01 11:13 PDT, Mattias Nissler
no flags Details | Formatted Diff | Diff
patch (10.18 KB, patch)
2010-04-06 05:22 PDT, Mattias Nissler
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mattias Nissler 2010-04-01 01:13:39 PDT
The InspectorFrontendHost implementation for chromium (in WebKit/chromium/src/WebDevToolsFrontendImpl.cpp) implements canAttachWindow() returning always true. This poses a problem when trying to dock a devtools window that cannot be docked. This is the case i.e. for the devtools windows that correspond to extension views in chromium. The inspector switches to attached mode (different toolbar layout), but still has its own window.
Comment 1 Mattias Nissler 2010-04-01 01:20:40 PDT
Created attachment 52275 [details]
patch
Comment 2 WebKit Review Bot 2010-04-01 01:22:49 PDT
Attachment 52275 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebDevToolsFrontendClient.h:51:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mattias Nissler 2010-04-01 01:32:32 PDT
Created attachment 52277 [details]
Patch
Comment 4 Pavel Feldman 2010-04-01 02:25:27 PDT
Comment on attachment 52277 [details]
Patch

Overall looks good. Could of nits that need to be fixed prior to landing.

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> index 551c957..aefd631 100644
> --- a/WebKit/chromium/ChangeLog
> +++ b/WebKit/chromium/ChangeLog
> @@ -1,3 +1,16 @@
> +2010-04-01  Mattias Nissler  <mnissler@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement canAttachWindow() for the WebDevToolsFrontendImpl implementation of
> +        InspectorFrontendHost and forward the call to WebDevToolsFrontendClient. This
> +        allows us to implement canAttachWindow() properly in the chromium code.
> +

ChangeLog entry should reference bug url here.

> +    WebDevToolsFrontendImpl* frontend = static_cast<WebDevToolsFrontendImpl*>(v8::External::Cast(*args.Data())->Value());
> +    frontend->m_client->sendDebuggerPauseScript();

No need to pause debugger for this one.
Comment 5 Mattias Nissler 2010-04-01 02:33:35 PDT
Created attachment 52278 [details]
patch
Comment 6 Pavel Feldman 2010-04-01 02:43:44 PDT
Comment on attachment 52278 [details]
patch

This looks good, although please look at my comments for the Chromium part. It might be that we should go the other way in WebKit and remove canAttachWindow as a whole. Clearing r? flag for now.
Comment 7 Mattias Nissler 2010-04-01 11:13:44 PDT
Created attachment 52314 [details]
patch
Comment 8 Pavel Feldman 2010-04-01 12:39:20 PDT
Comment on attachment 52314 [details]
patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 23f2bf3..9888bc8 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,24 @@
> +2010-04-01  Mattias Nissler  <mnissler@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +

It'd be nice if this started with "Web Inspector:". Note that using webkit.org/new-inspector-bug has a nice template for this.

> +    virtual void requestAttachWindow() = 0;
>      virtual void attachWindow() = 0;

InspectorFrontendClient should only have requestAttachWindow. attachWindow should be declared in InspectorFrontendClientLocal.h insead.
> +++ b/WebCore/inspector/InspectorFrontendHost.h
> @@ -64,7 +64,7 @@ public:
>      void bringToFront();
>      void inspectedURLChanged(const String&);
>  
> -    bool canAttachWindow() const;
> +    void requestAttachWindow() const;

This guy should lose attach() as well, right? Probably attach and detach should now be requestAttach and requestDetach for symmetry.

>  WebInspector.toggleAttach = function()
>  {
> -    if (!this.attached && !InspectorFrontendHost.canAttachWindow())
> -        return;
> +    if (!this.attached) {
> +      InspectorFrontendHost.requestAttachWindow();
> +      return;
> +    }
>  
>      this.attached = !this.attached;

Actual request for attachment is done in "attached" setter (line ~239). You probably want to remove the line above from here and introduce a case for requestDetach. "attached" setter is going to be called as a part of callback and should only take care of the front-end javascript state (update toolbar style and titles). Otherwise you are calling attach twice and re-entering it in the setter.

> +    virtual void requestDockWindow() {};
>      virtual void dockWindow() {};

Just use existing dockWindow. Otherwise - what is it for...
Comment 9 Mattias Nissler 2010-04-06 04:47:30 PDT
Updating summary to reflect discussion in previous comments.
Comment 10 Mattias Nissler 2010-04-06 05:22:55 PDT
Created attachment 52622 [details]
patch
Comment 11 Pavel Feldman 2010-04-06 05:48:15 PDT
Comment on attachment 52622 [details]
patch

This looks good. Please coordinate landing with me.
Comment 12 Pavel Feldman 2010-04-06 06:45:28 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.Inspector.exp
	M	WebCore/inspector/InspectorFrontendClient.h
	M	WebCore/inspector/InspectorFrontendClientLocal.cpp
	M	WebCore/inspector/InspectorFrontendClientLocal.h
	M	WebCore/inspector/InspectorFrontendHost.cpp
	M	WebCore/inspector/InspectorFrontendHost.h
	M	WebCore/inspector/InspectorFrontendHost.idl
	M	WebCore/inspector/front-end/inspector.js
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/public/WebDevToolsFrontendClient.h
	M	WebKit/chromium/src/InspectorFrontendClientImpl.cpp
	M	WebKit/chromium/src/InspectorFrontendClientImpl.h
Committed r57146