[webkit-reviews] review denied: [Bug 36944] Chromium InspectorFrontendHost does not correctly implement canAttachWindow : [Attachment 52314] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Mattias Nissler
<mnissler at chromium.org>'s request for review:
Bug 36944: Chromium InspectorFrontendHost does not correctly implement
canAttachWindow
https://bugs.webkit.org/show_bug.cgi?id=36944

Attachment 52314: patch
https://bugs.webkit.org/attachment.cgi?id=52314&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> 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 at 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...