Bug 43924 - Web Inspector: -[WebInspector attach] and detach should work
Summary: Web Inspector: -[WebInspector attach] and detach should work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-12 10:53 PDT by Joseph Pecoraro
Modified: 2010-08-15 23:06 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Implement attach and detach (4.21 KB, patch)
2010-08-12 14:00 PDT, Joseph Pecoraro
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Allow attach and detach entirely in WebKit (13.64 KB, patch)
2010-08-14 22:51 PDT, Joseph Pecoraro
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2010-08-12 10:53:31 PDT
Currently they do nothing. I just got a report:

> It seems like WebInspector detach: should do this, but
> it doesn't actually do anything!

Looks like their implementation is empty. They should call
down to the controller, who should call requestToAttach or
Detach on its InspectorFrontendClient. Currently the only
caller to that is from JavaScript.
Comment 1 Joseph Pecoraro 2010-08-12 14:00:46 PDT
Created attachment 64257 [details]
[PATCH] Implement attach and detach
Comment 2 Joseph Pecoraro 2010-08-12 14:01:51 PDT
Does this need a test? I tested locally in gdb.
Comment 3 Pavel Feldman 2010-08-12 14:09:42 PDT
Comment on attachment 64257 [details]
[PATCH] Implement attach and detach

We've been ripping attach/detach logic from the InspectorController/Client intentionally since it is a pure 'view' concept. Ideally, it should be implemented entirely in the WebKit layer - no need to dive into WebCore. You should look at what InspectorFrontendClient is calling in WebKit and wire WebKit API to those WebKit methods directly.
Comment 4 Mark Rowe (bdash) 2010-08-12 17:20:38 PDT
These methods were gutted in r56051 without explanation  <http://trac.webkit.org/changeset/56051/trunk/WebKit/mac/WebInspector/WebInspector.mm>

