Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
656383002:
[Telemetry] Move some implementations from android_browser_backend_to android_platform_backend (Closed)
DescriptionCode refactoring by moving implementations of the following functionalities from android_browser_backend to android_platform_backend:
setting SSL relax flag
application profile manipulation
setting debug app
kill app
port forwarding
dumping stack trace and standard output
BUG=417542
Committed: https://crrev.com/6dd5949ca6ad88981576aa98aed22e8f4590c8aa
Cr-Commit-Position: refs/heads/master@{#301668}
Committed: https://crrev.com/857bd35e8d5253994d50b4916bf1e766ef0f4330
Cr-Commit-Position: refs/heads/master@{#301899}
Patch Set 1 #
Total comments: 3
Patch Set 2 : update #
Total comments: 14
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : fixed some comments #
Total comments: 3
Patch Set 7 : #Patch Set 8 : removing test cert related code #
Total comments: 2
Patch Set 9 : #
Messages
Total messages: 41 (11 generated)
wuhu@google.com changed reviewers: + nednguyen@google.com, slamm@google.com
nednguyen@google.com changed reviewers: + dtu@chromium.org
With this refactoring, can you add documentation for those functions? Style guide in: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments https://codereview.chromium.org/656383002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/656383002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_platform_backend.py:374: def IsAppRunning(self, package): The parameter should be process_name instead of package https://codereview.chromium.org/656383002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_platform_backend.py:451: def RemoveProfile(self, package, ignore_list): Does this notion of Profile apply for general hybrid app?
https://codereview.chromium.org/656383002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/656383002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_platform_backend.py:451: def RemoveProfile(self, package, ignore_list): On 2014/10/17 03:34:31, nednguyen wrote: > Does this notion of Profile apply for general hybrid app? Yes, it applies to general android app.
slamm@chromium.org changed reviewers: + slamm@chromium.org
I am excited to see this change. This will be a nice clean-up. I have included a bunch of style nits in my comments. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:335: self._android_platform_backend.RemoveTestCa() Would it make more sense to remove the test CA after killing the browser? https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:375: # or we are failing to dismiss. Make a doc string. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:408: 'Will fallback to ignoring certificate errors. ') No trailing space. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:428: """ These triple quotes can go on the previous line. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:455: Remove the blank line. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:470: uid = re.search('\d+', id_line).group() It's good practice to use regex literals (changes how backslashes are treated). r'\d+' https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:474: paths = ['%s/%s' % (profile_dir, f) for f in files] Looks like this makes a string like "/PROFILE_DIR//file" (See _GetProfileDIR). https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:491: paths = [ '"%s/%s"' % (profile_dir, f) for f in files No space after the opening square bracket. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:540: """ Put triple quotes on the previous line. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:553: return title + '\n' + content + '\n' + '*' * 80 + '\n' A format string would probably work better: return "%s\n%s\n%s\n" % (title, content, '*' * 80)
Thanks Steve for catching those. I've made the corrections except for the two noted below. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:335: self._android_platform_backend.RemoveTestCa() On 2014/10/17 18:31:05, slamm wrote: > Would it make more sense to remove the test CA after killing the browser? This is to accommodate applications that restart themselves immediately after being killed (eg. agsa). https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:470: uid = re.search('\d+', id_line).group() On 2014/10/17 18:31:05, slamm wrote: > It's good practice to use regex literals (changes how backslashes are treated). > > r'\d+' Here aren't we are matching against list of digits instead of the literal string?
lgtm https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:335: self._android_platform_backend.RemoveTestCa() On 2014/10/17 18:53:39, wuhu wrote: > On 2014/10/17 18:31:05, slamm wrote: > > Would it make more sense to remove the test CA after killing the browser? > > This is to accommodate applications that restart themselves immediately after > being killed (eg. agsa). Acknowledged. https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:470: uid = re.search('\d+', id_line).group() On 2014/10/17 18:53:39, wuhu wrote: > On 2014/10/17 18:31:05, slamm wrote: > > It's good practice to use regex literals (changes how backslashes are > treated). > > > > r'\d+' > > Here aren't we are matching against list of digits instead of the literal > string? The "r" means raw string. It is a way to avoid special treatment of backslashes. '\n' is a newline. r'\n' is two characters (a backslash followed by the letter n). It so happens that '\d' is not a special character, so python treats it as two characters. A few special reg ex sequences are problematic: '\b' (backspace) vs. r'\b' (word boundry) '\1' (ascii 1) vs. r'\1' (first matching group) It does not matter here, however, it is a quirk of Python that is worth knowing.
This is sexy. Lgtm Please wait for tony's feedback also. On 2014/10/17 20:11:44, slamm wrote: > lgtm > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py > (right): > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:335: > self._android_platform_backend.RemoveTestCa() > On 2014/10/17 18:53:39, wuhu wrote: > > On 2014/10/17 18:31:05, slamm wrote: > > > Would it make more sense to remove the test CA after killing the browser? > > > > This is to accommodate applications that restart themselves immediately after > > being killed (eg. agsa). > > Acknowledged. > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/platform/android_platform_backend.py > (right): > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/platform/android_platform_backend.py:470: uid = > re.search('\d+', id_line).group() > On 2014/10/17 18:53:39, wuhu wrote: > > On 2014/10/17 18:31:05, slamm wrote: > > > It's good practice to use regex literals (changes how backslashes are > > treated). > > > > > > r'\d+' > > > > Here aren't we are matching against list of digits instead of the literal > > string? > > The "r" means raw string. It is a way to avoid special treatment of backslashes. > > '\n' is a newline. > r'\n' is two characters (a backslash followed by the letter n). > > It so happens that '\d' is not a special character, so python treats it as two > characters. A few special reg ex sequences are problematic: > '\b' (backspace) vs. r'\b' (word boundry) > '\1' (ascii 1) vs. r'\1' (first matching group) > > It does not matter here, however, it is a quirk of Python that is worth knowing.
nednguyen@google.com changed reviewers: + tonyg@chromium.org
https://codereview.chromium.org/656383002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/656383002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:538: def GetStandardOutput(self): Nits: let make number_of_lines a param default to 500. https://codereview.chromium.org/656383002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_platform_backend.py:549: target_arch: Device architecture (eg. arm, arm64, mips, x86, x86_64) Please add type info
On 2014/10/17 20:11:44, slamm wrote: > lgtm > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py > (right): > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:335: > self._android_platform_backend.RemoveTestCa() > On 2014/10/17 18:53:39, wuhu wrote: > > On 2014/10/17 18:31:05, slamm wrote: > > > Would it make more sense to remove the test CA after killing the browser? > > > > This is to accommodate applications that restart themselves immediately after > > being killed (eg. agsa). > > Acknowledged. > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/platform/android_platform_backend.py > (right): > > https://codereview.chromium.org/656383002/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/platform/android_platform_backend.py:470: uid = > re.search('\d+', id_line).group() > On 2014/10/17 18:53:39, wuhu wrote: > > On 2014/10/17 18:31:05, slamm wrote: > > > It's good practice to use regex literals (changes how backslashes are > > treated). > > > > > > r'\d+' > > > > Here aren't we are matching against list of digits instead of the literal > > string? > > The "r" means raw string. It is a way to avoid special treatment of backslashes. > > '\n' is a newline. > r'\n' is two characters (a backslash followed by the letter n). > > It so happens that '\d' is not a special character, so python treats it as two > characters. A few special reg ex sequences are problematic: > '\b' (backspace) vs. r'\b' (word boundry) > '\1' (ascii 1) vs. r'\1' (first matching group) > > It does not matter here, however, it is a quirk of Python that is worth knowing. Thanks for the clear explanation :)
My mental model of platform_backends is that they should only exist to implement the platform their associated with and not really be used directly. Are we sure we want to tighten the coupling here? I thought rather, we were going to move into the direction of pulling an Application super out of Browser. Then it'd be the one that'd have methods for doing things like clearing/pushing an application's profile directory. Please correct me if I've forgotten some discussions/bugs/docs about this. Dave, do you have opinions?
I think it doesn't make sense that android_browser_backend is coupled with any platforn other than Android, so this coupling is ok. On Oct 17, 2014 5:42 PM, <tonyg@chromium.org> wrote: > My mental model of platform_backends is that they should only exist to > implement > the platform their associated with and not really be used directly. Are we > sure > we want to tighten the coupling here? I thought rather, we were going to > move > into the direction of pulling an Application super out of Browser. Then > it'd be > the one that'd have methods for doing things like clearing/pushing an > application's profile directory. Please correct me if I've forgotten some > discussions/bugs/docs about this. > > Dave, do you have opinions? > > https://codereview.chromium.org/656383002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by wuhu@google.com
The CQ bit was unchecked by wuhu@google.com
On 2014/10/17 22:29:19, chromium-reviews wrote: > I think it doesn't make sense that android_browser_backend is coupled with > any platforn other than Android, so this coupling is ok. > On Oct 17, 2014 5:42 PM, <mailto:tonyg@chromium.org> wrote: > > > My mental model of platform_backends is that they should only exist to > > implement > > the platform their associated with and not really be used directly. Are we > > sure > > we want to tighten the coupling here? I thought rather, we were going to > > move > > into the direction of pulling an Application super out of Browser. Then > > it'd be > > the one that'd have methods for doing things like clearing/pushing an > > application's profile directory. Please correct me if I've forgotten some > > discussions/bugs/docs about this. > > > > Dave, do you have opinions? > > > > https://codereview.chromium.org/656383002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. More rationale for this patch: I agree that the right model should be that platform_backend is only used to implement the abstraction in platform and it's a bug that we have different browser_backend implementations based on the platform type. However, it would be another big refactoring, while we have so much work to do to accomplish the baseline of Telemetry v2 already. Furthermore, this patch should also make us one step closer to the right direction by refactor all the android specific logic to android_platform_backend.
nednguyen@google.com changed reviewers: + nduca@chromium.org
On 2014/10/21 17:05:22, nednguyen wrote: PTAL since this is blocking Hubert.
On 2014/10/20 21:06:28, nednguyen wrote: > On 2014/10/17 22:29:19, chromium-reviews wrote: > > I think it doesn't make sense that android_browser_backend is coupled with > > any platforn other than Android, so this coupling is ok. > > On Oct 17, 2014 5:42 PM, <mailto:tonyg@chromium.org> wrote: > > > > > My mental model of platform_backends is that they should only exist to > > > implement > > > the platform their associated with and not really be used directly. Are we > > > sure > > > we want to tighten the coupling here? I thought rather, we were going to > > > move > > > into the direction of pulling an Application super out of Browser. Then > > > it'd be > > > the one that'd have methods for doing things like clearing/pushing an > > > application's profile directory. Please correct me if I've forgotten some > > > discussions/bugs/docs about this. > > > > > > Dave, do you have opinions? > > > > > > https://codereview.chromium.org/656383002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > More rationale for this patch: > > I agree that the right model should be that platform_backend is only used to > implement the abstraction in platform and it's a bug that we have different > browser_backend implementations based on the platform type. However, it would be > another big refactoring, while we have so much work to do to accomplish the > baseline of Telemetry v2 already. > > Furthermore, this patch should also make us one step closer to the right > direction by refactor all the android specific logic to > android_platform_backend. There're two possibilities for what PlatformBackend is. 1. Platform is the interface outside of telemetry.core, for Measurement and Metric authors, while PlatformBackend is the interface within core, and AndroidPlatformBackend is the implementation of PlatformBackend. 2. Platform is the interface for everyone, and PlatformBackend is the implementation of that interface. I think #1 is closer to what's going to end up happening? But I don't have a strong opinion either way.
https://codereview.chromium.org/656383002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/656383002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:167: self._saved_sslflag = self._platform_backend.SetRelaxSslCheck('yes') _android_platform_backend https://codereview.chromium.org/656383002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:362: return self._android_platform_backend.wpr_ca_cert_path Is the plan to move these callers to use the PlatformBackend directly instead of plumbing the calls through the BrowserBackend?
https://codereview.chromium.org/656383002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/656383002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:362: return self._android_platform_backend.wpr_ca_cert_path On 2014/10/27 21:27:17, dtu wrote: > Is the plan to move these callers to use the PlatformBackend directly instead of > plumbing the calls through the BrowserBackend? Yes, replay and its browser backend_dependencies are moving to the platform_backend. I have a CL taking a step in that direction: https://codereview.chromium.org/656303003/
The CQ bit was checked by wuhu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/656383002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6dd5949ca6ad88981576aa98aed22e8f4590c8aa Cr-Commit-Position: refs/heads/master@{#301668}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/678413004/ by wuhu@google.com. The reason for reverting is: Unable to install test certificate authority on device. Will fallback to ignoring certificate errors. Traceback (most recent call last): InstallTestCa at tools/telemetry/telemetry/core/platform/android_platform_backend.py:420 self._cert_util.install_cert(True) install_cert at third_party/webpagereplay/adb_install_cert.py:143 raise CertInstallError('Cert Install Failed') CertInstallError: Cert Install Failed.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/683313002/ by wuhu@google.com. The reason for reverting is: 140688544552608:error:02001002:system library:fopen:No such file or directory:bss_file.c:398:fopen('/tmp/tmpLJeMcv/testca.pem','r') 140688544552608:error:20074002:BIO routines:FILE_CTRL:system lib:bss_file.c:400: unable to load certificate.
Message was sent while issue was closed.
Ned, I've removed code related to cert, please let me know if it looks good. Thanks.
LGTM Please make sure that you run most benchmarks on local before checking the CQ https://codereview.chromium.org/656383002/diff/140001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/656383002/diff/140001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:32: self.relax_ssl_check = relax_ssl_check Please keep the comment "# Don't delete lib, since it is created by the installer." https://codereview.chromium.org/656383002/diff/140001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:43: if not self._profile_ignore_list: Why not just return ['lib'] here? We don't have any backend setting that override profile_ignore_list, do we?
The CQ bit was checked by wuhu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/656383002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wuhu@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/656383002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/857bd35e8d5253994d50b4916bf1e766ef0f4330 Cr-Commit-Position: refs/heads/master@{#301899} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
