Bug 43924 – Web Inspector: -[WebInspector attach] and detach should work

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 64257 [details]
[PATCH] Implement attach and detach

[Joseph Pecoraro]

Does this need a test? I tested locally in gdb.

[Pavel Feldman]

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.

[Mark Rowe (bdash)]

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?!

[Mark Rowe (bdash)]

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.

[Pavel Feldman]

(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.

[Mark Rowe (bdash)]

(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.

[Pavel Feldman]

> 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.

[Joseph Pecoraro]

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.

[Pavel Feldman]

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+!

[Joseph Pecoraro]

> 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.

[Pavel Feldman]

> > 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.

[Joseph Pecoraro]

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

[Yury Semikhatsky]

(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.