[webkit-reviews] review denied: [Bug 171293] webkitpy: Teardown iOS Simulators on exit if managed Simulators are still running : [Attachment 308149] Patch

[bugzilla-daemon at webkit.org]

Aakash Jain <aakash_jain at apple.com> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 171293: webkitpy: Teardown iOS Simulators on exit if managed Simulators are
still running
https://bugs.webkit.org/show_bug.cgi?id=171293

Attachment 308149: Patch

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




--- Comment #3 from Aakash Jain <aakash_jain at apple.com> ---
Comment on attachment 308149
  --> https://bugs.webkit.org/attachment.cgi?id=308149
Patch

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

Overall looks fine. Couple of comments below.

> Tools/Scripts/webkitpy/port/ios_simulator.py:187
> +    def _teardown_managed_simulators():

Please consider adding a comment that this method will be called multiple times
in a run. or add a log.debug statement.

> Tools/Scripts/webkitpy/port/ios_simulator.py:224
> +	       atexit.register(IOSSimulatorPort._teardown_managed_simulators)

You can consider moving this register() one line above. What if
_createSimulatorApps() failed (with exception) after creating couple of
simulators. We still want the cleanup.

> Tools/Scripts/webkitpy/port/ios_simulator.py:-240
> -	       return

I would prefer to keep the check for _using_dedicated_simulators inside the
method (_quit_ios_simulator), instead of adding the responsibility to the
callers. It is very easy to miss the check for the callers.

> Tools/Scripts/webkitpy/port/ios_simulator.py:-255
> -		   pass

Please separate this change in another patch as this is unrelated to other
changes.

> Tools/Scripts/webkitpy/port/ios_simulator.py:343
> +	       # Maybe this should delete all devices that we've created?

we shouldn't need this change after reverting _quit_ios_simulator behavior.