RESOLVED FIXED 36944
Web inspector: Docking should not rely on synchronous canAttachWindow() call
https://bugs.webkit.org/show_bug.cgi?id=36944
Summary Web inspector: Docking should not rely on synchronous canAttachWindow() call
Mattias Nissler
Reported 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.
Attachments
patch (1.22 KB, patch)
2010-04-01 01:20 PDT, Mattias Nissler
no flags
Patch (2.03 KB, patch)
2010-04-01 01:32 PDT, Mattias Nissler
no flags
patch (2.14 KB, patch)
2010-04-01 02:33 PDT, Mattias Nissler
no flags
patch (9.33 KB, patch)
2010-04-01 11:13 PDT, Mattias Nissler
no flags
patch (10.18 KB, patch)
2010-04-06 05:22 PDT, Mattias Nissler
pfeldman: review+
Mattias Nissler
Comment 1 2010-04-01 01:20:40 PDT
WebKit Review Bot
Comment 2 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.
Mattias Nissler
Comment 3 2010-04-01 01:32:32 PDT
Pavel Feldman
Comment 4 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.
Mattias Nissler
Comment 5 2010-04-01 02:33:35 PDT
Pavel Feldman
Comment 6 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.
Mattias Nissler
Comment 7 2010-04-01 11:13:44 PDT
Pavel Feldman
Comment 8 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...
Mattias Nissler
Comment 9 2010-04-06 04:47:30 PDT
Updating summary to reflect discussion in previous comments.
Mattias Nissler
Comment 10 2010-04-06 05:22:55 PDT
Pavel Feldman
Comment 11 2010-04-06 05:48:15 PDT
Comment on attachment 52622 [details] patch This looks good. Please coordinate landing with me.
Pavel Feldman
Comment 12 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
Note You need to log in before you can comment on or make changes to this bug.