Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
50493010:
Fix starting Telemetry tests on Android with profile (Closed)
|
Created:
7 years, 1 month ago by aberent Modified:
7 years, 1 month ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org, jeremy, Dominik Grewe Base URL:
https://chromium.googlesource.com/chromium/src.git@startupurl Visibility:
Public. |
DescriptionFix starting Telemetry tests on Android with profile
To match desktop behaviour; when asked to save or restore a profile save or restore the whole of the browser's data directory, except for the lib subdirectory.
BUG=313184
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235212
Patch Set 1 #
Total comments: 2
Patch Set 2 : Save and restore all Chrome data as profile #Patch Set 3 : Use PushIfNeeded to speed up pushing profile. #
Total comments: 10
Patch Set 4 : Respond to tonyg's comments #
Total comments: 1
Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/50493010/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/50493010/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:286: if not self.browser_options.profile_dir: as noted in my email i think the more appriate way to do this is to formalize the concept behidn this flag... its nothign to do with profile dir and by conflating the two you're adding coupling between what previously was independent features.
https://codereview.chromium.org/50493010/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/50493010/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:286: if not self.browser_options.profile_dir: On 2013/10/31 12:26:32, nduca wrote: > as noted in my email i think the more appriate way to do this is to formalize > the concept behidn this flag... its nothign to do with profile dir and by > conflating the two you're adding coupling between what previously was > independent features. Yes, as far as I can see all this does is clear the tab state. The confusion is that it seems that on the desktop the tab state, and I think all the session data, is part of the profile so clearing the profile clears the tab state, and setting a profile sets the tab state. If we want the tests to work the same on Android and desktop we need the tests to save and restore the complete session state, and not just the profile. As such maybe the name of the profile_dir option is wrong.
On 2013/11/01 18:48:30, aberent wrote: > https://codereview.chromium.org/50493010/diff/1/tools/telemetry/telemetry/cor... > File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py > (right): > > https://codereview.chromium.org/50493010/diff/1/tools/telemetry/telemetry/cor... > tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:286: > if not self.browser_options.profile_dir: > On 2013/10/31 12:26:32, nduca wrote: > > as noted in my email i think the more appriate way to do this is to formalize > > the concept behidn this flag... its nothign to do with profile dir and by > > conflating the two you're adding coupling between what previously was > > independent features. > Yes, as far as I can see all this does is clear the tab state. The confusion is > that it seems that on the desktop the tab state, and I think all the session > data, is part of the profile so clearing the profile clears the tab state, and > setting a profile sets the tab state. > > If we want the tests to work the same on Android and desktop we need the tests > to save and restore the complete session state, and not just the profile. As > such maybe the name of the profile_dir option is wrong. nduca@, could you take another look, and lgtm if you are happy.
https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (left): https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:282: args.append('--no-restore-state') Do we always clear the profile now? Or do we still need this when not running with an explicit profile? https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:39: 'su -c rm -r "%s/%s"' % (self.profile_dir, f)) How many times do we end up running this command? Since each RunShellCommand() carries with it some overhead, should we consider concatenating all the args to rm into a single command to run once? https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:87: self.profile_dir)) nit: indent https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:97: self.adb.RunShellCommand('chown %s.%s "%s"' % (uid, uid, path)) Again, possible to chown multiple files at once? https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:103: def profile_dir(self): Can this method be moved to the base class now?
https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (left): https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:282: args.append('--no-restore-state') On 2013/11/11 20:21:54, tonyg wrote: > Do we always clear the profile now? Or do we still need this when not running > with an explicit profile? Yes, we always now clear the profile. We always tried to, but we needed this because the part of the profile we cleared didn't include the tab state (or the caches etc.). https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:39: 'su -c rm -r "%s/%s"' % (self.profile_dir, f)) On 2013/11/11 20:21:54, tonyg wrote: > How many times do we end up running this command? Since each RunShellCommand() > carries with it some overhead, should we consider concatenating all the args to > rm into a single command to run once? Done. It is only run 6 times, but it is probably worth doing. https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:87: self.profile_dir)) On 2013/11/11 20:21:54, tonyg wrote: > nit: indent Done. https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:97: self.adb.RunShellCommand('chown %s.%s "%s"' % (uid, uid, path)) On 2013/11/11 20:21:54, tonyg wrote: > Again, possible to chown multiple files at once? Done. https://codereview.chromium.org/50493010/diff/110001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:103: def profile_dir(self): On 2013/11/11 20:21:54, tonyg wrote: > Can this method be moved to the base class now? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/50493010/170001
Message was sent while issue was closed.
Change committed as 235212
Message was sent while issue was closed.
https://codereview.chromium.org/50493010/diff/170001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/50493010/diff/170001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:34: files = self.adb.RunShellCommand('su -c ls "%s"' % self.profile_dir) is there a reason you don't use 'pm clear'? |
