Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1015313002:
Detect iOS simulator as an iOS test device. (Closed)
|
Created:
5 years, 9 months ago by bustamante Modified:
5 years, 9 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetect iOS simulator as an iOS test device.
If on a MacOS machine, iossim and Chromium.app are built than we can
launch and use them for running tests.
BUG=468565
Committed: https://crrev.com/3662aebf0def98f829abf6927558850e54d1935a
Cr-Commit-Position: refs/heads/master@{#321423}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Better indenting and a doc string #
Total comments: 1
Messages
Total messages: 14 (5 generated)
bustamante@chromium.org changed reviewers: + sullivan@chromium.org
bustamante@chromium.org changed reviewers: + sullivan@chromium.org
bustamante@chromium.org changed reviewers: + bengr@chromium.org
This is an initial CL to get the ball rolling on iOS support, please take a look when you get a chance.
https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/ios_device.py (right): https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:39: path.GetChromiumSrcDir(), 'out', build_dir, 'iossim') indent 4 https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:41: path.GetChromiumSrcDir(), 'out', build_dir, 'Chromium.app') indent 4 https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:43: # If the iOS simulator and Chromium app are present, return True Should the be outside of the for loop? Please add a comment to the method to describe what it does. https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:45: return True indent 2 https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:57: if not _IsIosDeviceAttached() and not _IsIosSimulatorAvailable(): Should this be an or?
Thanks for the review! https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/ios_device.py (right): https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:39: path.GetChromiumSrcDir(), 'out', build_dir, 'iossim') On 2015/03/19 15:41:38, bengr wrote: > indent 4 Done. https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:41: path.GetChromiumSrcDir(), 'out', build_dir, 'Chromium.app') On 2015/03/19 15:41:38, bengr wrote: > indent 4 Done. https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:43: # If the iOS simulator and Chromium app are present, return True On 2015/03/19 15:41:38, bengr wrote: > Should the be outside of the for loop? Please add a comment to the method to > describe what it does. Done for a method doc string. This should be in the for loop, as long as at least one simulator is present we can return true. Retrieving the details about the device is handled in telemetry/core/backends/chrome/ios_browser_finder.py https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:45: return True On 2015/03/19 15:41:38, bengr wrote: > indent 2 Done. https://codereview.chromium.org/1015313002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/ios_device.py:57: if not _IsIosDeviceAttached() and not _IsIosSimulatorAvailable(): On 2015/03/19 15:41:38, bengr wrote: > Should this be an or? I think this should be an "and", if either a device is present, or the simulator is available, we want to return that there is an iOS device available in line 60. For context, once this returns IOSDevice(), there's a separate module called ios_browser_finder.py that will actually handle finding the "device" and getting the details about accessing it. If this returns [] though it will be skipped.
lgtm
lgtm
The CQ bit was checked by bustamante@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1015313002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3662aebf0def98f829abf6927558850e54d1935a Cr-Commit-Position: refs/heads/master@{#321423}
Message was sent while issue was closed.
nednguyen@google.com changed reviewers: + nednguyen@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1015313002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/ios_device.py (right): https://codereview.chromium.org/1015313002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/ios_device.py:46: path.GetChromiumSrcDir(), 'out', build_dir, 'iossim') FYI, this won't work once telemetry is moved to github. :-( |
