Summary: | Web inspector: Docking should not rely on synchronous canAttachWindow() call | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mattias Nissler <mnissler> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Minor | CC: | pfeldman, webkit.review.bot | ||||||||||||
Priority: | P3 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Mattias Nissler
2010-04-01 01:13:39 PDT
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 |