Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1960363002:
Change provision_devices.py to disable verity for remove webview on M (Closed)
|
Created:
4 years, 7 months ago by Yoland Yan(Google) Modified:
4 years, 7 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange provision_devices.py to disable verity for remove webview on M
BUG=
Committed: https://crrev.com/161d167a91621178a83bff8febea1a849c7fda14
Cr-Commit-Position: refs/heads/master@{#392796}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Check before nuke #Patch Set 3 : #
Total comments: 4
Patch Set 4 : Use DeviceUtils.PathExists #
Total comments: 6
Patch Set 5 : Minor changes #
Total comments: 2
Patch Set 6 : Nits #Messages
Total messages: 17 (4 generated)
yolandyan@google.com changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... build/android/provision_devices.py:302: device.Reboot() I'm not crazy about introducing yet another reboot here. Is it necessary? Could we instead split this across one of the reboots we already do? https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... build/android/provision_devices.py:303: device.adb.WaitForDevice() If we keep this, this should change to device.WaitUntilFullyBooted
https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... build/android/provision_devices.py:308: device.RunShellCommand(['rm', '-rf'] + _SYSTEM_WEBVIEW_PATHS, I think maybe what we should fix is that none of this has to run if the SYSTEM_WEBVIEW_PATHS have already been removed. My vote would be check if the system webview paths exist before doing any of this. In this case, even if we leave the reboot, it should only happen once (the first run) after the device has been flashed.
https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... build/android/provision_devices.py:302: device.Reboot() On 2016/05/09 21:07:33, jbudorick wrote: > I'm not crazy about introducing yet another reboot here. Is it necessary? Could > we instead split this across one of the reboots we already do? hmm, I don't think there is a good place to include, previous reboots is included is done by run_phase, which shouldn't include any commandline argument specific action. Also, I changed so that this only happens once on device as Mike suggested https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... build/android/provision_devices.py:303: device.adb.WaitForDevice() On 2016/05/09 21:07:33, jbudorick wrote: > If we keep this, this should change to device.WaitUntilFullyBooted Done. https://codereview.chromium.org/1960363002/diff/1/build/android/provision_dev... build/android/provision_devices.py:308: device.RunShellCommand(['rm', '-rf'] + _SYSTEM_WEBVIEW_PATHS, On 2016/05/09 21:09:55, mikecase wrote: > I think maybe what we should fix is that none of this has to run if the > SYSTEM_WEBVIEW_PATHS have already been removed. My vote would be check if the > system webview paths exist before doing any of this. In this case, even if we > leave the reboot, it should only happen once (the first run) after the device > has been flashed. Done.
https://codereview.chromium.org/1960363002/diff/40001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/40001/build/android/provision... build/android/provision_devices.py:300: output = device.RunShellCommand(['ls', p], check_return=False, Use DeviceUtils.PathExists: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/1960363002/diff/40001/build/android/provision... build/android/provision_devices.py:309: return This shouldn't return. It should simply skip the removal below.
https://codereview.chromium.org/1960363002/diff/40001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/40001/build/android/provision... build/android/provision_devices.py:300: output = device.RunShellCommand(['ls', p], check_return=False, On 2016/05/10 21:52:44, jbudorick wrote: > Use DeviceUtils.PathExists: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... Done. https://codereview.chromium.org/1960363002/diff/40001/build/android/provision... build/android/provision_devices.py:309: return On 2016/05/10 21:52:44, jbudorick wrote: > This shouldn't return. It should simply skip the removal below. Done.
https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... build/android/provision_devices.py:298: original_webview_exist = False These four lines can just be: if any(device.PathExists(p) for p in _SYSTEM_WEBVIEW_PATHS): ... https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... build/android/provision_devices.py:310: # This is required, e.g., to replace the system webview on a device. nit: line break before this line https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... build/android/provision_devices.py:319: logging.info('Original WebView already removed') nit: "system webview" instead of "original webview"
https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... build/android/provision_devices.py:298: original_webview_exist = False On 2016/05/10 22:28:44, jbudorick wrote: > These four lines can just be: > > if any(device.PathExists(p) for p in _SYSTEM_WEBVIEW_PATHS): > ... Nicee, done! https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... build/android/provision_devices.py:310: # This is required, e.g., to replace the system webview on a device. On 2016/05/10 22:28:44, jbudorick wrote: > nit: line break before this line Done. https://codereview.chromium.org/1960363002/diff/60001/build/android/provision... build/android/provision_devices.py:319: logging.info('Original WebView already removed') On 2016/05/10 22:28:44, jbudorick wrote: > nit: "system webview" instead of "original webview" Done.
lgtm w/ nit https://codereview.chromium.org/1960363002/diff/80001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/80001/build/android/provision... build/android/provision_devices.py:299: logging.info('Original WebView exists and needs to be removed') nit: System instead of Original here too
https://codereview.chromium.org/1960363002/diff/80001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1960363002/diff/80001/build/android/provision... build/android/provision_devices.py:299: logging.info('Original WebView exists and needs to be removed') On 2016/05/10 22:58:08, jbudorick wrote: > nit: System instead of Original here too Done.
The CQ bit was checked by yolandyan@google.com
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/1960363002/#ps100001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1960363002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1960363002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Change provision_devices.py to disable verity for remove webview on M BUG= ========== to ========== Change provision_devices.py to disable verity for remove webview on M BUG= Committed: https://crrev.com/161d167a91621178a83bff8febea1a849c7fda14 Cr-Commit-Position: refs/heads/master@{#392796} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/161d167a91621178a83bff8febea1a849c7fda14 Cr-Commit-Position: refs/heads/master@{#392796} |
