Bug 168332 - webkitpy: Refactor Device class
Summary: webkitpy: Refactor Device class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-14 13:26 PST by Jonathan Bedard
Modified: 2017-02-21 13:15 PST (History)
6 users (show)

See Also:


Attachments
Patch (63.25 KB, patch)
2017-02-14 13:30 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (63.85 KB, patch)
2017-02-20 15:48 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (63.80 KB, patch)
2017-02-21 11:34 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-02-14 13:26:46 PST
Differentiate between a simulated device and an abstract device.  Move the simulated device out of the Simulator.py.
Comment 1 Radar WebKit Bug Importer 2017-02-14 13:27:58 PST
<rdar://problem/30519212>
Comment 2 Jonathan Bedard 2017-02-14 13:30:03 PST
Created attachment 301540 [details]
Patch
Comment 3 Dean Johnson 2017-02-20 12:48:12 PST
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.
Comment 4 Daniel Bates 2017-02-20 13:49:02 PST
Can you elaborate on the need to move the simctl classes to sinulated_device.py as opposed to leaving them in simulator.py?
Comment 5 Jonathan Bedard 2017-02-20 14:22:44 PST
(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.
Comment 6 Jonathan Bedard 2017-02-20 15:48:58 PST
Created attachment 302185 [details]
Patch
Comment 7 Alexey Proskuryakov 2017-02-21 09:28:39 PST
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.
Comment 8 Alexey Proskuryakov 2017-02-21 09:32:39 PST
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?
Comment 9 Jonathan Bedard 2017-02-21 11:31:35 PST
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.
Comment 10 Jonathan Bedard 2017-02-21 11:34:22 PST
Created attachment 302290 [details]
Patch
Comment 11 WebKit Commit Bot 2017-02-21 13:05:41 PST
Comment on attachment 302290 [details]
Patch

Clearing flags on attachment: 302290

Committed r212741: <http://trac.webkit.org/changeset/212741>
Comment 12 WebKit Commit Bot 2017-02-21 13:05:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Jonathan Bedard 2017-02-21 13:15:36 PST
As an additional note, rdar://problem/30638036 will track the refactoring discussed.