DevTools: introduce Target.grantRemoteDebuggingSocket() method [chromium/src : master]

Andrey Lushnikov (Gerrit)

unread,

May 19, 2018, 6:14:34 AM5/19/18

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

please, take a look.

It looks like headless_js_bindings_browsertests can't be migrated onto this as part of this patch.

Patch set 2:Commit-Queue +1

View Change

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Dmitry Gozman (Gerrit)

unread,

May 21, 2018, 1:04:32 PM5/21/18

to Andrey Lushnikov, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

We gotta have a test!

View Change

5 comments:

File content/browser/devtools/protocol/target_handler.h:
- Patch Set #2, Line 112: BrowserDevToolsAgentHost* browser_host_;
  Instead of passing one, use DevToolsAgentHost::CreateForDiscovery().
File content/browser/devtools/protocol/target_handler.cc:
- Patch Set #2, Line 155: class BrowserClient : public DevToolsAgentHostClient {
  You can just make BrowserToPageConnector a DevToolsAgentHostClient and use passed agent_host to differentiate between two parties.
- Patch Set #2, Line 394: return Response::OK();
  Cleanup connectors here.
- Patch Set #2, Line 563: browser_host_
  Does |browser_only_| work for you?
- Patch Set #2, Line 584: browser_to_page_connectors_[agent_host->GetId()].reset(connector);
  Why map from string? Make it from DevToolsAgentHost* to connector.

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Andrey Lushnikov (Gerrit)

unread,

May 22, 2018, 7:11:22 PM5/22/18

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Commit Bot, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

View Change

5 comments:

File content/browser/devtools/protocol/target_handler.h:

- Patch Set #2, Line 112: BrowserDevToolsAgentHost* browser_host_;
  Instead of passing one, use DevToolsAgentHost::CreateForDiscovery().

- Patch Set #2, Line 155: class BrowserClient : public DevToolsAgentHostClient {

- You can just make BrowserToPageConnector a DevToolsAgentHostClient and use passed agent_host to diff […]
  Done

- Patch Set #2, Line 394: return Response::OK();
  Cleanup connectors here.

- I moved connectors to a global variable; I don't think clearing them here is needed any more.

- Patch Set #2, Line 563: browser_host_
  Does |browser_only_| work for you?

- I removed the restriction since the ::CreateForDiscovery lets me to.

- Patch Set #2, Line 584: browser_to_page_connectors_[agent_host->GetId()].reset(connector);
  Why map from string? Make it from DevToolsAgentHost* to connector.

- Done

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Dmitry Gozman (Gerrit)

unread,

May 22, 2018, 7:44:06 PM5/22/18

to Andrey Lushnikov, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

View Change

9 comments:

File content/browser/devtools/protocol/target_handler.cc:
- Patch Set #3, Line 30: window.CDPSocket = {};
  let's pass a name over protocol.
- Patch Set #3, Line 55: g_browser_to_page_connectors;
  Maybe you don't even need a map?
- Patch Set #3, Line 71: SendProtocolMessageToPage("Page.addScriptToEvaluateOnLoad",
  Is this main-frame only? If not, we should make it so (or pass a frame id).
- Patch Set #3, Line 148: "window.CDPSocket.onmessage(atob(\"" + encoded + "\"))");
  This might be very big. Do a reserve in advance. See https://cs.chromium.org/chromium/src/content/browser/devtools/protocol/tracing_handler.cc?rcl=ec54e1ca07094b9b8e737d9e87e72555ef2215b5&l=267 for example.
- Patch Set #3, Line 513: Response TargetHandler::GrantRemoteDebuggingSocket(
  Let's only do this when |browser_only_|, otherwise you are widening the capability.
- Patch Set #3, Line 600: g_browser_to_page_connectors.Get().erase(host);
  You don't need this one, connector handles cleanup itself.
File third_party/WebKit/LayoutTests/inspector-protocol/target/target-grant-remote-debugging.js:
- Patch Set #3, Line 5: await dp.Target.grantRemoteDebuggingSocket({targetId: page._targetId});
  This is the same page you are talking to. Create two pages instead. Or better yet, grant capability through browser target to a single page.
- Patch Set #3, Line 28: testRunner.log('CDP Socket responses: ' + responses.length);
  If you dump the number of responses, might as well dump their contents.
File third_party/blink/renderer/core/inspector/browser_protocol.pdl:
- Patch Set #3, Line 5538: grantRemoteDebuggingSocket
  grantRemoteDebuggingCapability

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Andrey Lushnikov (Gerrit)

unread,

May 25, 2018, 6:39:54 PM5/25/18

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Commit Bot, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

Patch set 6:Commit-Queue +1

View Change

9 comments:

File content/browser/devtools/protocol/target_handler.cc:
- Patch Set #3, Line 30: std::unique_ptr<Target::TargetInfo> CreateInfo(DevToolsAgentHost* host) {

- let's pass a name over protocol.

- Done
- Patch Set #3, Line 55: DevToolsAgentHost* page_host)

- Maybe you don't even need a map?

- Switched to flat_map; should work better for our needs.
- Patch Set #3, Line 71: std::unique_ptr<base::DictionaryValue> params =

- Is this main-frame only? If not, we should make it so (or pass a frame id).

- The addScriptToEvaluateOnLoad injects in all the frames, but we need a mainframe-only.
  Changed this to check for window.top === window.self in initializer script to avoid spoiling global space of nested frames.
