Differentiate between a simulated device and an abstract device. Move the simulated device out of the Simulator.py.
<rdar://problem/30519212>
Created attachment 301540 [details] Patch
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.
Can you elaborate on the need to move the simctl classes to sinulated_device.py as opposed to leaving them in simulator.py?
(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.
Created attachment 302185 [details] Patch
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 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 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.
Created attachment 302290 [details] Patch
Comment on attachment 302290 [details] Patch Clearing flags on attachment: 302290 Committed r212741: <http://trac.webkit.org/changeset/212741>
All reviewed patches have been landed. Closing bug.
As an additional note, rdar://problem/30638036 will track the refactoring discussed.