Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(639)

Issue 23467004: [android] Update FlagChanger to work with additional apks. (Closed)

Created:
7 years, 3 months ago by craigdh
Modified:
7 years, 3 months ago
Reviewers:
frankf
CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org, nyquist
Visibility:
Public.

Description

[android] Update FlagChanger to work with additional apks. BUG=277715 TEST=None NOTRY=True Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220313

Patch Set 1 #

Total comments: 6

Patch Set 2 : addressed comments #

Total comments: 6

Patch Set 3 : added default parameter to constructor to avoid breaking downstream #

Patch Set 4 : clean up some code duplication #

Patch Set 5 : cmd_helper._Call -> cmd_helper.Call #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -46 lines) Patch
M build/android/pylib/base/base_test_runner.py View 3 chunks +0 lines, -4 lines 0 comments Download
M build/android/pylib/cmd_helper.py View 1 2 3 4 3 chunks +4 lines, -4 lines 0 comments Download
M build/android/pylib/constants.py View 3 chunks +46 lines, -12 lines 0 comments Download
M build/android/pylib/flag_changer.py View 1 2 4 chunks +17 lines, -12 lines 0 comments Download
M build/android/pylib/gtest/test_package_apk.py View 4 chunks +8 lines, -13 lines 0 comments Download
M build/android/pylib/instrumentation/test_runner.py View 1 2 3 3 chunks +11 lines, -1 line 0 comments Download

Messages

Total messages: 10 (0 generated)
craigdh
7 years, 3 months ago (2013-08-26 23:35:53 UTC) #1
frankf
is there a corresponding downstream patch? https://codereview.chromium.org/23467004/diff/1/build/android/pylib/flag_changer.py File build/android/pylib/flag_changer.py (right): https://codereview.chromium.org/23467004/diff/1/build/android/pylib/flag_changer.py#newcode21 build/android/pylib/flag_changer.py:21: def __init__(self, adb, ...
7 years, 3 months ago (2013-08-27 00:28:08 UTC) #2
craigdh
https://codereview.chromium.org/23467004/diff/1/build/android/pylib/flag_changer.py File build/android/pylib/flag_changer.py (right): https://codereview.chromium.org/23467004/diff/1/build/android/pylib/flag_changer.py#newcode21 build/android/pylib/flag_changer.py:21: def __init__(self, adb, cmdline_file): On 2013/08/27 00:28:09, frankf wrote: ...
7 years, 3 months ago (2013-08-27 17:26:12 UTC) #3
craigdh
On 2013/08/27 00:28:08, frankf wrote: > is there a corresponding downstream patch? No corresponding downstream ...
7 years, 3 months ago (2013-08-27 17:28:32 UTC) #4
frankf
lgtm https://codereview.chromium.org/23467004/diff/11001/build/android/pylib/flag_changer.py File build/android/pylib/flag_changer.py (right): https://codereview.chromium.org/23467004/diff/11001/build/android/pylib/flag_changer.py#newcode144 build/android/pylib/flag_changer.py:144: logging.warn("Unterminated quoted argument: " + current_flag) single quote ...
7 years, 3 months ago (2013-08-27 23:57:46 UTC) #5
craigdh
https://codereview.chromium.org/23467004/diff/11001/build/android/pylib/flag_changer.py File build/android/pylib/flag_changer.py (right): https://codereview.chromium.org/23467004/diff/11001/build/android/pylib/flag_changer.py#newcode20 build/android/pylib/flag_changer.py:20: def __init__(self, adb, cmdline_file): make cmdline_file optional so as ...
7 years, 3 months ago (2013-08-28 00:03:03 UTC) #6
craigdh
https://codereview.chromium.org/23467004/diff/11001/build/android/pylib/flag_changer.py File build/android/pylib/flag_changer.py (right): https://codereview.chromium.org/23467004/diff/11001/build/android/pylib/flag_changer.py#newcode20 build/android/pylib/flag_changer.py:20: def __init__(self, adb, cmdline_file): On 2013/08/28 00:03:03, craigdh wrote: ...
7 years, 3 months ago (2013-08-28 19:17:10 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/23467004/34001
7 years, 3 months ago (2013-08-29 16:23:31 UTC) #8
commit-bot: I haz the power
Change committed as 220313
7 years, 3 months ago (2013-08-29 16:26:12 UTC) #9
Dominik Grewe
7 years, 3 months ago (2013-09-02 09:44:30 UTC) #10
Message was sent while issue was closed.
On 2013/08/27 17:28:32, craigdh wrote:
> On 2013/08/27 00:28:08, frankf wrote:
> > is there a corresponding downstream patch?
> 
> No corresponding downstream patch yet. I left in the constants that are needed
> downstream so the patches don't have to land together.

Your patch seems to be breaking this CL:
https://gerrit-int.chromium.org/#/c/28115/
It relies on BaseTestRunner to have a 'flags' attribute which you removed in
this patch.

Powered by Google App Engine
This is Rietveld 408576698