[webkit-reviews] review granted: [Bug 185315] Check for com.apple.datamigrator before declaring simulators booted : [Attachment 339604] 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 185315: Check for com.apple.datamigrator before declaring simulators booted
https://bugs.webkit.org/show_bug.cgi?id=185315

Attachment 339604: Patch

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




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

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

r=me with three optional items to consider.

> Tools/Scripts/webkitdirs.pm:2457
> +    while (system("/bin/ps -eo pid,comm | /usr/bin/grep '$process'") == 0) {

Optional:  Hopefully 'com.apple.datamigrator' doesn't appear in the path to a
completely different binary on macOS.  :)  Could mitigate this by checking for
the process name at the end of the line:

    while (system("/bin/ps -eo pid,comm | /usr/bin/grep '" + $process + "\$'")
== 0) {

NOTE: UNTESTED.  Not sure if I'm escaping the $ at the end of the regex enough.

I don't think this has to be fixed to land the patch (since behavior would be
"obvious"--hang running Simulator app), but might be good to fix if it's easy
to do.

> Tools/Scripts/webkitdirs.pm:2487
> +    waitUntilProcessNotRunning('com.apple.datamigrator');

Optional super-nit:  Use double-quotes for strings in Perl unless there is a
reason to use single quotes.  (Python is the opposite.)

> Tools/Scripts/webkitpy/xcode/simulated_device.py:368
> +	   SimulatedDeviceManager.wait_until_data_migration_is_done(host,
deadline - time.time())

Optional:  You could state that negative timeouts are equivalent to 0 by using
a max() function here, too:

	SimulatedDeviceManager.wait_until_data_migration_is_done(host, max(0,
deadline - time.time()))