Bug 168332 – webkitpy: Refactor Device class

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

[Jonathan Bedard]

Differentiate between a simulated device and an abstract device.  Move the simulated device out of the Simulator.py.

[Radar WebKit Bug Importer]

<rdar://problem/30519212>

[Jonathan Bedard]

Created attachment 301540 [details]
Patch

[Dean Johnson]

Comment on attachment 301540 [details]
Patch

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

Looks good, mostly a refactor. Update the copyright notices and if all tests pass EWS then unofficial r=me. Still need an official r=me from a WebKit reviewer though.

> Tools/Scripts/webkitpy/xcode/device.py:1
> +# Copyright (C) 2014, 2015 Apple Inc. All rights reserved.

Add 2017 here.

> Tools/Scripts/webkitpy/xcode/device.py:25
> +

Nit: Add a docstring or remove this line.

> Tools/Scripts/webkitpy/xcode/simulated_device.py:1
> +# Copyright (C) 2014, 2015 Apple Inc. All rights reserved.

Add 2017 here.

> Tools/Scripts/webkitpy/xcode/simulator.py:191
> +            Simulator.Device = SimulatedDevice

I don't like this, but talking with Jonathan in-person it sounds like the SimulatedDevice::shutdown method uses Simulator, which causes this circular dependency. We should fix this for the future, but it should not block the landing of this patch since this patch organizes the Device/Simulator class hierarchy more logically.

[Daniel Bates]

Can you elaborate on the need to move the simctl classes to sinulated_device.py as opposed to leaving them in simulator.py?

[Jonathan Bedard]

(In reply to comment #4)
> Can you elaborate on the need to move the simctl classes to
> sinulated_device.py as opposed to leaving them in simulator.py?

This is not a requirement for the code to work, but I feel that it makes the code more clear.

A SimulatedDevice is distinctly different from the Simulator class which is managing all SimulatedDevices.  The addition of on-device testing means that the Device class is relevant beyond just a UDID, which is currently how classes outside of the Simulator file interact with devices.  An impending change which is dependent on this patch moves management of Devices from iOSSimulatorPort to the iOSPort, where devices are managed as a class instead of as a UDID, and demonstrates that conceptually, the SimulatedDevice stands separate from the Simulator class.

Unfortunately, this is at odds with the current implementation of the Device class in Simulator.py, which depends on unbound methods defined in the Simulator class.  This is the reason for some of the strange imports in Simulator.py.  Eventually, the SimulatedDevice will not depend on Simulator.py and will implement some of Simulator.py's device specific unbound methods with it's own bound methods attached to an instance of a device.  Rather than cloud this refactor with those changes, I opted to make this change about the Device/SimulatedDevice relationship.

[Jonathan Bedard]

Created attachment 302185 [details]
Patch

[Alexey Proskuryakov]

Here are a couple thoughts that I have about how simulator related code could be designed nicer.

- I think that code that does "simctl list" doesn't need to be in the same class as code that manipulates simulators.
- As we move to JSON, the list would be basically unwrapped JSON, plus some code to refresh/cache/update the state.
- Manipulation that deals with individual devices should be in Device classes.

So I think that this is going in the right direction.

[Alexey Proskuryakov]

Comment on attachment 302185 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/device.py:33
> +    def launch_app(self, bundle_id, args, env=None, attempts=3):

attempts=3 seems out of place here, it's an implementation detail that would be different for subclasses.

> Tools/Scripts/webkitpy/xcode/simulated_device.py:1
> +# Copyright (C) 2014, 2015, 2016, 2017 Apple Inc. All rights reserved.

2014-2017

> Tools/Scripts/webkitpy/xcode/simulator.py:1
> +# Copyright (C) 2014, 2015, 2016, 2017 Apple Inc. All rights reserved.

2014-2017

> Tools/Scripts/webkitpy/xcode/simulator.py:185
> +    Device = None

Is this line needed?

[Jonathan Bedard]

Comment on attachment 302185 [details]
Patch

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

>> Tools/Scripts/webkitpy/xcode/simulator.py:185
>> +    Device = None
> 
> Is this line needed?

Yes, it is.  I believe it's since I test it before setting so that we don't import more than we need to.

[Jonathan Bedard]

Created attachment 302290 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 302290 [details]
Patch

Clearing flags on attachment: 302290

Committed r212741: <http://trac.webkit.org/changeset/212741>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

As an additional note, rdar://problem/30638036 will track the refactoring discussed.