[webkit-reviews] review denied: [Bug 173775] Replace --runtime with something for both ios-simulator and ios-device : [Attachment 314772] 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 173775: Replace --runtime with something for both ios-simulator and
ios-device
https://bugs.webkit.org/show_bug.cgi?id=173775

Attachment 314772: Patch

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




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

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:298
>	   optparse.make_option('--runtime', help='iOS Simulator runtime
identifier (default: latest runtime)'),

do we want to allow passing both --runtime and --version? Is the plan to remove
--runtime entirely?

> Tools/Scripts/webkitpy/port/ios.py:111
> +	   if version_identifier:

you should return early if version_identifier is not specified, something like:

if not version_identifier:
    return None

> Tools/Scripts/webkitpy/port/ios.py:112
> +	       split_by_period = version_identifier.split('.')

It would be a good idea to add a comment indicating sample version_identifier.
It would make it easy for reader to understand why/how are we splitting and
parsing the data.

> Tools/Scripts/webkitpy/port/ios.py:118
> +	       return version_identifier

seems like all you are doing here is verifying whether version_identifier is
valid or not. It would be a good idea to separate it out in another function,
something like is_valid_ios_version()

> Tools/Scripts/webkitpy/port/ios_device.py:102
>	   if len(self._device_for_worker_number_map()) == 0:

I know this is not part of this patch. But maybe you can simplify it to in this
patch:

if not self._device_for_worker_number_map():

This would comply with webkit guideline:
https://webkit.org/code-style-guidelines/#null-false-and-zero