WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(2.03 KB, patch)
2010-04-01 01:32 PDT
,
Mattias Nissler
no flags
Details
Formatted Diff
Diff
patch
(2.14 KB, patch)
2010-04-01 02:33 PDT
,
Mattias Nissler
no flags
Details
Formatted Diff
Diff
patch
(9.33 KB, patch)
2010-04-01 11:13 PDT
,
Mattias Nissler
no flags
Details
Formatted Diff
Diff
patch
(10.18 KB, patch)
2010-04-06 05:22 PDT
,
Mattias Nissler
pfeldman
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Mattias Nissler
Comment 1
2010-04-01 01:20:40 PDT
Created
attachment 52275
[details]
patch
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
Created
attachment 52277
[details]
Patch
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
Created
attachment 52278
[details]
patch
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
Created
attachment 52314
[details]
patch
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
Created
attachment 52622
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug