Bug 36944 – Web inspector: Docking should not rely on synchronous canAttachWindow() call

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

[Mattias Nissler]

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.

[Mattias Nissler]

Created attachment 52275 [details]
patch

[WebKit Review Bot]

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.

[Mattias Nissler]

Created attachment 52277 [details]
Patch

[Pavel Feldman]

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 @@
> [phone]  Mattias Nissler  <[e-mail]>
> +
> +        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.

[Mattias Nissler]

Created attachment 52278 [details]
patch

[Pavel Feldman]

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.

[Mattias Nissler]

Created attachment 52314 [details]
patch

[Pavel Feldman]

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 @@
> [phone]  Mattias Nissler  <[e-mail]>
> +
> +        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...

[Mattias Nissler]

Updating summary to reflect discussion in previous comments.

[Mattias Nissler]

Created attachment 52622 [details]
patch

[Pavel Feldman]

Comment on attachment 52622 [details]
patch

This looks good. Please coordinate landing with me.

[Pavel Feldman]

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