Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470933002:
Disable agents when client disconnects (Closed)
|
Created:
4 years, 1 month ago by Sarmad Hashmi Modified:
4 years, 1 month ago Reviewers:
sadrul CC:
chromium-reviews, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable agents when client disconnects
Create a Disable method for the UiDevToolsAgent which calls the disable
method on agent that subclasses from generated backends. This is then
called from the UiDevToolsClient, which in turn has a DisableAllAgents
method called by the UiDevToolsServer when a client disconnects from
the inspector.
BUG=648701
Committed: https://crrev.com/ca5eebc52d4de0237b911dbd9ec9808a6735b050
Cr-Commit-Position: refs/heads/master@{#429591}
Patch Set 1 #
Total comments: 6
Patch Set 2 : sadruls comments #
Total comments: 2
Patch Set 3 : sadruls comments #Patch Set 4 : update to use new devtools style #Patch Set 5 : update to use new devtools style #Patch Set 6 : update to use new devtools style #Patch Set 7 : fix nit #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 38 (30 generated)
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
CL disables agents upon client disconnect so we can eventually remove the DOM agent as an observer from windows.
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_base_agent.h (right): https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_base_agent.h:31: void Init(protocol::UberDispatcher* dispatcher) override { Before this line, add: // UiDevToolsAgent: https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_client.cc (right): https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_client.cc:31: } Don't need the {} https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_server.cc:165: client->set_connection_id(UiDevToolsClient::kNotConnected); Would it make sense to combine these two into a single Disconnect() method?
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_base_agent.h (right): https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_base_agent.h:31: void Init(protocol::UberDispatcher* dispatcher) override { On 2016/11/02 01:45:38, sadrul wrote: > Before this line, add: > // UiDevToolsAgent: Done. https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_client.cc (right): https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_client.cc:31: } On 2016/11/02 01:45:39, sadrul wrote: > Don't need the {} Done. https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... File components/ui_devtools/devtools_server.cc (right): https://codereview.chromium.org/2470933002/diff/1/components/ui_devtools/devt... components/ui_devtools/devtools_server.cc:165: client->set_connection_id(UiDevToolsClient::kNotConnected); On 2016/11/02 01:45:39, sadrul wrote: > Would it make sense to combine these two into a single Disconnect() method? Great idea! Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2470933002/diff/20001/components/ui_devtools/... File components/ui_devtools/devtools_client.h (right): https://codereview.chromium.org/2470933002/diff/20001/components/ui_devtools/... components/ui_devtools/devtools_client.h:32: void DisableAllAgents(); This should go into private section? Or do you expect a use-case for DisableAllAgents() without also disconnecting at the same time? If not, maybe just remove, and move the code into Disconnect().
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2470933002/diff/20001/components/ui_devtools/... File components/ui_devtools/devtools_client.h (right): https://codereview.chromium.org/2470933002/diff/20001/components/ui_devtools/... components/ui_devtools/devtools_client.h:32: void DisableAllAgents(); On 2016/11/02 16:05:57, sadrul wrote: > This should go into private section? Or do you expect a use-case for > DisableAllAgents() without also disconnecting at the same time? If not, maybe > just remove, and move the code into Disconnect(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mhashmi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2470933002/#ps120001 (title: "fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Disable agents when client disconnects Create a Disable method for the UiDevToolsAgent which calls the disable method on agent that subclasses from generated backends. This is then called from the UiDevToolsClient, which in turn has a DisableAllAgents method called by the UiDevToolsServer when a client disconnects from the inspector. BUG=648701 ========== to ========== Disable agents when client disconnects Create a Disable method for the UiDevToolsAgent which calls the disable method on agent that subclasses from generated backends. This is then called from the UiDevToolsClient, which in turn has a DisableAllAgents method called by the UiDevToolsServer when a client disconnects from the inspector. BUG=648701 Committed: https://crrev.com/ca5eebc52d4de0237b911dbd9ec9808a6735b050 Cr-Commit-Position: refs/heads/master@{#429591} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ca5eebc52d4de0237b911dbd9ec9808a6735b050 Cr-Commit-Position: refs/heads/master@{#429591} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