- Patch Set #3, Line 148: std::string eval_code = "window." + binding_name_ + ".onmessage(atob(\"";
  This might be very big. Do a reserve in advance. See https://cs.chromium. […]
  Done
- Patch Set #3, Line 513: bool* out_success) {

- Let's only do this when |browser_only_|, otherwise you are widening the capability.

- Done

- Patch Set #3, Line 600:

- You don't need this one, connector handles cleanup itself.
  Done
File third_party/WebKit/LayoutTests/inspector-protocol/target/target-grant-remote-debugging.js:

- Patch Set #3, Line 5:

- This is the same page you are talking to. Create two pages instead. […]
  I don't see benefits in two pages: I still need to connect to the page with granted
  remote debugging capability so that I can evaluate inside it.

- Patch Set #3, Line 28:

- If you dump the number of responses, might as well dump their contents.
  This will add unnecessary output to the test: we don't really care what's inside targetInfo,
  we only care that responses and events are delivered over the page binding.
File third_party/blink/renderer/core/inspector/browser_protocol.pdl:
- Patch Set #3, Line 5538: method to send messages o
  grantRemoteDebuggingCapability
  Done

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Andrey Lushnikov (Gerrit)

unread,

May 25, 2018, 7:29:50 PM5/25/18

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Commit Bot, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

Patch set 7:Commit-Queue +1

View Change

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Dmitry Gozman (Gerrit)

unread,

May 29, 2018, 8:09:39 PM5/29/18

to Andrey Lushnikov, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Commit Bot, Pavel Feldman, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

lgtm

Patch set 8:Code-Review +1

View Change

7 comments:

File content/browser/devtools/protocol/target_handler.cc:
- Patch Set #8, Line 141: std::string encoded;
  DCHECK(agent_host == browser_host_.get())
- Patch Set #8, Line 156: browser_host_->DetachClient(this);
  You should not call DetachClient on the agent host which called AgentHostClosed.
- Patch Set #8, Line 522: if (!browser_only_)
  style: {}
- Patch Set #8, Line 540: if (binding_name.size() > 1024 || binding_name.size() == 0)
  style: {}
- Patch Set #8, Line 555: g_browser_to_page_connectors.Get()[agent_host.get()].reset(connector);
  Do this in BrowserToPageConnector constructor instead.
File third_party/blink/renderer/core/inspector/browser_protocol.pdl:
- Patch Set #8, Line 5539: json
  - `binding.send(json)`
  - `binding.onmessage = json => handleMessage(json)`
- Patch Set #8, Line 5541: # > **NOTE** This method is available only from browser session.
  Drop this one :-)

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Pavel Feldman (Gerrit)

unread,

May 30, 2018, 10:42:36 PM5/30/18

to Andrey Lushnikov, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Commit Bot, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

Awesome!

View Change

3 comments:

File third_party/blink/renderer/core/inspector/browser_protocol.pdl:
- Patch Set #8, Line 5542: RemoteDebuggingCapability
  It is not remote. exposeDevToolsProtocol(targetId) ?
- Patch Set #8, Line 5545: regex
  Why is this necessary?
- Patch Set #8, Line 5546: string bindingName
  # Binding name, 'cdp' if not specified.
  optional string name

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Pavel Feldman (Gerrit)

unread,

Jun 11, 2018, 2:11:08 PM6/11/18

to Andrey Lushnikov, apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Dmitry Gozman, Commit Bot, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

Is there a blocker or should we be able to land it?

View Change

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Andrey Lushnikov (Gerrit)

unread,

Jun 13, 2018, 7:24:29 PM6/13/18

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Pavel Feldman, Dmitry Gozman, Commit Bot, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

Addressed all the comments and migrated to use Runtime.addBinding method

View Change

10 comments:

File content/browser/devtools/protocol/target_handler.cc:
- Patch Set #8, Line 141: message.SetInteger("id", page_message_id_++);
  DCHECK(agent_host == browser_host_. […]
  Done
- Patch Set #8, Line 156: base::Value* method = value->FindKey("method");

- You should not call DetachClient on the agent host which called AgentHostClosed.

- Done

- Patch Set #8, Line 522:

- style: {}
  Done
- Patch Set #8, Line 540: DevToolsAgentHost::GetForId(target_id));
  style: {}
  Done

- Patch Set #8, Line 555: }

- Do this in BrowserToPageConnector constructor instead.
  Done
File third_party/blink/renderer/core/inspector/browser_protocol.pdl:
- Patch Set #8, Line 5539: igin
  - `binding.send(json)` […]
  Done
- Patch Set #8, Line 5541: parameters
  Drop this one :-)
  Done

- Patch Set #8, Line 5542:

- It is not remote. […]
  Done

- Patch Set #8, Line 5545:

- Why is this necessary?
  Removed constrains on the binding name.
- Patch Set #8, Line 5546: t cacheStorageCont
  # Binding name, 'cdp' if not specified. […]
  Done

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Andrey Lushnikov (Gerrit)

unread,

Jun 13, 2018, 7:24:34 PM6/13/18

to apavlo...@chromium.org, blink-...@chromium.org, caseq...@chromium.org, kozyatins...@chromium.org, lushnik...@chromium.org, pfeldma...@chromium.org, Pavel Feldman, Dmitry Gozman, Commit Bot, chromium...@chromium.org, devtools...@chromium.org, John Abd-El-Malek

Patch set 9:Commit-Queue +1

View Change

To view, visit change 1066289. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward