Bug 164568 – Make it possible to use an existing simulator instance for one-off testing

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

[Jonathan Bedard]

During local testing, run-webkit-tests boots up simulators every test run.  For test automation, this is preferred.  However, for local testing, run-webkit-tests should either boot a single simulator and keep it running or use an already running simulator to run tests on.

[Radar WebKit Bug Importer]

<rdar://problem/29189133>

[Simon Fraser (smfr)]

Yes please! This will be awesome.

[Jonathan Bedard]

There appears to be a bug in LayoutTestRelay that is preventing this.

Rather than fix this bug, I will delay this patch until LayoutTestRelay has been removed.

The following bugs are tracking the removal of LayoutTestRelay:

https://bugs.webkit.org/show_bug.cgi?id=164338
https://bugs.webkit.org/show_bug.cgi?id=164344

[Jonathan Bedard]

Created attachment 294381 [details]
Beginning of patch to use running simulators

This patch is very incomplete.  Currently, there is a bug in LayoutTestRelay that causes the installed app to crash when booting.  This will likely be fixed by removing LayoutTestRelay.

[Jonathan Bedard]

Created attachment 295608 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 295608 [details]
Patch

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

I think we need to warn or restart the sim if the running sim has the wrong device type.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:300
> +        optparse.make_option('--build-simulators', action="store_true", default=False, help="If set, iOS simulators will always be built"),

--dedicated-simulators ?

[Daniel Bates]

