| Summary: | Web Inspector: -[WebInspector attach] and detach should work | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bweinstein, joepeck, keishi, pfeldman, pmuellr, rik, timothy, yurys | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
Created attachment 64257 [details]
[PATCH] Implement attach and detach
Does this need a test? I tested locally in gdb. 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.
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?! 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. (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. (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. > 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.
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 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+!
> 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. > > 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. 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 (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. |
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.