[webkit-reviews] review granted: [Bug 175390] Allow nested timeouts in webkitpy : [Attachment 317715] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 175390: Allow nested timeouts in webkitpy
https://bugs.webkit.org/show_bug.cgi?id=175390

Attachment 317715: Patch

https://bugs.webkit.org/attachment.cgi?id=317715&action=review




--- Comment #4 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 317715
  --> https://bugs.webkit.org/attachment.cgi?id=317715
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317715&action=review

r=me

Wow, this makes the code much more readable in simulated_device.py and
simulator_process.py!

> Tools/Scripts/webkitpy/common/timeout_context.py:28
> +import os
> +import math
> +import logging
> +import signal
> +import time
> +import threading

Nit: Alphabetize.

> Tools/Scripts/webkitpy/common/timeout_context.py:56
> +	       _log.critical('Using both alarms and threading in the same
process')
> +	       _log.critical('This is unsupported')

Nit: Any particular reason this can't be one log message?

> Tools/Scripts/webkitpy/common/timeout_context.py:61
> +	   # seconds=0 is invalid

Nit: Shouldn't this be:

# seconds == 0 is invalid

> Tools/Scripts/webkitpy/common/timeout_context.py:62
> +	   if seconds == 0:

Nit: If you created a helper method called secondsAreInvalid(seconds), then you
can get rid of the comment:

	if self.secondsAreInvalid(seconds):

> Tools/Scripts/webkitpy/common/timeout_context.py:63
> +	       raise RuntimeError('Cannot have a timeout of 0')

Nit:

	    raise RuntimeError('Cannot have a timeout of 0 seconds')

> Tools/Scripts/webkitpy/common/timeout_context.py:97
> +	   # Another timeout is more urgent

Nit: Comments should be sentences ending in periods.  (Please check other
comments below.)

> Tools/Scripts/webkitpy/common/timeout_context.py:113
> +	   signal.alarm(0)  # Imiediatly disable the alarm so we aren't
interupted

Typos:	Imiediatly, interupted.
Nit: Period at end of sentence.

> Tools/Scripts/webkitpy/common/timeout_context_unittest.py:36
> +    def test_current_timeout(self):
> +	   self.assertEqual(None, Timeout.current())
> +	   with Timeout(1) as tmp:
> +	       self.assertEqual(tmp.data, Timeout.current())
> +	   self.assertEqual(None, Timeout.current())

This could be racy if the with: block doesn't execute in less than 1 second,
correct?

This doesn't block landing the patch; just trying to understand how it works.

> Tools/Scripts/webkitpy/port/simulator_process.py:28
>  from webkitpy.port.server_process import ServerProcess
> -
> +from webkitpy.common.timeout_context import Timeout

Nit: Do we alphabetize by dotted path, or by class name?  Either is fine.  (I
was assuming dotted path.)

> Tools/Scripts/webkitpy/xcode/simulated_device.py:30
> +from webkitpy.common.timeout_context import Timeout
>  from webkitpy.common.system.executive import ScriptError
>  from webkitpy.xcode.simulator import Simulator
>  from webkitpy.common.host import Host

Nit:  Alphabetized by neither dotted path nor class name?