Headless: Add support for the --remote-debugging-pipe flag [chromium/src : master]
Joel Einbinder (Gerrit)
Pavel Feldman (Gerrit)
2 comments:
File headless/app/headless_shell.cc:
Patch Set #1, Line 603:
command_line.HasSwitch(switches::kRemoteDebuggingSocketFd) ||Question to reviewers: is this one used?
File headless/lib/browser/headless_devtools.cc:
Patch Set #1, Line 129:
void StartLocalDevToolsHttpHandler(HeadlessBrowser::Options* options) {Pipe handler is not an HTTP handler strictly speaking, move it to the call site instead?
To view, visit change 954405. To unsubscribe, or for help writing mail filters, visit settings.
Alex Clarke (Gerrit)
The feature seems fine. I'm happy with this when Pavel is.
2 comments:
Can we get a little more context here please?
File headless/app/headless_shell.cc:
Patch Set #1, Line 603:
command_line.HasSwitch(switches::kRemoteDebuggingSocketFd) ||Question to reviewers: is this one used?
I'm not sure whose using this, if anyone, the bug provides some context:
https://bugs.chromium.org/p/chromium/issues/detail?id=624837
To view, visit change 954405. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
I'm not sure why this would be preferable over the remote debugging fd that we already support? Could you provide some more context about the use case and why an fd doesn't work?
Pavel Feldman (Gerrit)
I'm not sure why this would be preferable over the remote debugging fd that we already support? Could you provide some more context about the use case and why an fd doesn't work?
I don't think it is better. We simply ended up having 2 versions - one that works in content shell and chrome, another working in headless. The one in Chrome works on Mac and Windows and I don't think you can wrap single pipe with TCP socket in Windows. At least I failed. That would be an argument in favor of the chrome's solution. It hard-codes the pipes though to 3 for output and 4 for input, which works for us and not sure will work for headless.
Sami Kyöstilä (Gerrit)
I don't think it is better. We simply ended up having 2 versions - one that works in content shell and chrome, another working in headless. The one in Chrome works on Mac and Windows and I don't think you can wrap single pipe with TCP socket in Windows. At least I failed. That would be an argument in favor of the chrome's solution. It hard-codes the pipes though to 3 for output and 4 for input, which works for us and not sure will work for headless.
I see. I looked around and couldn't find anyone actively using --remote-debugging-socket-fd -- My guess is folks generally have a timeout/retry solution to work around the issue. Given this I'd be fine unifying everything under --remote-debugging-pipe.
Pavel Feldman (Gerrit)
Patch set 1:Code-Review +1
Joel Einbinder (Gerrit)
Should I remove remote-debugging-fd in this patch?
Sami Kyöstilä (Gerrit)
Patch Set 1:
Should I remove remote-debugging-fd in this patch?
That would be good I think.
Joel Einbinder (Gerrit)
removed remote-debugging-fd
Sami Kyöstilä (Gerrit)
Thank you!
Patch set 2:Code-Review +1
Joel Einbinder (Gerrit)
Patch set 2:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Headless: Add support for the --remote-debugging-pipe flag
Change-Id: I89abcf3b5350722ac4d58842250acf9d4b714aa4
Reviewed-on: https://chromium-review.googlesource.com/954405
Reviewed-by: Sami Kyöstilä <skyo...@chromium.org>
Reviewed-by: Pavel Feldman <pfel...@chromium.org>
Commit-Queue: Joel Einbinder <einb...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546685}
---
M headless/app/headless_shell.cc
M headless/app/headless_shell_switches.cc
M headless/app/headless_shell_switches.h
M headless/lib/browser/headless_devtools.cc
M headless/public/headless_browser.cc
M headless/public/headless_browser.h
6 files changed, 22 insertions(+), 56 deletions(-)