(In reply to comment #4)
> Created attachment 294381 [details]
> Beginning of patch to use running simulators
> 
> This patch is very incomplete.  Currently, there is a bug in LayoutTestRelay
> that causes the installed app to crash when booting.  This will likely be
> fixed by removing LayoutTestRelay.

Can we fix this issue as part of this patch?

[Jonathan Bedard]

Created attachment 295616 [details]
Patch

[Jonathan Bedard]

(In reply to comment #7)
> (In reply to comment #4)
> > Created attachment 294381 [details]
> > Beginning of patch to use running simulators
> > 
> > This patch is very incomplete.  Currently, there is a bug in LayoutTestRelay
> > that causes the installed app to crash when booting.  This will likely be
> > fixed by removing LayoutTestRelay.
> 
> Can we fix this issue as part of this patch?

This was actually a different issue with the simulator name that was manifesting itself in a way that looked like a bug in LayoutTestRelay, the source of the issue was actually in the Python code.

The posted changes in ios.py fixed it.

[Daniel Bates]

Comment on attachment 295616 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:43
> +from webkitpy.xcode.simulator import Simulator

It is not appropriate to import port-specific code into this port-independent file. This module is specific to the iOS Simulator port. I further elaborate on this in my comment on the change to _set_up_derived_options() (below).

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:300
> +        optparse.make_option('--dedicated-simulators', action="store_true", default=False, help="If set, dedicated iOS simulators will always be created"),

This name of this option is better than in the previous patch as the word "dedicated" is more descriptive than the word "build"/"built" formerly used. This name is OK-as-is. For your consideration, I think a better way to describe the behavior this option is toggling is whether we will open a new simulator instance(s) or not and hence maybe a better name for this option would be --open-new-simulator-instance? --open-new-simulator-instances? (would it be confusing to support both?) In addition to Simon's remark in comment #6 to error out/relaunch the Simulator.app for device incompatibilities I suggest that we consider this option and --child-processes > 1 as mutually exclusive and error out with an informative message. We should also explain this mutual exclusivity in the usage message for this option.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:379
> +    if not options.dedicated_simulators and options.platform == "ios-simulator":
> +        sim = Simulator()
> +        if not sim.device():
> +            options.dedicated_simulators = True
> +        if not options.dedicated_simulators:
> +            options.child_processes = 1
> +

This is not the appropriate place for such port-specific code. Port-specific code should be placed in the appropriate port object (e.g. IOSSimulatorPort).

> Tools/Scripts/webkitpy/port/ios.py:279
> +        if self._managing_simulators():

Maybe a better name for this method would be _should_open_new_simulator_instances? _should_use_dedicated_simulators?

> Tools/Scripts/webkitpy/port/ios.py:283
>          for i in xrange(self.child_processes()):
>              self._create_device(i)

Keeping this logic and fixing up _create_device() to return the Simulator.app booted device seems unintuitive. I mean, we should not be creating devices when a person opts to use the Simulator.app booted device. One way to fix this is to reserve Simulator._create_device() for creating new devices and add a new Simulator method, say current_device(), that returns the Simulator.app booted device, and conditionalize calling these functions.

> Tools/Scripts/webkitpy/port/ios.py:390
>      def _create_device(self, number):
> -        return Simulator.create_device(number, self.simulator_device_type(), self.simulator_runtime)
> +        return Simulator.create_device(number, self.simulator_device_type(), self.simulator_runtime, self._managing_simulators())

This change violates the intention of the use of "create" in the name of this function to create a new simulator device using simctl because it may now return an existing simulator device. This approach of "fixing up" this function to support using an existing simulator device seems unintuitive. See my comment above in _create_simulators() for one way to make this code more intuitive.

> Tools/Scripts/webkitpy/port/ios.py:393
> +        Simulator.remove_device(number, self._managing_simulators())

Similarly, this changes violates the intention of this function. I suggest that we modify the code to never call this function when using an existing Simulator.app simulated device.

> Tools/Scripts/webkitpy/xcode/simulator.py:331
> +        if managing:
> +            device = Simulator().lookup_or_create_device(device_type.name + ' WebKit Tester' + str(number), device_type, runtime)
> +        else:
> +            device = Simulator().device()
> +            assert(device)
> +            if device_type.name != device.name:
> +                _log.error("Expected simulator of type '" + device_type.name + "' but found simulator of type '" + device.name + "'")
> +                _log.error('The next block of tests may fail due to device mis-match')

This does not seem like the appropriate place to do this logic. The caller should be responsible for such validation and error handling.

> Tools/Scripts/webkitpy/xcode/simulator.py:338
> +    def remove_device(number, managing=True):
> +        if not Simulator._managed_devices[number] or not managing:

This does not seem like the appropriate place for this logic.

> Tools/Scripts/webkitpy/xcode/simulator.py:553
> -            raise TypeError('Must supply name and/or runtime.')
> +            for device in self.devices:
> +                if device.state == 3:
> +                    return device
> +            return None

This does not seem like the appropriate place to determine the Simulator.app simulator device. See my remark in _create_simulators() about defining a new method.

Moreover this logic only find the correct simulator device if there is exactly one simulator device booted and it was the simulator device that was booted by Simulator.app. Without loss of generality, suppose a person boots a simulator device using simctl and then opens Simulator.app (which boots another simctl device), depending on the listing order of `simctl list` we may pick the simctl booted simulator device and not the Simulator.app one. Is this OK?

[Daniel Bates]

Comment on attachment 295616 [details]
Patch

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

> Tools/ChangeLog:5
> +

Please add the Radar bug URL, <rdar://problem/29189133>, under the WebKit bug URL.

> Tools/ChangeLog:9
> +        ââ-dedicated-simulatorsâ is not passed into the application, only one simulator

Pretty-print is not happy with your use of fancy quotes?

> Tools/ChangeLog:11
> +        If no simulator is running or ââ-dedicated-simulatorsâ is passed to the script,

Ditto.

> Tools/ChangeLog:26
> +        (Simulator.remove_device): If not managing simulators, donât remove a device.

Ditto.

[Jonathan Bedard]

(In reply to comment #10)
> Comment on attachment 295616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295616&action=review
> 
> ...
> We should also explain this mutual exclusivity in the
> usage message for this option.
> ...
> This is not the appropriate place for such port-specific code. Port-specific
> code should be placed in the appropriate port object (e.g. IOSSimulatorPort).
> ...
>

As a larger point with the port-dependent code and mutual exclusivity, this option is both a port-dependent and state dependent option.  Generally, all of our port dependent code is in ports.  But, generally, all of our option parsing code (for run_webkit_tests, anyways) is in run_webkit_tests.py.  Does port specific option parsing code belong with ports or run_webkit_tests?

>
> ...
> Moreover this logic only find the correct simulator device if there is
> exactly one simulator device booted and it was the simulator device that was
> booted by Simulator.app. Without loss of generality, suppose a person boots
> a simulator device using simctl and then opens Simulator.app (which boots
> another simctl device), depending on the listing order of `simctl list` we
> may pick the simctl booted simulator device and not the Simulator.app one.
> Is this OK?

Yes.  After this point, the booted simulator is referred to by it's device ID.  So it shouldn't matter.

[Jonathan Bedard]

Comment on attachment 295616 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/ios.py:390
>> +        return Simulator.create_device(number, self.simulator_device_type(), self.simulator_runtime, self._managing_simulators())
> 
> This change violates the intention of the use of "create" in the name of this function to create a new simulator device using simctl because it may now return an existing simulator device. This approach of "fixing up" this function to support using an existing simulator device seems unintuitive. See my comment above in _create_simulators() for one way to make this code more intuitive.

create_device doesn't use simctl (even though I agree, the name of the function is not intuitive)  What it actually does is assign a device with the target type to the specified index.

>> Tools/Scripts/webkitpy/port/ios.py:393
>> +        Simulator.remove_device(number, self._managing_simulators())
> 
> Similarly, this changes violates the intention of this function. I suggest that we modify the code to never call this function when using an existing Simulator.app simulated device.

remove_device removes the device from the specified index and shuts it down.  The only additional change here is that the flag prevents the device from being shut-down.

>> Tools/Scripts/webkitpy/xcode/simulator.py:331
>> +                _log.error('The next block of tests may fail due to device mis-match')
> 
> This does not seem like the appropriate place to do this logic. The caller should be responsible for such validation and error handling.

Currently, lookup_or_create_device handles this type of error when dedicated simulators are being used.

>> Tools/Scripts/webkitpy/xcode/simulator.py:338
>> +        if not Simulator._managed_devices[number] or not managing:
> 
> This does not seem like the appropriate place for this logic.

This code does have an error in it, but the error is that non-dedicated devices should be removed from the list but not shut-down.

[Jonathan Bedard]

(In reply to comment #12)
>
> ...
>

Discussed in person.  The port specific code for options will be moved into the ios port and the app will, for ease of use, default to the Simulator.app instance instead of one spawned through simctl.

[Jonathan Bedard]

Comment on attachment 295616 [details]
Patch

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

>>> Tools/Scripts/webkitpy/xcode/simulator.py:331
>>> +                _log.error('The next block of tests may fail due to device mis-match')
>> 
>> This does not seem like the appropriate place to do this logic. The caller should be responsible for such validation and error handling.
> 
> Currently, lookup_or_create_device handles this type of error when dedicated simulators are being used.

After talking in person, will split create_device into two functions, one for creating and one for binding the current booted simulator.

>>> Tools/Scripts/webkitpy/xcode/simulator.py:338
>>> +        if not Simulator._managed_devices[number] or not managing:
>> 
>> This does not seem like the appropriate place for this logic.
> 
> This code does have an error in it, but the error is that non-dedicated devices should be removed from the list but not shut-down.

After talking and looking this over, I think the best approach here is actually to eliminate the boolean flag in the function but provide the boolean flag to the device.  If a device was not created by the Simulator, then we won't kill it.

[Jonathan Bedard]

Created attachment 295648 [details]
Patch

[Jonathan Bedard]

Comment on attachment 295648 [details]
Patch

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

> Tools/Scripts/webkitpy/xcode/simulator.py:544
> +        # FIXME: No good way to determine if a device was created through simctl

A quick note here: The only discernible difference between simulators from difference simulator.apps  is name.  And this isn't a reliable difference.

[Jonathan Bedard]

Created attachment 295659 [details]
Patch

[Daniel Bates]

Comment on attachment 295659 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:328
> +        if self._using_dedicated_simulators():
> +            for i in xrange(self.child_processes()):
> +                device_udid = self._testing_device(i).udid
> +                _log.debug('testing device %s has udid %s', i, device_udid)
> +
> +                # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator
> +                self._executive.run_command([
> +                    'open', '-g', '-b', self.SIMULATOR_BUNDLE_ID + str(i),
> +                    '--args', '-CurrentDeviceUDID', device_udid])
> +
> +                if mac_os_version in ['elcapitan', 'yosemite', 'mavericks']:
> +                    time.sleep(2.5)
> +
> +            _log.info('Waiting for all iOS Simulators to finish booting.')
> +            for i in xrange(self.child_processes()):
> +                Simulator.wait_until_device_is_booted(self._testing_device(i).udid)

I would write this using an early return style:

if not self._using_dedicated_simulators():
    return

for i in xrange(self.child_processes()):
...

Using an early return style improves readability. It also avoids the need to indent the existing code and preserves the git/svn blame info for the existing code among other benefits.

> Tools/Scripts/webkitpy/port/ios.py:334
> +        if self._using_dedicated_simulators():
> +            _log.debug("_quit_ios_simulator killing all Simulator processes")
> +            # FIXME: We should kill only the Simulators we started.
> +            subprocess.call(["killall", "-9", "-m", "Simulator"])

Ditto.

> Tools/Scripts/webkitpy/port/ios.py:371
> +        if self._using_dedicated_simulators():
> +            for i in xrange(self.child_processes()):
> +                simulator_path = self.get_simulator_path(i)
> +                device_udid = self._testing_device(i).udid
> +                self._remove_device(i)
>  
> -            if not os.path.exists(simulator_path):
> -                continue
> -            try:
> -                self._executive.run_command([self.LSREGISTER_PATH, "-u", simulator_path])
> -
> -                _log.debug('rmtree %s', simulator_path)
> -                self._filesystem.rmtree(simulator_path)
> +                if not os.path.exists(simulator_path):
> +                    continue
> +                try:
> +                    self._executive.run_command([self.LSREGISTER_PATH, "-u", simulator_path])
>  
> -                logs_path = self._filesystem.join(self._filesystem.expanduser("~"), "Library/Logs/CoreSimulator/", device_udid)
> -                _log.debug('rmtree %s', logs_path)
> -                self._filesystem.rmtree(logs_path)
> +                    _log.debug('rmtree %s', simulator_path)
> +                    self._filesystem.rmtree(simulator_path)
>  
> -                saved_state_path = self._filesystem.join(self._filesystem.expanduser("~"), "Library/Saved Application State/", self.SIMULATOR_BUNDLE_ID + str(i) + ".savedState")
> -                _log.debug('rmtree %s', saved_state_path)
> -                self._filesystem.rmtree(saved_state_path)
> +                    logs_path = self._filesystem.join(self._filesystem.expanduser("~"), "Library/Logs/CoreSimulator/", device_udid)
> +                    _log.debug('rmtree %s', logs_path)
> +                    self._filesystem.rmtree(logs_path)
> +
> +                    saved_state_path = self._filesystem.join(self._filesystem.expanduser("~"), "Library/Saved Application State/", self.SIMULATOR_BUNDLE_ID + str(i) + ".savedState")
> +                    _log.debug('rmtree %s', saved_state_path)
> +                    self._filesystem.rmtree(saved_state_path)
>  
> -            except:
> -                _log.warning('Unable to remove Simulator' + str(i))
> +                except:
> +                    _log.warning('Unable to remove Simulator' + str(i))

Ditto.

> Tools/Scripts/webkitpy/port/ios.py:411
> +        if self._using_dedicated_simulators():
> +            return Simulator.device_number(number)
> +        return self.current_device

We should not be invoking this function when using self.current_device. The caller should be responsible for either using self.current_device or _testing_device() depending on whether we are using dedicated simulators or not.

> Tools/Scripts/webkitpy/xcode/simulator.py:335
> -        del Simulator._managed_devices[number]
> +        Simulator._managed_devices[number]

Is this change intentional?

> Tools/Scripts/webkitpy/xcode/simulator.py:533
> +        # FIXME: No good way to determine if a device was created through simctl

This is an indirect and imprecise way of writing that there is not a straightforward way of determining the device that was booted by Simulator.app. We should be direct and precise:

# FIXME: Find the simulator device that was booted by Simulator.app. For now, pick some booted simulator device, which
# may have been booted using the simctl command line tool.

> Tools/Scripts/webkitpy/xcode/simulator.py:536
> +            if device.state == 3:

We should use the constant DeviceState.BOOTING instead of hardcoding 3 here.

[Jonathan Bedard]

Created attachment 295672 [details]
Patch

[Daniel Bates]

Comment on attachment 295672 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:106
> +        self.current_device = Simulator().current_device()

I do not see the need for this to be a public instance variable. We should convey that it is private by prefixing its name with an underscore.

> Tools/Scripts/webkitpy/port/ios.py:112
> +        if not self.current_device:
> +            self.set_option('dedicated_simulators', True)
> +        if not self.get_option('dedicated_simulators'):
> +            if self.get_option('child_processes') > 1:
> +                _log.error('Cannot have more than one child process when using a running simulator.  Setting child_processes to 1.')
> +            self.set_option('child_processes', 1)

I'm assuming that it is OK to call set_option() from here and that it is not possible that earlier code cached the old values of these options such that one part of the python codebase thinks we have N child processes and another part of the codebase thinks we have 1 child process.

> Tools/Scripts/webkitpy/port/ios.py:301
> +                _log.error("Expected simulator of type '" + self.simulator_device_type().name + "' but found simulator of type '" + self.current_device.name + "'")
> +                _log.error('The next block of tests may fail due to device mis-match')

Does it make sense for us to continue execution if we get here? If we should continue execution please explain your reasoning? And we should use _log.warn() instead of _log.error(). If we should not continue execution then I suggest we write this "else" branch using an early-return style (i.e. if not self._using_dedicated_simulators()).

> Tools/Scripts/webkitpy/xcode/simulator.py:535
> +        device = None

This line is not needed. Please remove.

[Jonathan Bedard]

Comment on attachment 295672 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/ios.py:301
>> +                _log.error('The next block of tests may fail due to device mis-match')
> 
> Does it make sense for us to continue execution if we get here? If we should continue execution please explain your reasoning? And we should use _log.warn() instead of _log.error(). If we should not continue execution then I suggest we write this "else" branch using an early-return style (i.e. if not self._using_dedicated_simulators()).

I think we should just keep the warning.  The reason I say this (and talked with Simon about this as well) is that the device type is very picky (i.e.: iPhone 5s would be the wrong device type if the target was an iPhone 5) and most of our tests aren't dependent on device type anyways.  If an engineer runs a set of local tests which actually do require a specific device and the device type is mis-matched, the logs would have the explanation of the failure.

Given that the expected use case for this change is a developer running specifically targeted tests, I think being able to run a layout-test on a simulator with the wrong configuration is expected behavior.  It's not the only way you could get a mis-configuration, by the way, something like a rotated simulator would have the same problem.

[Jonathan Bedard]

Created attachment 295716 [details]
Patch

[Jonathan Bedard]

Created attachment 295719 [details]
Patch

[Jonathan Bedard]

Created attachment 295723 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295723 [details]
Patch

Clearing flags on attachment: 295723

Committed r209136: <http://trac.webkit.org/changeset/209136>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 165337

[Michael Catanzaro]

Unfortunately it looks like this broke a bunch of webkitpy tests, see e.g. https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20(Tests)/builds/19618/steps/webkitpy-test/logs/stdio

Looks like the macOS bots are having some other bad problem running the Python tests which might explain why it wasn't noticed.

[Michael Catanzaro]

*Unrelated* note: macOS bots are not able to run the webkitpy tests at all due to this error:

No handlers could be found for logger "webkitpy.common.system.autoinstall"
Traceback (most recent call last):
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/test/main.py", line 42, in <module>
    from webkitpy.test.runner import Runner, unit_test_name
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/test/runner.py", line 29, in <module>
    from webkitpy.common import message_pool
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/message_pool.py", line 52, in <module>
    from webkitpy.common.host import Host
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/host.py", line 37, in <module>
    from webkitpy.common.net import bugzilla, buildbot, web
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/net/bugzilla/__init__.py", line 4, in <module>
    from .bugzilla import Bugzilla
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py", line 48, in <module>
    from webkitpy.common.net.credentials import Credentials
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/net/credentials.py", line 40, in <module>
    from webkitpy.thirdparty.autoinstalled import keyring
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/thirdparty/__init__.py", line 91, in find_module
    self._install_keyring()
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/thirdparty/__init__.py", line 101, in _install_keyring
    "keyring-7.3.1/keyring")
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/thirdparty/__init__.py", line 156, in _install
    installer.install(url=url, url_subpath=url_subpath, target_name=target_name)
  File "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/common/system/autoinstall.py", line 516, in install
    raise Exception(message)
Exception: Error auto-installing the keyring package to:
 "/Volumes/Data/slave/sierra-release-tests-wk2/build/Tools/Scripts/webkitpy/thirdparty/autoinstalled/keyring"
 --> Inner message: Could not download Python modules from URL "None".
 Make sure you are connected to the internet.
 You must be connected to the internet when downloading needed modules for the first time.
 --> Inner message: <urlopen error [Errno 65] No route to host>

[Jonathan Bedard]

Actually, this is related to a problem pointed out in https://bugs.webkit.org/show_bug.cgi?id=165249.

Looking at the log you've provided, the issue is that all of our ports run the unit tests for every platform.  For the most part, these unit tests mock-up the command line which allows the tests for the Mac port to pass when being tested on other ports.

simulator.py, however, is an exception to this rule.  Mac bots didn't catch it because it doesn't fail on Mac.  Thanks for the catch, Michael!

[Jonathan Bedard]

Created attachment 296010 [details]
Patch

[Jonathan Bedard]

The change adds a try-catch around the code that calls Simulator().current_device().    This keeps port specific checks out of simulator.py and means that appropriate type-errors will be thrown on Mac and iOS ports when other simulator.py functions are called.

[Daniel Bates]

Comment on attachment 296010 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:109
> +        try:
> +            self._current_device = Simulator().current_device()
> +        except TypeError:
> +            self._current_device = None

This is not correct. Notice that Simulator takes an optional host object. We should instantiate it with the host object passed to us.

[Daniel Bates]

Comment on attachment 296010 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/ios.py:109
>> +            self._current_device = None
> 
> This is not correct. Notice that Simulator takes an optional host object. We should instantiate it with the host object passed to us.

I meant to add that if we instantiate Simulator with a host object then we do not need the try-except.

[Daniel Bates]

(In reply to comment #35)
> Comment on attachment 296010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296010&action=review
> 
> >> Tools/Scripts/webkitpy/port/ios.py:109
> >> +            self._current_device = None
> > 
> > This is not correct. Notice that Simulator takes an optional host object. We should instantiate it with the host object passed to us.
> 
> I meant to add that if we instantiate Simulator with a host object then we
> do not need the try-except.

*with the host object passed to the IOSSimulatorPort constructor

[Jonathan Bedard]

Created attachment 296020 [details]
Patch

[Daniel Bates]

Comment on attachment 296020 [details]
Patch

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

> Tools/ChangeLog:26
> +        (Simulator.refresh): Add check for empty iterator.

The description of this change is incorrect. You changed the code such that we early return when PlatformInfo.xcode_simctl_list() returns an object that evaluates to False. This takes advantage of the fact that an empty generator (e.g. (i for i in [])) evaluates to true in a boolean context, that PlatformInfo.xcode_simctl_list() returns a generator object when a real PlatformInfo object is used, and that the mock PlatformInfo.xcode_simctl_list() (defined in platforminfo_mock.py) returns None.

[Jonathan Bedard]

Created attachment 296139 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 296139 [details]
Patch

Rejecting attachment 296139 [details] from commit-queue.

New failing tests:
media/modern-media-controls/media-controller/media-controller-resize.html
Full output: http://webkit-queues.webkit.org/results/2627054

[WebKit Commit Bot]

Created attachment 296144 [details]
Archive of layout-test-results from webkit-cq-01 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jonathan Bedard]

Committed r209337: <http://trac.webkit.org/changeset/209337>