Bug 148902 – Web Inspector: pass frontend token to agent's frontend attach/detach methods

• Status: NEW
• Component: Web Inspector

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/22592414>

[Blaze Burg]

Created attachment 261018 [details]
WIP

Dumping this diff on here, need to move to other stuff for now.

[Blaze Burg]

Created attachment 261527 [details]
WIP, rebased.

[Andre Moreira Magalhaes]

We added support for multiple frontend channels on top of wk2gtk 2.10.4 for a client project and we took a slightly different approach here (passing FrontendChannel to all methods requiring a response (instead of a BackendRequest structure containing the Channel and the Message).

We also added support for running the embedded inspector UI (right click -> Inspect Element) together with one or more browser instances running on WEBKIT_INSPECTOR_SERVER address (which right now invokes WebKitInspector::remoteFrontendConnected but doesnt create a new frontendchannel for that).

I opened bug 153123 and plan to post a rebased version there.

[Blaze Burg]

(In reply to comment #4)
> We added support for multiple frontend channels on top of wk2gtk 2.10.4 for
> a client project and we took a slightly different approach here (passing
> FrontendChannel to all methods requiring a response (instead of a
> BackendRequest structure containing the Channel and the Message).
> 
> We also added support for running the embedded inspector UI (right click ->
> Inspect Element) together with one or more browser instances running on
> WEBKIT_INSPECTOR_SERVER address (which right now invokes
> WebKitInspector::remoteFrontendConnected but doesnt create a new
> frontendchannel for that).
> 
> I opened bug 153123 and plan to post a rebased version there.

Feel free to post your patch here, or in the meta bug.

I encourage you to join our IRC channel to discuss the design issues at stake so we don't spend unnecessary time in patch review. Anything landing in trunk JSC/WebCore (inspector backend code) needs to take into account the REMOTE_INSPECTOR code path used on Cocoa-based ports.

[Andre Moreira Magalhaes]

Created attachment 271350 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #6)
> Created attachment 271350 [details]
> Patch

This patch was tested by running MiniBrowser with WEBKIT_INSPECTOR_SERVER set and opening 2 inspector UI instances (right-click > Inspect Element plus another MiniBrowser window pointing to WEBKIT_INSPECTOR_SERVER addr) and an instance of chromedevtools plugin running on Eclipse. All 3 inspector frontends running in parallel and in sync.
Closing and reopening a frontend also works as expected.

I didnt test the REMOTE_INSPECTOR branch as this seem to require an osx build (which I dont have available), but I did patch a mm file for a method whose signature changed.

Please let me know what you think.

[Andre Moreira Magalhaes]

Created attachment 271351 [details]
Patch

[Andre Moreira Magalhaes]

Created attachment 271355 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 271355 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:13
> +        * inspector/InspectorAgentBase.h:
> +        (Inspector::InspectorAgentBase::frontendAttached):
> +        (Inspector::InspectorAgentBase::frontendDetached):

There should really be a description in one of these ChangeLogs about what FrontendToken represents and how it is being used.

> Source/JavaScriptCore/inspector/InspectorAgentBase.h:78
> -    virtual void didCreateFrontendAndBackend(FrontendRouter*, BackendDispatcher*) = 0;
> -    virtual void willDestroyFrontendAndBackend(DisconnectReason) = 0;
> +    virtual void frontendAttached(FrontendToken, DidAttachFirstFrontend) { }
> +    virtual void frontendDetached(FrontendToken, DidDetachLastFrontend, DisconnectReason) { }

This change is going to be a pretty big change on its own!

Could you break this out into its own patch, so it can be reviewed separate from the WebKit2 additions for remote inspection? Otherwise this patch does too much at once to review accurately.

> Source/WebCore/page/ContextMenuController.cpp:900
> -                if (!(frame->page() && (frame->page()->inspectorController().inspectionLevel() > 0 || frame->page()->inspectorController().hasRemoteFrontend()))) {
> +                if (!(frame->page() && (frame->page()->inspectorController().inspectionLevel() > 0))) {

Hmm, why was this removed?

[Joseph Pecoraro]

This is neat! I can't wait to see where this goes. But please break the large patch down into smaller chunks!

[Andre Moreira Magalhaes]

Created attachment 271370 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #10)
> Comment on attachment 271355 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271355&action=review
> 
> > Source/JavaScriptCore/ChangeLog:13
> > +        * inspector/InspectorAgentBase.h:
> > +        (Inspector::InspectorAgentBase::frontendAttached):
> > +        (Inspector::InspectorAgentBase::frontendDetached):
> 
> There should really be a description in one of these ChangeLogs about what
> FrontendToken represents and how it is being used.
> 
> > Source/JavaScriptCore/inspector/InspectorAgentBase.h:78
> > -    virtual void didCreateFrontendAndBackend(FrontendRouter*, BackendDispatcher*) = 0;
> > -    virtual void willDestroyFrontendAndBackend(DisconnectReason) = 0;
> > +    virtual void frontendAttached(FrontendToken, DidAttachFirstFrontend) { }
> > +    virtual void frontendDetached(FrontendToken, DidDetachLastFrontend, DisconnectReason) { }
> 
> This change is going to be a pretty big change on its own!
> 
> Could you break this out into its own patch, so it can be reviewed separate
> from the WebKit2 additions for remote inspection? Otherwise this patch does
> too much at once to review accurately.
> 
I will check this out, possibly breaking it into the original (rebased) patch from Brian in one commit and my additions in a separate commit.

> > Source/WebCore/page/ContextMenuController.cpp:900
> > -                if (!(frame->page() && (frame->page()->inspectorController().inspectionLevel() > 0 || frame->page()->inspectorController().hasRemoteFrontend()))) {
> > +                if (!(frame->page() && (frame->page()->inspectorController().inspectionLevel() > 0))) {
> 
> Hmm, why was this removed?

Otherwise it would hide some (most) context menu items in the main browser view (the one being inspected) when right clicking the page and running a remote frontend (e.g. a minibrowser instance opening WEBKIT_INSPECTOR_SERVER addr). Note that we now create an instance of WebInspector (with connection type = remote) when connecting a remote frontend.

[Blaze Burg]

Comment on attachment 271370 [details]
Patch

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

The changes below WebKit2 look fine to me, but since I wrote most of that code I cannot r+ it for you.

The changes for INSPECTOR_SERVER definitely belong in a separate patch.

> Source/JavaScriptCore/ChangeLog:9
> +        Original patch from Brian Burg  <[e-mail]>.

:)

You need to fill in the changelog with a description of the patch as it's relevant to this module. Per method comments are also helpful if changes are nontrivial / not mechanical.

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:118
> +        TemporaryChange<Optional<FrontendToken>> scopedFrontend(m_currentFrontend, token);

Nit: name should be scopedFrontendToken

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:150
> +        TemporaryChange<Optional<FrontendToken>> scopedFrontend(m_currentFrontend, token);

here too.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:129
> +    if (wasFirstFrontend == DidAttachFirstFrontend::No)

I would add a comment saying that extra domains and augmenting client only expect to be called once, so break if it's not the first frontend.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:305
> +    if (m_frontendRouter->hasFrontends())

Need a comment here saying that extra agents don't care about the frontend token and only expect to be notified once.

> Source/WebCore/inspector/InspectorController.cpp:-332
> -        m_inspectorClient->bringFrontendToFront();

This seems wrong. Now we will always create a new frontend even if there is already a local one.

> Source/WebCore/inspector/InspectorController.cpp:338
> +    if (frontendChannel) {

Moving this out of the else-if is not any clearer.

> Source/WebCore/inspector/InspectorController.cpp:339
> +        // Make sure frontend is connected

Comment not useful.

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:78
> +    

Be sure to mention this change in the ChangeLog.

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:105
>          // the owning frontend client, so keep a protector reference here.

Nit: grammar no longer makes sense

> Source/WebCore/page/ContextMenuController.cpp:900
> +                if (!(frame->page() && (frame->page()->inspectorController().inspectionLevel() > 0))) {

I don't remember what this change was for.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:69
> +    HashMap<unsigned, Vector<WebSocketServerConnection*>>::iterator end = m_connectionMap.end();

Use 'auto' for iterator types to make this readable.

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:29
> +#include "WebInspectorRemoteManager.h"

Nit: add newline after the guard

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:68
> +    HashMap<uint64_t, RefPtr<WebInspector>>::const_iterator end = m_remoteFrontends.end();

Nit: use 'auto'

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.h:2
> + * Copyright (C) 2015 Collabora Ltd.

Should probably be 2015, 2016.

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.h:67
> +    WebPage* m_page;

Nit: add { nullptr }

> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.h:73
> +#endif

Nit: // ENABLE(INSPECTOR_SERVER)

[Blaze Burg]

(In reply to comment #11)
> This is neat! I can't wait to see where this goes. But please break the
> large patch down into smaller chunks!

Aside from the WK2 stuff, the rest isn't really worth breaking down IMO. It will just be shuffling around arguments at the same call sites and functions.

[Blaze Burg]

Comment on attachment 271370 [details]
Patch

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

r- this version since it needs splitting and build fixes. Not many serious things to fix, though.

> Source/JavaScriptCore/inspector/remote/RemoteControllableTarget.h:-55
> -    virtual void dispatchMessageFromRemote(const String& message) = 0;

Please undo this function renaming. It specifically does not mention 'frontend' because the remote end could be an automation test instead of an inspector.

[Andre Moreira Magalhaes]

Created attachment 271393 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #17)
> Created attachment 271393 [details]
> Patch

This patch addresses most of the review comments except for the split and ChangeLog updates.

I plan to work on them next.

[WebKit Commit Bot]

Attachment 271393 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andre Moreira Magalhaes]

(In reply to comment #14)
> Comment on attachment 271370 [details]
> > Source/WebCore/inspector/InspectorController.cpp:-332
> > -        m_inspectorClient->bringFrontendToFront();
> 
> This seems wrong. Now we will always create a new frontend even if there is
> already a local one.
> 
Indeed, this was a leftover for when I was trying multiple inspectors but creating a WebInspector with connection type Local (now it uses Remote for remote connections). Reverted this change.

> > Source/WebCore/page/ContextMenuController.cpp:900
> > +                if (!(frame->page() && (frame->page()->inspectorController().inspectionLevel() > 0))) {
> 
> I don't remember what this change was for.
> 
See my reply on #c13.

> > Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:29
> > +#include "WebInspectorRemoteManager.h"
> 
> Nit: add newline after the guard
> 
For this one I added the newline but webkit-patch upload complained with:
"ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]".
I kept the newline in any case :).

[Andre Moreira Magalhaes]

Created attachment 271396 [details]
Patch

[WebKit Commit Bot]

Attachment 271396 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andre Moreira Magalhaes]

I will do the split once I get this to build in all ports (need to use the buildbots for win/mac as I dont have any available).

Please also let me know what to do with the style issue on comment 22 (it gets fixed by removing the newline after the guard on Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30).

[Andre Moreira Magalhaes]

Created attachment 271400 [details]
Patch

Remove duplicated ChangeLog entries

[WebKit Commit Bot]

Attachment 271400 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

(In reply to comment #23)
> I will do the split once I get this to build in all ports (need to use the
> buildbots for win/mac as I dont have any available).

If you can't make any progress after a few tries, I can spin a Mac build tomorrow.
 
> Please also let me know what to do with the style issue on comment 22 (it
> gets fixed by removing the newline after the guard on
> Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30).

Ignore it. The style checker is pedantic and also not very smart, which can be annoying.

[Blaze Burg]

Created attachment 271428 [details]
Build Fix for Mac

[WebKit Commit Bot]

Attachment 271428 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/InspectorFrontendRouter.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 93 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 271428 [details]
Build Fix for Mac

Attachment 271428 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/840975

New failing tests:
inspector/protocol/backend-dispatcher-malformed-message-errors.html

[Build Bot]

Created attachment 271435 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 271428 [details]
Build Fix for Mac

Attachment 271428 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/840977

New failing tests:
inspector/protocol/backend-dispatcher-malformed-message-errors.html

[Build Bot]

Created attachment 271436 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 271428 [details]
Build Fix for Mac

Attachment 271428 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/840972

New failing tests:
inspector/protocol/backend-dispatcher-malformed-message-errors.html

[Build Bot]

Created attachment 271439 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

(In reply to comment #33)
> Comment on attachment 271428 [details]
> Build Fix for Mac
> 
> Attachment 271428 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/840972
> 
> New failing tests:
> inspector/protocol/backend-dispatcher-malformed-message-errors.html

Looks like there is a regression on this test. You should try enabling logging (see the wiki) and find out what broke it.

[Andre Moreira Magalhaes]

Created attachment 279611 [details]
Patch

[WebKit Commit Bot]

Attachment 279611 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 96 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andre Moreira Magalhaes]

(In reply to comment #36)
> Created attachment 279611 [details]
> Patch

This patch should fix the regression on the inspector/protocol/backend-dispatcher-malformed-message-errors.html test. The issue was that the test was expecting an error/response without an "id" (id:null) and the backend was sending a valid id, even though there was none (BackendRequest.requestId was set to 0 - now changed to Optional<long> and properly checked before setting "id" on the response).

Note that it is still pending to split the patch and update the ChangeLogs though. I will do it after done with build/test issues.

[Andre Moreira Magalhaes]

Created attachment 279618 [details]
Patch

[WebKit Commit Bot]

Attachment 279618 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 98 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andre Moreira Magalhaes]

Created attachment 279620 [details]
Patch

[WebKit Commit Bot]

Attachment 279620 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorRemoteManager.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 98 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andre Moreira Magalhaes]

Created attachment 279804 [details]
Staged patch

The original patch was split in 2, one for this bug and another one for bug 153123. Note that the patch on bug 153123 depends on this one and will not build without it.

Please use the combined patch on https://bugs.webkit.org/attachment.cgi?id=279620 if testing both functionalities.

The new patches (when combined) are the same as the combined patch but with updated ChangeLog entries.

[Blaze Burg]

(In reply to comment #43)
> Created attachment 279804 [details]
> Staged patch
> 
> The original patch was split in 2, one for this bug and another one for bug
> 153123. Note that the patch on bug 153123 depends on this one and will not
> build without it.
> 
> Please use the combined patch on
> https://bugs.webkit.org/attachment.cgi?id=279620 if testing both
> functionalities.
> 
> The new patches (when combined) are the same as the combined patch but with
> updated ChangeLog entries.

Hi again, I am sorry about the horrible delay in reviewing. If you can post rebased patches, I will take a look this weekend.

[Blaze Burg]

Comment on attachment 279620 [details]
Patch

Please upload rebased patches, or unassign yourself from the bug. I would like to make progress on this feature.

[Andre Moreira Magalhaes]

Sorry the delay, quite busy here these days, rebased patches coming...

[Andre Moreira Magalhaes]

Created attachment 285014 [details]
Patch

[WebKit Commit Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Andre Moreira Magalhaes]

(In reply to comment #47)
> Created attachment 285014 [details]
> Patch

This is the rebased patch (without changes from bug 153123).

[Blaze Burg]

Comment on attachment 285014 [details]
Patch

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

r=me.

I think this is ready to go as-is. There are some follow-up refactoring to do, but we will need to audit each agent for frontend-specific handling of data anyway.

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:131
>      Optional<long> m_currentRequestId { Nullopt };

Looking back on this, it seems like we could just store a BackendRequest as m_currentRequest and the id could be Nullopt. If we have protocol errors we could staple them to the related request rather than a separate field. This restructuring is independent of this patch, so don't worry about it for now.

> Source/JavaScriptCore/inspector/InspectorFrontendRouter.cpp:118
> +    return 0;

This should return a Nullopt rather than using 0 as an invalid value.

> Source/JavaScriptCore/inspector/InspectorFrontendRouter.cpp:130
> +    return nullptr;

But an Optional<T> is not necessary here due to nullptr being acceptable as an invalid value.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:135
> +    // extra domains and augmenting client only expect to be called once and will

Nit: capitalize 'extra'

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:173
> +    m_agents.frontendDetached(0, DidDetachLastFrontend::Yes, DisconnectReason::InspectedTargetDestroyed);

Seems we should pass a Nullopt here instead of 0. We can make that refactor in a follow-up patch.

[Blaze Burg]

Comment on attachment 285014 [details]
Patch

I think you need to provide rebaselined code generator results. Do `run-inspector-generator-tests --reset-results`

[Andre Moreira Magalhaes]

Created attachment 289821 [details]
Patch

[Andre Moreira Magalhaes]

(In reply to comment #52)
> Created attachment 289821 [details]
> Patch

This new patch is the same as the last one but rebased to latest master and also including the updated code generator results.

[Joseph Pecoraro]

Comment on attachment 289821 [details]
Patch

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

r- for the open questions.

> Source/WebCore/ChangeLog:14
> +        * inspector/InspectorApplicationCacheAgent.cpp:
> +        (WebCore::InspectorApplicationCacheAgent::frontendDetached):
> +        * inspector/InspectorApplicationCacheAgent.h:
> +        * inspector/InspectorCSSAgent.cpp:

Do any of the Agents actually do anything if this was not the First attach or Last detach?

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:39
> +typedef uint32_t FrontendToken;

Maybe this file should just include InspectorAgentBase to avoid redefining this and having them get out of date.

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:131
> +    Optional<FrontendToken> m_currentFrontend { Nullopt };

m_currentFrontendToken would be clearer.

> Source/JavaScriptCore/inspector/InspectorFrontendRouter.h:60
> +    FrontendToken s_nextFrontendToken = 1;

This appears to be a static with the "s_" prefix, but isn't? You could make it a static by adding the `static` keyword, or rename it to m_nextFrontendToken.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:136
> +    // extra domains and augmenting client only expect to be called once and will

Nit: Comments should be sentences that start with a capital.

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:311
> +        // extra agents don't care about the frontend token and only expect to be notified once.

Nit: Comments should be sentences that start with a capital.

> Source/WebCore/inspector/InspectorController.cpp:-263
> +    m_agents.frontendAttached(token, isFirstFrontend);
>  
>      InspectorInstrumentation::frontendCreated();
>  
> -    if (connectedFirstFrontend) {
> +    if (isFirstFrontend == DidAttachFirstFrontend::Yes)
>          InspectorInstrumentation::registerInstrumentingAgents(m_instrumentingAgents.get());
> -        m_agents.didCreateFrontendAndBackend(&m_frontendRouter.get(), &m_backendDispatcher.get());
> -    }

The order of these operations changed slightly. I don't think it causes any issues, but it seems like we should be balanced with disconnect. The order of InspectorInstrumentation::frontendCreated/Deleted, frontendAttached/Detached, and InspectorInstrumentation::registerInstrumentingAgents/deregister should be some order of A, B, C in connectFrontend and C, B, A in disconnectFrontend. I could imagine a scenario where frontendAttached does something expecting to trigger an InspectorInstrumentation call, but that could bail if InspectorInstrumentation hasn't been notified there is a frontend yet.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:231
> -void InspectorDOMAgent::didCreateFrontendAndBackend(Inspector::FrontendRouter*, Inspector::BackendDispatcher*)
> +void InspectorDOMAgent::frontendAttached(FrontendToken, DidAttachFirstFrontend)
>  {
>      m_history = std::make_unique<InspectorHistory>();
>      m_domEditor = std::make_unique<DOMEditor>(m_history.get());

I think we need a better pattern here. This may work, but I can see subtle bugs crop up for multiple frontends.

It seem we are calling InspectorFooAgent::frontendAttached and InspectorFooAgent::frontendDetached on every agent whenever a frontend attaches / detaches. Pretty much every agent only expects to do work when the first frontend connects or the last disconnects. For instance, most just add/remove themselves from InstrumentingAgents. That only needs to happen on first/last.

DOMAgent is clearing state here in attach that another frontend might have cared about. It seems wrong to clear that data when the other frontend is still around. This also sounds like a weird place to do it. at all.

Most agents send "first open" data to the frontend in response to the `enable()` command. DebuggerAgent for example pushes didParseScript messages to the frontend as a response to enable. ConsoleAgent pushes stashed console messages.

So a few questions:

1. Does any agent do work in frontendAttached / frontendDetached that it really needs to do for every attach/detach or just the first / last?
  - can we reduce the InspectorAgent interface to just frontendAttached(Token) and frontendDetached(Token, DisconnectReason) and have it only be the first/last.

2. What should agents do in enable/disable when there are other frontends connected?
  - If Frontend #1 connects and calls enable() DebuggerAgent will send along its scripts (scriptParsed messages)
  - If Frontend #2 connects and calls enable() DebuggerAgent will do nothing since it was already enabled.
  - If Frontend #2 were to call disable() should Frontend #1 be affected?

3. Should we have general + specific hooks: firstFrontendAttached / lastFrontendDetached and specific frontendAttached(token)/frontendDetached(token)
  - would anyone even implement the specific hooks?

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:77
> +    ~InspectorBackendDispatchTask()
> +    {
> +        if (m_timer.isActive())
> +            m_timer.stop();
> +    }

~Timer will stop the timer. Is this actually fixing an issue? If not, I think we should drop it and keep the default constructor.

[Blaze Burg]

Comment on attachment 289821 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        * inspector/InspectorCSSAgent.cpp:
> 
> Do any of the Agents actually do anything if this was not the First attach or Last detach?

Right now, they don't. In subsequent patches, they will need to set up/tear down any per-frontend state (thinking specifically of pushed nodes for DOMAgent).

[Blaze Burg]

Comment on attachment 289821 [details]
Patch

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

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:231
>>      m_domEditor = std::make_unique<DOMEditor>(m_history.get());
> 
> I think we need a better pattern here. This may work, but I can see subtle bugs crop up for multiple frontends.
> 
> It seem we are calling InspectorFooAgent::frontendAttached and InspectorFooAgent::frontendDetached on every agent whenever a frontend attaches / detaches. Pretty much every agent only expects to do work when the first frontend connects or the last disconnects. For instance, most just add/remove themselves from InstrumentingAgents. That only needs to happen on first/last.
> 
> DOMAgent is clearing state here in attach that another frontend might have cared about. It seems wrong to clear that data when the other frontend is still around. This also sounds like a weird place to do it. at all.
> 
> Most agents send "first open" data to the frontend in response to the `enable()` command. DebuggerAgent for example pushes didParseScript messages to the frontend as a response to enable. ConsoleAgent pushes stashed console messages.
> 
> So a few questions:
> 
> 1. Does any agent do work in frontendAttached / frontendDetached that it really needs to do for every attach/detach or just the first / last?
>   - can we reduce the InspectorAgent interface to just frontendAttached(Token) and frontendDetached(Token, DisconnectReason) and have it only be the first/last.
> 
> 2. What should agents do in enable/disable when there are other frontends connected?
>   - If Frontend #1 connects and calls enable() DebuggerAgent will send along its scripts (scriptParsed messages)
>   - If Frontend #2 connects and calls enable() DebuggerAgent will do nothing since it was already enabled.
>   - If Frontend #2 were to call disable() should Frontend #1 be affected?
> 
> 3. Should we have general + specific hooks: firstFrontendAttached / lastFrontendDetached and specific frontendAttached(token)/frontendDetached(token)
>   - would anyone even implement the specific hooks?

Let me respond briefly.

1) DOM agent and Debugger agent will need to create/clear new per-frontend data structures. This will be done in a subsequent patch(es). For now it still works correctly as there is other support needed for multiple frontends to connect.
2) The backend should respect the most recent enable() or disable(). Code that blows away all state should be moved out of enable()/disable(), it should really just be to turn instrumentation on and off. We may want to ignore those calls if timeline recording is active, and have the frontend defer enabling if it sees recording is ongoing. And it will definitely need to fast-forward updates (scriptParsed, etc) to a new frontend where possible.
3) No, we have too many methods already. See 1)

[Andre Moreira Magalhaes]

Created attachment 289927 [details]
Patch

[Andre Moreira Magalhaes]

Thanks for the review. Comments below. 

(In reply to comment #54)
> Comment on attachment 289821 [details]
>  
> > Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:39
> > +typedef uint32_t FrontendToken;
> 
> Maybe this file should just include InspectorAgentBase to avoid redefining
> this and having them get out of date.
> 
Done.

> > Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:131
> > +    Optional<FrontendToken> m_currentFrontend { Nullopt };
> 
> m_currentFrontendToken would be clearer.
>
Done
 
> > Source/JavaScriptCore/inspector/InspectorFrontendRouter.h:60
> > +    FrontendToken s_nextFrontendToken = 1;
> 
> This appears to be a static with the "s_" prefix, but isn't? You could make
> it a static by adding the `static` keyword, or rename it to
> m_nextFrontendToken.
> 
Done, changed to static.

> > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:136
> > +    // extra domains and augmenting client only expect to be called once and will
> 
> Nit: Comments should be sentences that start with a capital.
>
Done.
 
> > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:311
> > +        // extra agents don't care about the frontend token and only expect to be notified once.
> 
> Nit: Comments should be sentences that start with a capital.
> 
Done.

> > Source/WebCore/inspector/InspectorController.cpp:-263
> > +    m_agents.frontendAttached(token, isFirstFrontend);
> >  
> >      InspectorInstrumentation::frontendCreated();
> >  
> > -    if (connectedFirstFrontend) {
> > +    if (isFirstFrontend == DidAttachFirstFrontend::Yes)
> >          InspectorInstrumentation::registerInstrumentingAgents(m_instrumentingAgents.get());
> > -        m_agents.didCreateFrontendAndBackend(&m_frontendRouter.get(), &m_backendDispatcher.get());
> > -    }
> 
> The order of these operations changed slightly. I don't think it causes any
> issues, but it seems like we should be balanced with disconnect. The order
> of InspectorInstrumentation::frontendCreated/Deleted,
> frontendAttached/Detached, and
> InspectorInstrumentation::registerInstrumentingAgents/deregister should be
> some order of A, B, C in connectFrontend and C, B, A in disconnectFrontend.
> I could imagine a scenario where frontendAttached does something expecting
> to trigger an InspectorInstrumentation call, but that could bail if
> InspectorInstrumentation hasn't been notified there is a frontend yet.
>
Done. Actually now it does not match A/B/C -> C/B/A exactly, but it does keep the same behaviour as it was before.

> > Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:77
> > +    ~InspectorBackendDispatchTask()
> > +    {
> > +        if (m_timer.isActive())
> > +            m_timer.stop();
> > +    }
> 
> ~Timer will stop the timer. Is this actually fixing an issue? If not, I
> think we should drop it and keep the default constructor.
Done.

The only missing bits are related to the open questions which Brian already replied.

[Joseph Pecoraro]

> Let me respond briefly.
> 
> 1) DOM agent and Debugger agent will need to create/clear new per-frontend
> data structures. This will be done in a subsequent patch(es). For now it
> still works correctly as there is other support needed for multiple
> frontends to connect.

Wouldn't it be better to take the conservative approach first?

Each frontendAttached would early return if DidAttachFirstFrontend::No. Then do things only to those agents that need it?

> 2) The backend should respect the most recent enable() or disable(). Code
> that blows away all state should be moved out of enable()/disable(), it
> should really just be to turn instrumentation on and off. We may want to
> ignore those calls if timeline recording is active, and have the frontend
> defer enabling if it sees recording is ongoing. And it will definitely need
> to fast-forward updates (scriptParsed, etc) to a new frontend where possible.

Currently Agent.enable() means "you can start sending events to me". That includes messages it has buffered up to send to a frontend (Console Messages) or information the frontend needs (Script Parsed). We never call disable directly from our frontend, so what it should do hasn't been a concern of ours yet.

> 3) No, we have too many methods already. See 1)

InspectorAgentBase only has 3 methods right now:

    virtual void didCreateFrontendAndBackend(FrontendRouter*, BackendDispatcher*) = 0;
    virtual void willDestroyFrontendAndBackend(DisconnectReason) = 0;
    virtual void discardAgent() { }

The top two are getting renamed. I'm saying we split them:

    virtual void firstFrontendAttached() = 0;
    virtual void lastFrontendDetached() = 0;

    virtual void extraFrontendAttached() = 0;
    virtual void extraFrontendDetached() = 0;

And only 1 or 2 agents might implement "extraFrontendAttached / Detached" if they have per-fronted data.

The reason I find this useful is because currently we have to write "if (Foo) return;" in every single agent frontendAttached / frontendDetached, which is both wasteful and possible to be missed each time a new agent is added. Its error prone. While having an explicit approach would make it easy to search and say "this agent, by implementing extraFrontend* has special handling when multiple frontends are connected".

[Blaze Burg]

(In reply to comment #59)
> > Let me respond briefly.
> > 
> > 1) DOM agent and Debugger agent will need to create/clear new per-frontend
> > data structures. This will be done in a subsequent patch(es). For now it
> > still works correctly as there is other support needed for multiple
> > frontends to connect.
> 
> Wouldn't it be better to take the conservative approach first?

It's going to get added eventually, but if there are no clients in this patch in particular, I can defer that to when I fix up relevant agents to track data per-frontend. So, for this patch we can go firstFrontendAttached/lastFrontendDetached.

> 
> Each frontendAttached would early return if DidAttachFirstFrontend::No. Then
> do things only to those agents that need it?
> 
> > 2) The backend should respect the most recent enable() or disable(). Code
> > that blows away all state should be moved out of enable()/disable(), it
> > should really just be to turn instrumentation on and off. We may want to
> > ignore those calls if timeline recording is active, and have the frontend
> > defer enabling if it sees recording is ongoing. And it will definitely need
> > to fast-forward updates (scriptParsed, etc) to a new frontend where possible.
> 
> Currently Agent.enable() means "you can start sending events to me". That
> includes messages it has buffered up to send to a frontend (Console
> Messages) or information the frontend needs (Script Parsed). We never call
> disable directly from our frontend, so what it should do hasn't been a
> concern of ours yet.
> 
> > 3) No, we have too many methods already. See 1)

Okay, maybe I was being flippant =)

> 
> InspectorAgentBase only has 3 methods right now:
> 
>     virtual void didCreateFrontendAndBackend(FrontendRouter*,
> BackendDispatcher*) = 0;
>     virtual void willDestroyFrontendAndBackend(DisconnectReason) = 0;
>     virtual void discardAgent() { }
> 
> The top two are getting renamed. I'm saying we split them:
> 
>     virtual void firstFrontendAttached() = 0;
>     virtual void lastFrontendDetached() = 0;
> 
>     virtual void extraFrontendAttached() = 0;
>     virtual void extraFrontendDetached() = 0;
> 
> And only 1 or 2 agents might implement "extraFrontendAttached / Detached" if
> they have per-fronted data.

I agree with this, except it shouldn't matter if it's the first or extra frontend, any frontend-specific work should go in these methods.

> 
> The reason I find this useful is because currently we have to write "if
> (Foo) return;" in every single agent frontendAttached / frontendDetached,
> which is both wasteful and possible to be missed each time a new agent is
> added. Its error prone. While having an explicit approach would make it easy
> to search and say "this agent, by implementing extraFrontend* has special
> handling when multiple frontends are connected".

I would be okay with this, as long as it's possible to split the frontend-agnostic work (like enabling) from the work of setting up one frontend (adding a new frontend to a map structure and fast-forwarding a new frontend). So I would like the sequence for a single frontend attaching and detaching to be

firstFrontendAttached()
frontendAttached()
frontendDetached()
lastFrontendDetached()

and for two frontends:

firstFrontendAttached()
frontendAttached()
frontendAttached()
frontendDetached()
frontendDetached()
lastFrontendDetached()

That way, we don't have to care whether it's an extra frontend or not. And we won't have to awkwardly reuse code that's shared between firstFrontendAttached and extraFrontendAttached. This still lets us get rid of the pervasive if(first/lastFrontend) checks.

[Blaze Burg]

Comment on attachment 289927 [details]
Patch

Clearing r? as we probably want a more conservative scope for this bug per last few comments. I can pick this up later tonight if Andre is not up yet.

[Joseph Pecoraro]

(In reply to comment #60)
> (In reply to comment #59)
> > > Let me respond briefly.
> > > 
> > > 1) DOM agent and Debugger agent will need to create/clear new per-frontend
> > > data structures. This will be done in a subsequent patch(es). For now it
> > > still works correctly as there is other support needed for multiple
> > > frontends to connect.
> > 
> > Wouldn't it be better to take the conservative approach first?
> 
> It's going to get added eventually, but if there are no clients in this
> patch in particular, I can defer that to when I fix up relevant agents to
> track data per-frontend. So, for this patch we can go
> firstFrontendAttached/lastFrontendDetached.

That sounds good. Either way this first step is providing something we know doesn't provide complete support for multiple frontends.


> > > 3) No, we have too many methods already. See 1)
> 
> Okay, maybe I was being flippant =)
> 
> > 
> > InspectorAgentBase only has 3 methods right now:
> > 
> >     virtual void didCreateFrontendAndBackend(FrontendRouter*,
> > BackendDispatcher*) = 0;
> >     virtual void willDestroyFrontendAndBackend(DisconnectReason) = 0;
> >     virtual void discardAgent() { }
> > 
> > The top two are getting renamed. I'm saying we split them:
> > 
> >     virtual void firstFrontendAttached() = 0;
> >     virtual void lastFrontendDetached() = 0;
> > 
> >     virtual void extraFrontendAttached() = 0;
> >     virtual void extraFrontendDetached() = 0;
> > 
> > And only 1 or 2 agents might implement "extraFrontendAttached / Detached" if
> > they have per-fronted data.
> 
> I agree with this, except it shouldn't matter if it's the first or extra
> frontend, any frontend-specific work should go in these methods.
> 
> > The reason I find this useful is because currently we have to write "if
> > (Foo) return;" in every single agent frontendAttached / frontendDetached,
> > which is both wasteful and possible to be missed each time a new agent is
> > added. Its error prone. While having an explicit approach would make it easy
> > to search and say "this agent, by implementing extraFrontend* has special
> > handling when multiple frontends are connected".
> 
> I would be okay with this, as long as it's possible to split the
> frontend-agnostic work (like enabling) from the work of setting up one
> frontend (adding a new frontend to a map structure and fast-forwarding a new
> frontend). So I would like the sequence for a single frontend attaching and
> detaching to be
> 
> firstFrontendAttached()
> frontendAttached()
> frontendDetached()
> lastFrontendDetached()
> 
> and for two frontends:
> 
> firstFrontendAttached()
> frontendAttached()
> frontendAttached()
> frontendDetached()
> frontendDetached()
> lastFrontendDetached()

Sounds good. I waffled on the approach.

I don't mind if that comes as a follow-up or now. But seeing as we are already renaming these methods everywhere, it would be easier to do it now then introduce a follow-up that renames the same method again =)