Bug 200854

Summary: Web Inspector: implement a consistent style and lifecycle for all agents
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: NEW ---    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, rniwa, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200109
https://bugs.webkit.org/show_bug.cgi?id=200114
https://bugs.webkit.org/show_bug.cgi?id=200113
https://bugs.webkit.org/show_bug.cgi?id=200120
https://bugs.webkit.org/show_bug.cgi?id=200110
https://bugs.webkit.org/show_bug.cgi?id=200119
https://bugs.webkit.org/show_bug.cgi?id=200116
https://bugs.webkit.org/show_bug.cgi?id=200115
https://bugs.webkit.org/show_bug.cgi?id=200112
Bug Depends on: 200947, 200950, 200959    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews113 for mac-highsierra
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews100 for mac-highsierra
none
Archive of layout-test-results from ews113 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Patch none

Description Devin Rousso 2019-08-16 23:59:16 PDT
After speaking at length offline with Joe about what `enable`/`disable` represent, we've come to the following conclusion:

An agent's `enable`/`disable` controls whether that agent is "allowed" to send events to the connected frontend.  For now, an agent isn't "required" to be `enable`d in order for commands being sent from the frontend to the backend to have any effect (some agents already work this way).  Agents that need other agents to be `enable`d shouldn't ever assume anything about the state of the other agent, and therefore should properly handle the case where that other agent isn't enabled (often this is just returning an error from the command or not sending an event).

As such, we should:
 - add `enable`/`disable` to all domains that have events
    - re-add them to the `Page` domain (partial "undo" of r248454)
    - this does NOT include the `DOM` domain, as that's going to be handled by <https://webkit.org/b/197025>
 - add error messages when `enable` is called if the domain is already enabled
 - add proper checks when agents are referenced by other agents

I think we should move towards a world where `enable`/`disable` is the single entry/exit point for ALL agents, but that can come later.
Comment 1 Devin Rousso 2019-08-17 00:11:09 PDT
Created attachment 376594 [details]
Patch

This _could_ be two or three separate patches, but I'd like to see how EWS feels about the entire thing all at once :)
Comment 2 Devin Rousso 2019-08-17 00:18:21 PDT
Created attachment 376595 [details]
Patch
Comment 3 EWS Watchlist 2019-08-17 01:22:33 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-08-17 01:22:35 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-08-17 01:24:02 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-08-17 01:24:03 PDT Comment hidden (obsolete)
Comment 7 Devin Rousso 2019-08-17 02:15:19 PDT
Created attachment 376603 [details]
Patch
Comment 8 Devin Rousso 2019-08-17 02:19:26 PDT
Created attachment 376604 [details]
Patch
Comment 9 EWS Watchlist 2019-08-17 03:30:31 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-08-17 03:30:33 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-08-17 03:41:38 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-08-17 03:41:40 PDT Comment hidden (obsolete)
Comment 13 Devin Rousso 2019-08-17 17:29:48 PDT
Created attachment 376617 [details]
Patch
Comment 14 EWS Watchlist 2019-08-17 18:21:07 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2019-08-17 18:21:09 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-08-17 18:35:31 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2019-08-17 18:35:33 PDT Comment hidden (obsolete)
Comment 18 Devin Rousso 2019-08-18 03:39:51 PDT
Created attachment 376632 [details]
Patch
Comment 19 EWS Watchlist 2019-08-18 05:28:34 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2019-08-18 05:28:36 PDT Comment hidden (obsolete)
Comment 21 Devin Rousso 2019-08-18 10:25:12 PDT
Created attachment 376642 [details]
Patch

Rather than have subclasses implement the command variant of `enable`/`disable`, create a separate protected member function that is called by the command variant so that the `errorString` doesn't need to be propagated
Comment 22 Radar WebKit Bug Importer 2019-08-20 09:21:27 PDT
<rdar://problem/54514408>
Comment 23 Timothy Hatcher 2020-02-14 14:23:36 PST
Comment on attachment 376642 [details]
Patch

Clearing review. I doubt this applies anymore.