Bug 168681 – webkitpy: Move some device management from iOSSimulatorPort to iOSPort class

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

iOS device and iOS simulator ports share some functionality, such as high-level worker number-to-device mapping, test driver name and test runner processes constructor.  Move these shared elements into a base iOSPort class.

[Radar WebKit Bug Importer]

<rdar://problem/30641446>

[Jonathan Bedard]

Created attachment 302325 [details]
Patch

[Jonathan Bedard]

Created attachment 302695 [details]
Patch

[Daniel Bates]

Comment on attachment 302695 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:-64
> -    ARCHITECTURES = ['armv7', 'armv7s', 'arm64']
> -    DEFAULT_ARCHITECTURE = 'arm64'
> -    VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8', 'ios-9', 'ios-10']
> -
> -    @classmethod
> -    def determine_full_port_name(cls, host, options, port_name):
> -        if port_name == cls.port_name:
> -            iphoneos_sdk_version = host.platform.xcode_sdk_version('iphoneos')
> -            if not iphoneos_sdk_version:
> -                raise Exception("Please install the iOS SDK.")
> -            major_version_number = iphoneos_sdk_version.split('.')[0]
> -            port_name = port_name + '-' + major_version_number
> -        return port_name
> -
> -    # Despite their names, these flags do not actually get passed all the way down to webkit-build.
> -    def _build_driver_flags(self):
> -        return ['--sdk', 'iphoneos'] + (['ARCHS=%s' % self.architecture()] if self.architecture() else [])

This does not seem correct. I suspect this will break iOS EWS for device.

> Tools/Scripts/webkitpy/port/ios.py:46
> +        # Avoid creating/connecting devices just for logging the commandline.

Nit: "connecting" => "connecting to"

> Tools/Scripts/webkitpy/port/ios.py:78
> +            return self._testing_device(number)

I suggest we add a comment above this line to explain that for simulator this branch implies we are using a dedicated simulator - webkitpy will launch a new Simulator app instead of re-using an open Simulator app instance.

> Tools/Scripts/webkitpy/port/ios_simulator.py:71
> +        self._simulator_group_manager = Simulator(host)

The Simulator class was largely designed as a singleton. If we can avoid holding a local then we should.

> Tools/Scripts/webkitpy/port/ios_simulator.py:72
> +        self._device_map = self._simulator_group_manager.managed_devices

For your consideration, I suggest we define a member function called device_map in the base class and require subclasses to override it. The benefit of this approach is that it prevent an implementer from inadvertently defining a new subclass of iOSPort that does not implement a device mapping. Alternatively, we could keep the code as-is and not define self._device_map in the base class as this would cause Python to raise an exception if a subclass does not override the base class instance variable. I tend to prefer the former approach as it is self-documenting.

> Tools/Scripts/webkitpy/port/ios_simulator.py:78
> +        self._current_device = self._simulator_group_manager.current_device()

Similarly, I suggest we define a base class method that subclasses can override to return the current device. If we find there is much duplication of code in subclasses to create/manage/retrieve from the device mapping we should considering consolidating such management to its own class.

> Tools/Scripts/webkitpy/port/ios_simulator.py:202
> +            self._device_map = self._simulator_group_manager.managed_devices

Is it necessary to do this here? Can the value of self._simulator_group_manager.managed_devices change between when its computed in __init__() and here?

> Tools/Scripts/webkitpy/port/ios_simulator.py:206
> +                self._simulator_group_manager.wait_until_device_is_in_state(device_udid, Simulator.DeviceState.SHUTDOWN)
> +                self._simulator_group_manager.reset_device(device_udid)

The Simulator class was largely designed as a mix of an instance and dumping ground for utility method. I suggest we just access it as the latter here. We should clean this class up.

> Tools/Scripts/webkitpy/port/ios_simulator.py:240
> +            self._simulator_group_manager.wait_until_device_is_booted(self._testing_device(i).udid)

Ditto.

> Tools/Scripts/webkitpy/port/ios_simulator.py:317
> +    def multiple_devices(self):

Maybe using_multiple_devices would be a better name? I do not like this name. I cannot think of a better name at the moment.

> Tools/Scripts/webkitpy/port/ios_simulator.py:324
> +        self._simulator_group_manager.remove_device(number)

The Simulator class was largely designed as a mix of an instance and dumping ground for utility method. I suggest we just access it as the latter here. We should clean this class up.

[Jonathan Bedard]

Created attachment 302973 [details]
Patch

[Jonathan Bedard]

Comment on attachment 302695 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/ios_simulator.py:78
>> +        self._current_device = self._simulator_group_manager.current_device()
> 
> Similarly, I suggest we define a base class method that subclasses can override to return the current device. If we find there is much duplication of code in subclasses to create/manage/retrieve from the device mapping we should considering consolidating such management to its own class.

_current_device is actually computed, this should remain a variable, although _device_map should be a function.

[Daniel Bates]

Comment on attachment 302973 [details]
Patch

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

> Tools/ChangeLog:7
> +

If you choose to incorporate the device_id_for_worker_number to device_for_worker_number renaming into this patch then please add a remark to the ChangeLog description.

> Tools/Scripts/webkitpy/port/ios.py:41
> +    def _device_map(self):

Maybe a better name for this function would be _device_for_worker_number_map.

> Tools/Scripts/webkitpy/port/ios.py:73
> +        if number is None:
> +            raise RuntimeError('No worker number given, cannot map devices.')

From talking with Jonathan in person today, this code is not needed. An instance of SimulatorProcess, which is the only caller of this function, never should pass number := None. We should consider changing SimulatorProcess.__init__() so that the worker number argument does not have a default value to avoid confusion that may lead someone to think that device_for_worker_number() can be passed number := None. This can be done in a separate bug/patch.

> Tools/Scripts/webkitpy/port/ios_device.py:31
> +    port_name = "ios"

Nit: " => '

> Tools/Scripts/webkitpy/port/ios_simulator.py:-372
> -    # FIXME: This is only exposed so that SimulatorProcess can use it.

We should keep this comment until it is no longer true.

[Jonathan Bedard]

Created attachment 303083 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 303083 [details]
Patch for landing

Clearing flags on attachment: 303083

Committed r213236: <http://trac.webkit.org/changeset/213236>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.