Bug 195973 - Web Inspector: Debugger: lazily create the agent
Summary: Web Inspector: Debugger: lazily create the agent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-19 15:57 PDT by Devin Rousso
Modified: 2019-03-19 22:40 PDT (History)
11 users (show)

See Also:


Attachments
Patch (20.75 KB, patch)
2019-03-19 16:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.56 KB, patch)
2019-03-19 18:14 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.91 MB, application/zip)
2019-03-19 22:18 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.57 MB, application/zip)
2019-03-19 22:30 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-03-19 15:57:02 PDT
.
Comment 1 Radar WebKit Bug Importer 2019-03-19 15:57:18 PDT
<rdar://problem/49039674>
Comment 2 Devin Rousso 2019-03-19 16:53:46 PDT
Created attachment 365264 [details]
Patch
Comment 3 Joseph Pecoraro 2019-03-19 17:32:35 PDT
Comment on attachment 365264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365264&action=review

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:245
> +        if (m_debuggerAgent) {

This should always be true. I'd drop the if check. The only path to get here (that we expect) is through InspectorAgent::initialized meaning the agents are alive and a frontend sent a message.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:107
> +    for (auto* listener : copyToVector(m_listeners))
> +        listener->debuggerWasDisabled();

Why upgrade this to a list. Do you expect to add more listeners?
Comment 4 Devin Rousso 2019-03-19 18:13:40 PDT
Comment on attachment 365264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365264&action=review

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:107
>> +        listener->debuggerWasDisabled();
> 
> Why upgrade this to a list. Do you expect to add more listeners?

No "good" reason other than that it seemed "wrong" to only allow for one listener.  This way, it also matches `JavaScriptCore::ScriptDebugServer`.
Comment 5 Devin Rousso 2019-03-19 18:14:06 PDT
Created attachment 365278 [details]
Patch
Comment 6 EWS Watchlist 2019-03-19 22:18:50 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2019-03-19 22:18:52 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-03-19 22:30:13 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2019-03-19 22:30:15 PDT Comment hidden (obsolete)
Comment 10 WebKit Commit Bot 2019-03-19 22:40:17 PDT
Comment on attachment 365278 [details]
Patch

Clearing flags on attachment: 365278

Committed r243192: <https://trac.webkit.org/changeset/243192>
Comment 11 WebKit Commit Bot 2019-03-19 22:40:19 PDT
All reviewed patches have been landed.  Closing bug.