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.
Created attachment 52275 [details] patch
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.
Created attachment 52277 [details] Patch
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.
Created attachment 52278 [details] patch
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.
Created attachment 52314 [details] patch
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...
Updating summary to reflect discussion in previous comments.
Created attachment 52622 [details] patch
Comment on attachment 52622 [details] patch This looks good. Please coordinate landing with me.
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