Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12218091:
Android Telemetry: automates devices running "user" build. (Closed)
|
Created:
7 years, 10 months ago by bulach Modified:
7 years, 10 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, bulach+watch_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, telemetry+watch_chromium.org, ilevy+watch_chromium.org, frankf+watch_chromium.org, aberent Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid Telemetry: automates devices running "user" build.
Rather than requiring manual intervention, we can use Get/SetProtectedFileContents.
TEST=tools/telemetry/run_tests --browser=android-chrome TabConsoleTest
BUG=175127
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181943
Patch Set 1 #
Total comments: 6
Patch Set 2 : sami's comments #
Total comments: 2
Patch Set 3 : tony's comments #
Messages
Total messages: 17 (0 generated)
skyostil: aberent is out this week, hope you can help. :) marja: OWNERS review for telemetry/ thanks!
tools/telemetry/telemetry/android_browser_backend.py lgtm
https://codereview.chromium.org/12218091/diff/1/build/android/pylib/android_c... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/12218091/diff/1/build/android/pylib/android_c... build/android/pylib/android_commands.py:728: return self.RunShellCommand('su -c sh %s' % temp_script) Any reason why we can't just do 'su -c cat "%s" 2>/dev/null'? If that doesn't work, then let's rm temp_script once we're done with it. https://codereview.chromium.org/12218091/diff/1/tools/telemetry/telemetry/and... File tools/telemetry/telemetry/android_browser_backend.py (right): https://codereview.chromium.org/12218091/diff/1/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_backend.py:96: self._adb.CloseApplication(self._package) I wonder if Chrome rewrites the preferences at exit? We could have a race between that happening and reading the prefs below. If you haven't seen anything like this, feel free to ignore. https://codereview.chromium.org/12218091/diff/1/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_backend.py:107: if preferences['devtools']['remote_enabled'] != True: Nit: could be combined with the 'if' above.
thanks sami! all comments addressed, another look please? https://codereview.chromium.org/12218091/diff/1/build/android/pylib/android_c... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/12218091/diff/1/build/android/pylib/android_c... build/android/pylib/android_commands.py:728: return self.RunShellCommand('su -c sh %s' % temp_script) On 2013/02/11 14:57:13, Sami wrote: > Any reason why we can't just do 'su -c cat "%s" 2>/dev/null'? If that doesn't > work, then let's rm temp_script once we're done with it. great idea, looks better... :) fixed GetFileContents too. https://codereview.chromium.org/12218091/diff/1/tools/telemetry/telemetry/and... File tools/telemetry/telemetry/android_browser_backend.py (right): https://codereview.chromium.org/12218091/diff/1/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_backend.py:96: self._adb.CloseApplication(self._package) On 2013/02/11 14:57:13, Sami wrote: > I wonder if Chrome rewrites the preferences at exit? We could have a race > between that happening and reading the prefs below. If you haven't seen anything > like this, feel free to ignore. hmm, it probably does, but I haven't seen this being issue... afaict, the preferences are atomic, so worst case is that this would not contain the full set. but it shouldn't really matter, as this whole block would only be triggered on the very fist run.. I'll leave as is for the time being.. https://codereview.chromium.org/12218091/diff/1/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_backend.py:107: if preferences['devtools']['remote_enabled'] != True: On 2013/02/11 14:57:13, Sami wrote: > Nit: could be combined with the 'if' above. Done.
Thanks, lgtm.
lgtm Can you remove step 5 under "Setting Up Telemetry" here? http://www.chromium.org/developers/telemetry https://codereview.chromium.org/12218091/diff/5/tools/telemetry/telemetry/and... File tools/telemetry/telemetry/android_browser_backend.py (right): https://codereview.chromium.org/12218091/diff/5/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_backend.py:71: if not is_content_shell and self._adb.RunShellCommand('su -c echo') == ['']: Maybe factor out a CanAccessProtectedFileContents() method?
thanks tony! extracted into a CanAccessProtectedFileContents() function. note that this still requires a "rooted" device (which means "su" is available, not necessarily "adb root"), so rather than removing step 5, I updated to clarify that it requires either "adb root" or "adb shell su". https://codereview.chromium.org/12218091/diff/5/tools/telemetry/telemetry/and... File tools/telemetry/telemetry/android_browser_backend.py (right): https://codereview.chromium.org/12218091/diff/5/tools/telemetry/telemetry/and... tools/telemetry/telemetry/android_browser_backend.py:71: if not is_content_shell and self._adb.RunShellCommand('su -c echo') == ['']: On 2013/02/11 19:19:09, tonyg wrote: > Maybe factor out a CanAccessProtectedFileContents() method? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/12218091/3002
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/12218091/3002
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/12218091/3002
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/12218091/3002
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bulach@chromium.org/12218091/3002
Message was sent while issue was closed.
Change committed as 181943 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