Why was that ok to begin with?!
Comment 5 Mark Rowe (bdash) 2010-08-12 17:22:46 PDT
For what it’s worth, an explanation of why it’s ok to turn methods like that in to no-ops is exactly the sort of thing that the ChangeLog should include.  Sadly, the ChangeLog entry from r56051 is effectively worthless.  It includes only a vague, high-level description of the change with no per-function commentary as is customary in WebKit ChangeLog entries.
Comment 6 Pavel Feldman 2010-08-12 22:19:58 PDT
(In reply to comment #4)
> These methods were gutted in r56051 without explanation  <http://trac.webkit.org/changeset/56051/trunk/WebKit/mac/WebInspector/WebInspector.mm>
> 
> Why was that ok to begin with?!

Yeah, oversight from my side, sorry. We were supposed to file a follow up bug and fix it. The change was getting too large, so we needed to land it.
Comment 7 Mark Rowe (bdash) 2010-08-12 22:28:20 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > These methods were gutted in r56051 without explanation  <http://trac.webkit.org/changeset/56051/trunk/WebKit/mac/WebInspector/WebInspector.mm>
> > 
> > Why was that ok to begin with?!
> 
> Yeah, oversight from my side, sorry. We were supposed to file a follow up bug and fix it. The change was getting too large, so we needed to land it.

It’s never OK to completely break API or SPI that a port exposes.  That’s a completely inappropriate manner in which to split up work.
Comment 8 Pavel Feldman 2010-08-12 22:40:11 PDT
> It’s never OK to completely break API or SPI that a port exposes.  That’s a completely inappropriate manner in which to split up work.

I agree, we are taking actions to resolve it asap. Sorry about that.
Comment 9 Joseph Pecoraro 2010-08-14 22:51:58 PDT
Created attachment 64436 [details]
[PATCH] Allow attach and detach entirely in WebKit

I'm not sure what the best name for the class would be... WebInspectorFrontend
is okay, but a little misleading. Maybe it should be WebInspectorLocalFrontend,
to indicate that it only makes sense to attach/detach a local frontend.
Comment 10 Pavel Feldman 2010-08-14 23:07:28 PDT
Comment on attachment 64436 [details]
[PATCH] Allow attach and detach entirely in WebKit

Thanks for doing this. Afaik Yury was going to fix it pretty much the same way.

Not sure whether you need this new public abstraction and the delegation. I'd simply place a reference to WebInspectorFrontendClient into WebInspectorPrivate.h. Rationale: WebInspector.h is the api to inspector and I am not sure we should distinguish between backend and front-end on the WebKit embedder level. Otherwise looks good! If you provide the rationale for the interface, I am not against putting r+!
Comment 11 Joseph Pecoraro 2010-08-14 23:22:22 PDT
> Thanks for doing this. Afaik Yury was going to fix it pretty much the same way.

Oh. I should have just assigned it to me when I decided I was going to
implement it. I had some spare time tonight.


> I am not sure we should distinguish between backend and front-end on
> the WebKit embedder level. 

I agree, but that I figured distinction already exists because of InspectorClient
and InspectorFrontendClient.


> Otherwise looks good! If you provide the rationale for the interface, I am not
> against putting r+!

My rationale for the extra class was that the extra class is an Obj-C
wrapper around the C++ class. I thought this was a common thing to do, or
at least I had seen the pattern so I felt I should emulate it. I'm fine with
either approach. WebKit clients should still only see and use the
"WebInspector" class. They don't have access to the WebInspectorFrontend.
Comment 12 Pavel Feldman 2010-08-15 00:04:39 PDT
> > I am not sure we should distinguish between backend and front-end on
> > the WebKit embedder level. 
> 
> I agree, but that I figured distinction already exists because of InspectorClient
> and InspectorFrontendClient.
> 

My point was that we should distinguish between those on the WebKit level, but not pass distinction further to the embedder.

> My rationale for the extra class was that the extra class is an Obj-C
> wrapper around the C++ class. I thought this was a common thing to do, or
> at least I had seen the pattern so I felt I should emulate it. I'm fine with
> either approach. WebKit clients should still only see and use the
> "WebInspector" class. They don't have access to the WebInspectorFrontend.

It was unclear from the code: header is there, it does not look too much different from WebInspector itself. As a result, embedder might get confused. Bug given that we maintain close relationship with the embedder, I guess it is fine.
Comment 13 Joseph Pecoraro 2010-08-15 13:16:06 PDT
Committed r65383
	M	WebKit/WebKit.xcodeproj/project.pbxproj
	M	WebKit/ChangeLog
	M	WebKit/mac/WebCoreSupport/WebInspectorClient.mm
	M	WebKit/mac/ChangeLog
	M	WebKit/mac/WebInspector/WebInspectorPrivate.h
	M	WebKit/mac/WebInspector/WebInspector.h
	A	WebKit/mac/WebInspector/WebInspectorFrontend.h
	M	WebKit/mac/WebInspector/WebInspector.mm
	A	WebKit/mac/WebInspector/WebInspectorFrontend.mm
r65383 = 250fdc600fdbc0febbc313f8415741fb7889429b
http://trac.webkit.org/changeset/65383
Comment 14 Yury Semikhatsky 2010-08-15 23:06:46 PDT
(In reply to comment #13)
> Committed r65383
>     M    WebKit/WebKit.xcodeproj/project.pbxproj
>     M    WebKit/ChangeLog
>     M    WebKit/mac/WebCoreSupport/WebInspectorClient.mm
>     M    WebKit/mac/ChangeLog
>     M    WebKit/mac/WebInspector/WebInspectorPrivate.h
>     M    WebKit/mac/WebInspector/WebInspector.h
>     A    WebKit/mac/WebInspector/WebInspectorFrontend.h
>     M    WebKit/mac/WebInspector/WebInspector.mm
>     A    WebKit/mac/WebInspector/WebInspectorFrontend.mm
> r65383 = 250fdc600fdbc0febbc313f8415741fb7889429b
> http://trac.webkit.org/changeset/65383

Thanks for fixing this! I was trying to do it on Friday but my knowledge of Obj-C is very limited so I didn't complete my change.