Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

support pipe connection (--remote-debugging-pipe) #867

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

support pipe connection (--remote-debugging-pipe) #867

wants to merge 7 commits into from

Conversation

ZekeLu
Copy link
Member

@ZekeLu ZekeLu commented Jun 26, 2021 •
edited

For #401

This PR implements Transport with a pipe connection.

Notes:

  1. pipe connection is not supported on windows yet (see os: Pipe can't be inherited on windows golang/go#21085)
  2. it will use pipe connection by default on non-windows
  3. use chromedp.DebuggingPort to force it to use WebSocket connection
  4. it allows remote debugging pipe and port simultaneously
  5. set CHROMEDP_WS=1 to force the unit tests to use WebSocket connections

Question:

I think chromedp.Transport and chromedp.Conn should be un-exported:

  • chromedp.Transport -> chromedp.transport
  • chromedp.Conn -> chromedp.wsConn

Should we do this?

@ZekeLu
remove jlexer.Lexer/jwriter.Writer from the Conn struct
401d887 
The fields have this comment:
"reuse the easyjson structs to avoid allocs per Read/Write"

But I don't see how it will reduce the allocs. Maybe this statement
does not stand any more due to code changes?

The benchmark shows there is not big difference:

name           old time/op    new time/op    delta
TabNavigate-8    10.8ms ± 6%    10.6ms ± 3%   ~     (p=0.218 n=10+10)

name           old alloc/op   new alloc/op   delta
TabNavigate-8     288kB ± 1%     287kB ± 1%   ~     (p=0.133 n=9+10)

name           old allocs/op  new allocs/op  delta
TabNavigate-8     1.60k ± 1%     1.59k ± 1%   ~     (p=0.189 n=9+10)
@ZekeLu
implement Transport with a pipe connection
5e4c75a
@ZekeLu
refactor NewBrowser to accept a connectFunc (instead of wsURL)
e60c71b 
So that it can accept other implementations of the Transport
interface.
The purpose is to make it support --remote-debugging-pipe.
@ZekeLu
make ExecAllocator use pipe connection by default
c8c5c52 
Note that pipe connection is just supported on non-windows as of
now. see golang/go#21085.
@ZekeLu
test both pipe and WebSocket connections
b5aaea2
@ZekeLu
move unit tests for ExecAllocator to stick together
e505cff 
this commit just contains format changes.
@ZekeLu
test pipe and port are enabled simultaneously
fc928a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@ZekeLu