Andreas Tolfsen ❲:ato❳ Reporter
	Description • 9 years ago

We should consider cancelling or extending the connection timeout (also for newSession) when --jsdebugger is enabled as debugging sessions can take a while.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 1 • 9 years ago

I hit this last week, I have a patch for it.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 3 • 9 years ago

/r/2211 - Bug 1119211 - Disable the 360 second socket timeout when marionette is invoked with --jsdebugger.;r=ato

Pull down this commit:

hg pull review -r c40015d0be0a991cb6f5575587af4e2e565be870

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 4 • 9 years ago

https://reviewboard.mozilla.org/r/2209/#review1461

::: testing/marionette/client/marionette/runner/base.py
(Diff revision 1)
> -                 adb_host=None, adb_port=None, **kwargs):
> +                 adb_host=None, adb_port=None, socket_timeout=360.0,

Magic number 360.0 repeated

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 5 • 9 years ago

https://reviewboard.mozilla.org/r/2209/#review1463

I think this patch is fine even if it doesn't pass the timeout to Marionette.wait_for_port.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 6 • 9 years ago

https://reviewboard.mozilla.org/r/2209/#review1469

> Magic number 360.0 repeated

I think this might be necessary -- the default is provided again in marionette.py and transport.py, presumably because these are all valid entrypoints, and they all need this default.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 7 • 9 years ago

https://reviewboard.mozilla.org/r/2209/#review1471

> I think this might be necessary -- the default is provided again in marionette.py and transport.py, presumably because these are all valid entrypoints, and they all need this default.

Yes, but can we define it in a constant in this file?

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 8 • 9 years ago

Comment on attachment 8546014 [details]
MozReview Request: bz://1119211/chmanchester

/r/2211 - Bug 1119211 - Disable the 360 second socket timeout when marionette is invoked with --jsdebugger.;r=ato

Pull down this commit:

hg pull review -r c38fa63e49b397ec545d056d25843d3d134adfba

	Monty the Python Review Bot
	Comment 9 • 9 years ago

https://reviewboard.mozilla.org/r/2211/#review2679

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  testing/marionette/client/marionette/runner/base.py

::: testing/marionette/client/marionette/runner/base.py
(Diff revision 2)
> +                        help='Set the global timeout for marionette socket operations.')

E501: line too long (88 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 10 • 9 years ago

(In reply to Monty the Python Review Bot from comment #9)
> https://reviewboard.mozilla.org/r/2211/#review2679
> 
> Always look on the bright side of life.
> 
> I analyzed your Python changes and found 1 errors.
> 
> The following files were examined:
> 
>   testing/marionette/client/marionette/runner/base.py
> 
> ::: testing/marionette/client/marionette/runner/base.py
> (Diff revision 2)
> > +                        help='Set the global timeout for marionette socket operations.')
> 
> E501: line too long (88 > 79 characters)
> Limit all lines to a maximum of 79 characters.
> 
> There are still many devices around that are limited to 80 character
> lines; plus, limiting windows to 80 characters makes it possible to have
> several windows side-by-side.  The default wrapping on such devices looks
> ugly.  Therefore, please limit all lines to a maximum of 79 characters.
> For flowing long blocks of text (docstrings or comments), limiting the
> length to 72 characters is recommended.
> 
> Reports error E501.

I don't agree with this and don't intend to change it.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 11 • 9 years ago

https://reviewboard.mozilla.org/r/2211/#review2989

> E501: line too long (88 > 79 characters)
> Limit all lines to a maximum of 79 characters.
> 
> There are still many devices around that are limited to 80 character
> lines; plus, limiting windows to 80 characters makes it possible to have
> several windows side-by-side.  The default wrapping on such devices looks
> ugly.  Therefore, please limit all lines to a maximum of 79 characters.
> For flowing long blocks of text (docstrings or comments), limiting the
> length to 72 characters is recommended.
> 
> Reports error E501.

This isn't relevant, but I can't close issues because I'm not the owner )-:

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 12 • 9 years ago

https://reviewboard.mozilla.org/r/2211/#review2991

Ship It!

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 13 • 9 years ago

Comment on attachment 8546014 [details]
MozReview Request: bz://1119211/chmanchester

https://reviewboard.mozilla.org/r/2209/#review2993

Ship It!

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 14 • 9 years ago

You can't ship it enough, apparently.  God damn child/root reviews.

	Chris Manchester (limited bugmail, email directly) Assignee
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/521af16144ac

	Carsten Book [:Tomcat]
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/521af16144ac