Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
253903004:
Make iOS browsers searchable by telemetry. (Closed)
|
Created:
6 years, 7 months ago by baxley Modified:
6 years, 6 months ago CC:
chromium-reviews, telemetry+watch_chromium.org, klundberg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake iOS browsers searchable by telemetry.
Add support, via ios-webkit-debug-proxy, to find running instances
of Chrome and Safari on an iOS device.
BUG=368323
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278880
Patch Set 1 #
Total comments: 13
Patch Set 2 : Simple clean up. #
Total comments: 1
Patch Set 3 : Update per offline discussion. #Patch Set 4 : Fix blank lines #
Total comments: 27
Patch Set 5 : rebase and address comments #Patch Set 6 : cleanup #Patch Set 7 : remove unit test #
Messages
Total messages: 26 (0 generated)
Here's a first pass at implementing browser_finder on iOS. Let me know what you think of the approach. With this implementation I was able to run "list_available_browsers" and see browsers running on my device, in addition to desktop browsers. If the use of ios-webkit-debug-proxy is a bit confusing, I can add more comments or link a brief document. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:17: class PossibleIOSBrowser(possible_browser.PossibleBrowser): I left out some of the methods that should be overridden for the first pass to keep it simpler.
Nice! Looks like this is on the right track, but I have a few questions, have suggested a refactor and also this will need a unittest. See android_browser_finder_unittest.py for an example test. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:15: from urllib2 import URLError I think this belongs up on line 12. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:27: ALL_BROWSER_TYPES = IOS_BROWSERS.keys() Looks like this means the user would see CriOS and Version if they run --browser=list. I don't think that's right. We should follow the existing convention and make sure to use strings like --browser=ios-chrome and --browser=ios-safari. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:37: with contextlib.closing(urllib2.urlopen(DEVICE_LIST_URL)) as device_list: What listens on localhost:9221? Don't we need to start that process somehow? Also, ChromeBrowserBackend.ListInspectableContexts() already knows how to talk to /json to get a list of inspectable contexts. We should re-use that code. I think that entails some light refactoring. The interesting thing is that, if I'm understanding this code, it looks like an iOS device is equivalent to what Telemetry calls a Browser and that a UIWebView is equivalent to a Tab. That probably means we need a helper class called something like InspectorContextFinder. Both ChromeBrowserBackend and IosBrowserFinder would use it. I recommend landing the refactor first, then rebasing this patch. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:41: return [] Let's add a logging.debug('No iOS devices found. Will not try searching for iOS browsers.') https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:63: res = ws.recv() I think it'd be preferable to do a /json/version request to executing JS to call navigator.userAgent. See ChromeBrowserBackend._PostBrowserStartupInitialization(), which should have that functionality factored out to the InspectorContextFinder.
Thanks for the review! I had a couple of questions/answers to some of your questions. For the other questions, I agree and will make the changes once the others are resolved. In the meantime I'll also look through some other areas of the code you referenced to better understand how to do a refactor. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:37: with contextlib.closing(urllib2.urlopen(DEVICE_LIST_URL)) as device_list: On 2014/04/29 19:52:10, tonyg wrote: > What listens on localhost:9221? Don't we need to start that process somehow? Yes, ios-webkit-debug-proxy. I left it out assuming that it would be started somewhere else. Should the normal workflow be to launch ios-webkit-debug-proxy (if it exists) when running Telemetry? I am asking since I assume that most machines won't even have ios-webkit-debu-proxy installed. > > Also, ChromeBrowserBackend.ListInspectableContexts() already knows how to talk > to /json to get a list of inspectable contexts. We should re-use that code. > > I think that entails some light refactoring. The interesting thing is that, if > I'm understanding this code, it looks like an iOS device is equivalent to what > Telemetry calls a Browser and that a UIWebView is equivalent to a Tab. I'm not sure about the definitions in Telemetry, I think my naming may be a little confusing. Here's some clarification on the data from ios-webkit-debug-proxy. This block will get a different port number for each physical device (though we may only ever do it with one attached device since the underlying libraries to communicate with iOS devices don't work as well with multiple devices). Using that address we get a link to every running UIWebView, regardless of browser. From each of these I get a websocket debugger URL which I use to get the userAgent. So if I have Safari and Chrome running on a device, I see all the UIWebViews on the same page. If I use the specific links (see |data| and |debug_urls| below, then I can access the running UIWebView. >That > probably means we need a helper class called something like > InspectorContextFinder. Both ChromeBrowserBackend and IosBrowserFinder would use > it. > > I recommend landing the refactor first, then rebasing this patch. I'm going to read through the code you referenced to get a better idea of the architecture of Telemetry. But I think it's a good idea to do a refactor if it makes sense. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:41: return [] This is reached when ios-webkit-debug-proxy is not running. If it is running and there are no devices, it will still successfully do the urlopen. I'll add better error case handling once I understand how it should be implemented.
I looked at the json/version example to get the User-Agent, but couldn't figure out how to get that information via ios-webkit-debug-proxy without using the websocket. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:63: res = ws.recv() On 2014/04/29 19:52:10, tonyg wrote: > I think it'd be preferable to do a /json/version request to executing JS to call > navigator.userAgent. See > ChromeBrowserBackend._PostBrowserStartupInitialization(), which should have that > functionality factored out to the InspectorContextFinder. I haven't found a json API to get the user agent. The second json API (that gets all running webviews) gets the following: devtoolsFrontendUrl, faviconUrl, thumbnailUrl, title, url and webSocketDebuggerUrl. I was able to use the last one to get the userAgent via JS over a websocket. If I'm missing something or you have other suggestions, please let me know. I'll send a more detailed explanation of what data I'm getting from ios-webkit-debug-proxy offline.
Ping. Let me know if it'd be better to meet offline to talk about this. Thanks!
On 2014/04/30 16:29:52, baxley wrote: > Thanks for the review! > > I had a couple of questions/answers to some of your questions. > > For the other questions, I agree and will make the changes once the others are > resolved. In the meantime I'll also look through some other areas of the code > you referenced to better understand how to do a refactor. > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py > (right): > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:37: with > contextlib.closing(urllib2.urlopen(DEVICE_LIST_URL)) as device_list: > On 2014/04/29 19:52:10, tonyg wrote: > > What listens on localhost:9221? Don't we need to start that process somehow? > Yes, ios-webkit-debug-proxy. I left it out assuming that it would be started > somewhere else. Should the normal workflow be to launch ios-webkit-debug-proxy > (if it exists) when running Telemetry? I am asking since I assume that most > machines won't even have ios-webkit-debu-proxy installed. The workflow on Telemetry is designed to be: plug in a device, run Telemetry and it just works with no other steps or installation. More on that here: http://www.chromium.org/developers/telemetry/telemetry-feature-guidelines For android we check some binaries into cloud storage and download and install them on demand when running (see support_binaries.py). What if we scan the USB devices for an iPhone, and if one is detected, then we download, install and run the debug proxy. If no device is present, we can just shortcut. Alternatively, if it is fast enough we could just always start the proxy. > > > > > Also, ChromeBrowserBackend.ListInspectableContexts() already knows how to talk > > to /json to get a list of inspectable contexts. We should re-use that code. > > > > I think that entails some light refactoring. The interesting thing is that, if > > I'm understanding this code, it looks like an iOS device is equivalent to what > > Telemetry calls a Browser and that a UIWebView is equivalent to a Tab. > I'm not sure about the definitions in Telemetry, I think my naming may be a > little confusing. Here's some clarification on the data from > ios-webkit-debug-proxy. > This block will get a different port number for each physical device (though we > may only ever do it with one attached device since the underlying libraries to > communicate with iOS devices don't work as well with multiple devices). > Using that address we get a link to every running UIWebView, regardless of > browser. From each of these I get a websocket debugger URL which I use to get > the userAgent. So if I have Safari and Chrome running on a device, I see all the > UIWebViews on the same page. If I use the specific links (see |data| and > |debug_urls| below, then I can access the running UIWebView. > > > >That > > probably means we need a helper class called something like > > InspectorContextFinder. Both ChromeBrowserBackend and IosBrowserFinder would > use > > it. > > > > I recommend landing the refactor first, then rebasing this patch. > I'm going to read through the code you referenced to get a better idea of the > architecture of Telemetry. But I think it's a good idea to do a refactor if it > makes sense. > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:41: return > [] > This is reached when ios-webkit-debug-proxy is not running. If it is running and > there are no devices, it will still successfully do the urlopen. I'll add better > error case handling once I understand how it should be implemented. OK
On 2014/04/30 23:14:08, baxley wrote: > I looked at the json/version example to get the User-Agent, but couldn't figure > out how to get that information via ios-webkit-debug-proxy without using the > websocket. > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py > (right): > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:63: res = > ws.recv() > On 2014/04/29 19:52:10, tonyg wrote: > > I think it'd be preferable to do a /json/version request to executing JS to > call > > navigator.userAgent. See > > ChromeBrowserBackend._PostBrowserStartupInitialization(), which should have > that > > functionality factored out to the InspectorContextFinder. > > I haven't found a json API to get the user agent. The second json API (that gets > all running webviews) gets the following: devtoolsFrontendUrl, faviconUrl, > thumbnailUrl, title, url and webSocketDebuggerUrl. I was able to use the last > one to get the userAgent via JS over a websocket. If I'm missing something or > you have other suggestions, please let me know. Are you doing a request to /json/version like that code does? > > I'll send a more detailed explanation of what data I'm getting from > ios-webkit-debug-proxy offline.
On 2014/05/14 10:02:42, tonyg wrote: > On 2014/04/30 16:29:52, baxley wrote: > > Thanks for the review! > > > > I had a couple of questions/answers to some of your questions. > > > > For the other questions, I agree and will make the changes once the others are > > resolved. In the meantime I'll also look through some other areas of the code > > you referenced to better understand how to do a refactor. > > > > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > > File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py > > (right): > > > > > https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... > > tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:37: with > > contextlib.closing(urllib2.urlopen(DEVICE_LIST_URL)) as device_list: > > On 2014/04/29 19:52:10, tonyg wrote: > > > What listens on localhost:9221? Don't we need to start that process somehow? > > Yes, ios-webkit-debug-proxy. I left it out assuming that it would be started > > somewhere else. Should the normal workflow be to launch ios-webkit-debug-proxy > > (if it exists) when running Telemetry? I am asking since I assume that most > > machines won't even have ios-webkit-debu-proxy installed. > > The workflow on Telemetry is designed to be: plug in a device, run Telemetry and > it just works with no other steps or installation. More on that here: > http://www.chromium.org/developers/telemetry/telemetry-feature-guidelines > > For android we check some binaries into cloud storage and download and install > them on demand when running (see support_binaries.py). What if we scan the USB > devices for an iPhone, and if one is detected, then we download, install and run > the debug proxy. If no device is present, we can just shortcut. Alternatively, > if it is fast enough we could just always start the proxy. Thanks for the explanation. This part may be a little more complicated since ios-webkit-debug-proxy relies on multiple libraries to actually connect to the device. Installation hasn't been straightforward, as it often requires manual intervention as there are incompatible versions of the libraries that are used by other tools. I'd suggest not doing the installation in this CL, but just have some error handling to skip if the libraries aren't installed or devices can't be found. I'll add a TODO/bug for me to follow up and try to get a binary that can be downloaded.
On 2014/05/14 10:05:34, tonyg wrote: > On 2014/04/30 23:14:08, baxley wrote: > > I haven't found a json API to get the user agent. The second json API (that > gets > > all running webviews) gets the following: devtoolsFrontendUrl, faviconUrl, > > thumbnailUrl, title, url and webSocketDebuggerUrl. I was able to use the last > > one to get the userAgent via JS over a websocket. If I'm missing something or > > you have other suggestions, please let me know. > > Are you doing a request to /json/version like that code does? I poked around and couldn't get it to work. Here is some documentation from ios-webkit-debug-proxy where it says it provides a websocket to get information from within a tab: "Equivalent JSON-formatted APIs are provided for programmatic clients: http://localhost:9221/json to list all devices, http://localhost/9222/json to list device ":9222"'s tabs, and ws://localhost:9222/devtools/page/1 to inspect a tab. See the examples/README for example clients." I pinged Dave offline and I'll try to talk with him in case he has some advice. Thanks!
I looked at it a little more. This is still a ways from being ready to submit, but it may be a good reference to decide where everything should live. I'll do separate smaller CLs to implement it, once we have a better idea of how it should work. I was thinking of making something like ipsw_commands.py , similar to adb_commands to handle launching (and possibly installing the tools) and getting any running browsers. I left a few TODOs in the code, if this is what we want to do, I'll create bugs and associate them. -Mike https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:15: from urllib2 import URLError On 2014/04/29 19:52:10, tonyg wrote: > I think this belongs up on line 12. Done. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:27: ALL_BROWSER_TYPES = IOS_BROWSERS.keys() On 2014/04/29 19:52:10, tonyg wrote: > Looks like this means the user would see CriOS and Version if they run > --browser=list. I don't think that's right. > > We should follow the existing convention and make sure to use strings like > --browser=ios-chrome and --browser=ios-safari. Done. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:41: return [] On 2014/04/30 16:29:52, baxley wrote: > This is reached when ios-webkit-debug-proxy is not running. If it is running and > there are no devices, it will still successfully do the urlopen. I'll add better > error case handling once I understand how it should be implemented. Done. https://codereview.chromium.org/253903004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:63: res = ws.recv() On 2014/04/29 19:52:10, tonyg wrote: > I think it'd be preferable to do a /json/version request to executing JS to call > navigator.userAgent. See > ChromeBrowserBackend._PostBrowserStartupInitialization(), which should have that > functionality factored out to the InspectorContextFinder. I looked at this in a little more detail and didn't see a json API via ios-webkit-debug-proxy that takes /version. The ios-webkit-debug-proxy documentation says to use JSON APIs to list devices and tabs, but to use a websocket to inspect a tab. I was able to get the json/version information on the desktop browser, but could not do it on an iOS browser. https://codereview.chromium.org/253903004/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:35: logging.getLogger().setLevel(logging.DEBUG) Accidentally, left this in, I will remove.
Okay, I finally got around to applying some of the comments we discussed. I littered the code with TODOs. Most of these are areas that can be tackled when I implement ios_backend, and similar files (which is the next step). I had started the refactor and it got way too big and complicated for one CL, so I put it back into ios_browser_finder.py. I think it would be simpler to just get it working, then I can refactor things in smaller pieces. If any of them can simply be refactored (i.e. don't require creating a new class), let me know and I will change it in this CL. I also added a unit test file, which currently requires an iOS device attached. I'll stub it out later so it runs everywhere. Here is what is required to run the browser finder and find an iOS browser with this implementation. 1. ios_webkit_debug_proxy needs to be running or installed and included in the PATH. 2. It currently only runs on a Mac host. 3. The attached device must be unlocked. If these conditions are not met, it will still run, it just won't find any browsers. I plan to handle these cases in subsequent CLs. Again, sorry for the delay, any feedback is welcome. -Mike
Mostly nits at this point. Looking good! https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:24: super(PossibleIOSBrowser, self).__init__(browser_type, 'iOS', nit: lowercase ios? https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:59: def LaunchIosWebkitDebugProxy(logging=real_logging): nit: Remove logging, just use the standard logging module everywhere. android_browser_finder is just being weird. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:67: logging.debug('Failed to launch %s, make sure it is installed and in your' nit: warning level? https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:73: browsers = [] nit: Define this alll the way at the end. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:89: 'iOS devices.') nit: This is going to print on Win/Linux out of context. Probably just don't log this. We can print an error if the user specifically does --browser-ios-chrafari https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:122: # ready, if ios_webkit_debug_proxy is just launched. Can we use telemetry.core.util.WaitFor for this? https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:148: # into an appropriate backend file? InspectorBackend is the full-fledged class for communicating with a UIWebView/tab/extension that has EvaluateJavaScript. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py (right): https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. nit: 2014 https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py:11: # dependencies. We use discovery to find unit tests, so this will run automatically on all platforms. We need to skip it if we don't have a device.
Just a few comments from my end that I think should be addressed before landing. Once Dave is happy, feel free to land (I'll be out most of next week). https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:46: def IsIosWebkitDebugProxyRunning(): Replace this method with a call to: platform.GetHostPlatform().IsApplicationRunning(IOS_WEBKIT_DEBUG_PROXY) Hopefully, that'll just work. If not, we should fix up the platform code first. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:59: def LaunchIosWebkitDebugProxy(logging=real_logging): And this one with: platform.GetHostPlatform().LaunchApplication(IOS_WEBKIT_DEBUG_PROXY) https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:75: if sys.platform == 'darwin': platform.GetHostPlatform().GetOSName() == 'mac': https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:117: # to GetAttachedDevices() in adb_commands. This sounds really cool. Have you talked to jbudurick about this? He's doing a lot of refactoring in the android commands class and it would be really cool to have an ios library with the same API. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:148: # into an appropriate backend file? On 2014/06/14 00:03:58, dtu wrote: > InspectorBackend is the full-fledged class for communicating with a > UIWebView/tab/extension that has EvaluateJavaScript. I agree we need to address this part. Even in the initial version, we shouldn't duplication devtools commands that we already know how to speak. Let's make sure we're at minimum reusing an InspectorBackend. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py (right): https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py:11: # dependencies. On 2014/06/14 00:03:58, dtu wrote: > We use discovery to find unit tests, so this will run automatically on all > platforms. We need to skip it if we don't have a device. Recommend just adding: @test.Enabled('ios')
PTAL! I addressed the comments. My main outstanding question is about the unit tests and if they're worth it (right now), given they have restrictions to run. But if the 'ios' filter means no bots will run it, it should be okay. Let me know what you think. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:24: super(PossibleIOSBrowser, self).__init__(browser_type, 'iOS', On 2014/06/14 00:03:58, dtu wrote: > nit: lowercase ios? Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:46: def IsIosWebkitDebugProxyRunning(): On 2014/06/14 00:31:11, tonyg wrote: > Replace this method with a call to: > > platform.GetHostPlatform().IsApplicationRunning(IOS_WEBKIT_DEBUG_PROXY) > > Hopefully, that'll just work. If not, we should fix up the platform code first. Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:59: def LaunchIosWebkitDebugProxy(logging=real_logging): On 2014/06/14 00:03:58, dtu wrote: > nit: Remove logging, just use the standard logging module everywhere. > android_browser_finder is just being weird. Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:67: logging.debug('Failed to launch %s, make sure it is installed and in your' On 2014/06/14 00:03:58, dtu wrote: > nit: warning level? Removed this method altogether. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:73: browsers = [] On 2014/06/14 00:03:58, dtu wrote: > nit: Define this alll the way at the end. Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:75: if sys.platform == 'darwin': On 2014/06/14 00:31:11, tonyg wrote: > platform.GetHostPlatform().GetOSName() == 'mac': Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:89: 'iOS devices.') On 2014/06/14 00:03:58, dtu wrote: > nit: This is going to print on Win/Linux out of context. Probably just don't log > this. We can print an error if the user specifically does --browser-ios-chrafari Done. This should eventually work on Linux, so I left the TODO in, but took out the log statement. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:117: # to GetAttachedDevices() in adb_commands. On 2014/06/14 00:31:12, tonyg wrote: > This sounds really cool. Have you talked to jbudurick about this? He's doing a > lot of refactoring in the android commands class and it would be really cool to > have an ios library with the same API. Yeah, I talked to him briefly. When I get to the backend part then I'll work more closely to see if we can share an API. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:122: # ready, if ios_webkit_debug_proxy is just launched. On 2014/06/14 00:03:58, dtu wrote: > Can we use telemetry.core.util.WaitFor for this? Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:148: # into an appropriate backend file? On 2014/06/14 00:31:11, tonyg wrote: > On 2014/06/14 00:03:58, dtu wrote: > > InspectorBackend is the full-fledged class for communicating with a > > UIWebView/tab/extension that has EvaluateJavaScript. > > I agree we need to address this part. Even in the initial version, we shouldn't > duplication devtools commands that we already know how to speak. Let's make sure > we're at minimum reusing an InspectorBackend. I had trouble on this part since the InspectorBackend requires a BrowserBackend (which doesn't yet exist for iOS). I chatted with Dave where he pointed out that it isn't used that much in InspectorBackend, so I was able to just pass in None, along with the webDebuggerUrl/id in the context. This does cause a problem if there's an exception on connect as the except statement uses the backend. I'll implement ios_browser_backend.py next so it should be solved soon. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py (right): https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/06/14 00:03:58, dtu wrote: > nit: 2014 Done. https://codereview.chromium.org/253903004/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder_unittest.py:11: # dependencies. On 2014/06/14 00:31:12, tonyg wrote: > On 2014/06/14 00:03:58, dtu wrote: > > We use discovery to find unit tests, so this will run automatically on all > > platforms. We need to skip it if we don't have a device. > > Recommend just adding: > @test.Enabled('ios') I made the change. Will 'ios' work? Do I need set this value somewhere? and will it not run on any platform? Otherwise I can leave out the unit test until I implement the browser backend and create a stub.
lgtm
The CQ bit was checked by baxley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baxley@chromium.org/253903004/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by baxley@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baxley@chromium.org/253903004/120001
I removed the unit test since they were running with the test.Enabled('ios') on
other platforms. I figure it's safe to remove it since we don't expect any bots
to support this yet.
I'll create a new patch with the unit test, so we can discuss how it should be
handled.
On 2014/06/20 20:11:55, baxley wrote:
> I removed the unit test since they were running with the test.Enabled('ios')
on
> other platforms. I figure it's safe to remove it since we don't expect any
bots
> to support this yet.
>
> I'll create a new patch with the unit test, so we can discuss how it should be
> handled.
sg, but we should really debug why ios tests would try to run elsewhere. It
shouldn't be the case.
Message was sent while issue was closed.
Change committed as 278880 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
