Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1213423003:
Remove adb_commands from android_browser_backend and android_browser_backend_settings. (Closed)
DescriptionRemove adb_commands from android_browser_backend and android_browser_backend_settings.
Pulled from https://codereview.chromium.org/1167173002/
BUG=476709
Committed: https://crrev.com/2e706aa8db5d0cfd09573ad56903868c4d8cd4ee
Cr-Commit-Position: refs/heads/master@{#338229}
Patch Set 1 #
Total comments: 6
Patch Set 2 : fixed root checks #Patch Set 3 : Add android_browser_backend_settings.py #Patch Set 4 : Merge android_platform_backend changes into this CL #Patch Set 5 : added unit test #Patch Set 6 : Merge with https://codereview.chromium.org/1212813007/ #Patch Set 7 : Fix typo referencing invalid _backend_settings member of WebviewShellBackendSettings #
Messages
Total messages: 33 (12 generated)
The CQ bit was checked by sullivan@chromium.org to run a CQ dry run
sullivan@chromium.org changed reviewers: + jbudorick@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213423003/1
https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:57: if self.device.old_interface.CanAccessProtectedFileContents(): hm, looks like I missed this one. This should be: if self.device.HasRoot() or self.device.NeedsSU(): (android_platform_backend does the same thing: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:110: remote_devtools_port = self._backend_settings.GetDevtoolsRemotePort( I think we'll have to convert android_browser_backend_settings before landing this. https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:119: if not self.device.old_interface.CanAccessProtectedFileContents(): Same as above.
https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... File tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:57: if self.device.old_interface.CanAccessProtectedFileContents(): On 2015/06/30 19:31:36, jbudorick wrote: > hm, looks like I missed this one. This should be: > > if self.device.HasRoot() or self.device.NeedsSU(): > > (android_platform_backend does the same thing: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) Done. https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:110: remote_devtools_port = self._backend_settings.GetDevtoolsRemotePort( On 2015/06/30 19:31:36, jbudorick wrote: > I think we'll have to convert android_browser_backend_settings before landing > this. I'm going to add it to this CL, since it's dependent. https://codereview.chromium.org/1213423003/diff/1/tools/telemetry/telemetry/i... tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:119: if not self.device.old_interface.CanAccessProtectedFileContents(): On 2015/06/30 19:31:36, jbudorick wrote: > Same as above. Done.
The CQ bit was checked by sullivan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213423003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm pending https://codereview.chromium.org/1219663006
The CQ bit was checked by sullivan@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1213423003/#ps80001 (title: "added unit test")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213423003/80001
sullivan@chromium.org changed reviewers: + nednguyen@chromium.org
+Ned This CL and https://codereview.chromium.org/1219663006 are pretty interdependent. That CL changes android_platform_backend to have a "device" property instead of an "adb" property. This CL changes android_browser_backend to set its "device" property to the one in android_platform_backend; it currently has an "adb" property from android_platform_backend. So, as John said in the review comment, that CL would need to be submitted before this one. However, that CL can't be submitted as-is because the "adb" property on android_browser_backend can't be set as that CL removes it from android_platform_backend. I'd prefer to solve this problem by combining the two CLs, so it's clear what is happening to these properties. Alternatively, I could modify the other CL to have *both* an adb and device property on android_platform_backend, and this one to remove the adb property. But that leaves android_platform_backend in a confusing state in the meantime. Ned, John, are you okay with this CL that merges the two?
On 2015/07/01 at 15:28:29, sullivan wrote: > +Ned > > This CL and https://codereview.chromium.org/1219663006 are pretty interdependent. That CL changes android_platform_backend to have a "device" property instead of an "adb" property. This CL changes android_browser_backend to set its "device" property to the one in android_platform_backend; it currently has an "adb" property from android_platform_backend. So, as John said in the review comment, that CL would need to be submitted before this one. > > However, that CL can't be submitted as-is because the "adb" property on android_browser_backend can't be set as that CL removes it from android_platform_backend. > > I'd prefer to solve this problem by combining the two CLs, so it's clear what is happening to these properties. > > Alternatively, I could modify the other CL to have *both* an adb and device property on android_platform_backend, and this one to remove the adb property. But that leaves android_platform_backend in a confusing state in the meantime. > > Ned, John, are you okay with this CL that merges the two? sgtm, but then, I was ok with a monolithic CL :)
On 2015/07/01 15:29:24, jbudorick wrote: > On 2015/07/01 at 15:28:29, sullivan wrote: > > +Ned > > > > This CL and https://codereview.chromium.org/1219663006 are pretty > interdependent. That CL changes android_platform_backend to have a "device" > property instead of an "adb" property. This CL changes android_browser_backend > to set its "device" property to the one in android_platform_backend; it > currently has an "adb" property from android_platform_backend. So, as John said > in the review comment, that CL would need to be submitted before this one. > > > > However, that CL can't be submitted as-is because the "adb" property on > android_browser_backend can't be set as that CL removes it from > android_platform_backend. > > > > I'd prefer to solve this problem by combining the two CLs, so it's clear what > is happening to these properties. > > > > Alternatively, I could modify the other CL to have *both* an adb and device > property on android_platform_backend, and this one to remove the adb property. > But that leaves android_platform_backend in a confusing state in the meantime. > > > > Ned, John, are you okay with this CL that merges the two? > > sgtm, but then, I was ok with a monolithic CL :) lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
On 2015/07/01 15:34:45, nednguyen wrote: > On 2015/07/01 15:29:24, jbudorick wrote: > > On 2015/07/01 at 15:28:29, sullivan wrote: > > > +Ned > > > > > > This CL and https://codereview.chromium.org/1219663006 are pretty > > interdependent. That CL changes android_platform_backend to have a "device" > > property instead of an "adb" property. This CL changes android_browser_backend > > to set its "device" property to the one in android_platform_backend; it > > currently has an "adb" property from android_platform_backend. So, as John > said > > in the review comment, that CL would need to be submitted before this one. > > > > > > However, that CL can't be submitted as-is because the "adb" property on > > android_browser_backend can't be set as that CL removes it from > > android_platform_backend. > > > > > > I'd prefer to solve this problem by combining the two CLs, so it's clear > what > > is happening to these properties. > > > > > > Alternatively, I could modify the other CL to have *both* an adb and device > > property on android_platform_backend, and this one to remove the adb property. > > But that leaves android_platform_backend in a confusing state in the meantime. > > > > > > Ned, John, are you okay with this CL that merges the two? > > > > sgtm, but then, I was ok with a monolithic CL :) > > lgtm Looks like this is also blocked on https://codereview.chromium.org/1212813007/, which is blockecd on https://codereview.chromium.org/1211323004/ Also looks like there are some failing tests in the android_forwarder CL I can look at in the meantime.
The CQ bit was checked by sullivan@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1213423003/#ps100001 (title: "Merge with https://codereview.chromium.org/1212813007/")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213423003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sullivan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213423003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2e706aa8db5d0cfd09573ad56903868c4d8cd4ee Cr-Commit-Position: refs/heads/master@{#338229}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1224273007/ by dgozman@chromium.org. The reason for reverting is: Speculative revert. Looks like this patch broke android bots with "AttributeError: 'WebviewShellBackendSettings' object has no attribute '_backend_settings'". See https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus....
Message was sent while issue was closed.
cc kbr as FYI: Ken, there is an error in my CL that wasn't caught on the commit queue, it was only caught on chromium.gpu waterfall: https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus... Known issue? (I have a fix already, but hoping these issues can be caught in the CQ in the future)
Message was sent while issue was closed.
On 2015/07/10 14:37:51, sullivan wrote: > cc kbr as FYI: Ken, there is an error in my CL that wasn't caught on the commit > queue, it was only caught on chromium.gpu waterfall: > https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus... > > Known issue? Annie: yes, unfortunately as I've mentioned on https://codereview.chromium.org/1226373004/ there are not yet CQ bots covering the Android configurations on the chromium.gpu waterfall. Sorry about that. If you have an Android device to test with, I can help you get set up to run these tests locally to avoid breakage during these significant Telemetry refactorings. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
