Add a way to reset cached command line arguments

Add a way to reset cached command line arguments

Command line arguments are set at runtime in instrumentation tests
(see base.test.util.CommandLineFlags.java) and caching them breaks
some tests.

Added a reset counter, so that code caching the command line result
can check the counter to know when to checks the arguments again

TBR=yfriedman@chromium.org
BUG=514155
Committed: https://crrev.com/bef1072f72220ebc4c9a790f572c71ba86bf1777
Cr-Commit-Position: refs/heads/master@{#340457}

[dgn, 2015-07-27 10:49:23]

dgn@chromium.org changed reviewers:
+ bauerb@chromium.org

[dgn, 2015-07-27 10:49:23]

PTAL

[Bernhard Bauer, 2015-07-27 11:04:19]

https://codereview.chromium.org/1254973003/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/CommandLine.java (right):

https://codereview.chromium.org/1254973003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/CommandLine.java:137:
sResetCounter.incrementAndGet();
Could we use an observer here that would clear the cached value in
FeatureUtilities? That would mean we wouldn't incur the overhead of the atomic
access every time isDocumentMode is called (which seems to be often).

[dgn, 2015-07-27 11:35:35]

https://codereview.chromium.org/1254973003/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/CommandLine.java (right):

https://codereview.chromium.org/1254973003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/CommandLine.java:137:
sResetCounter.incrementAndGet();
On 2015/07/27 11:04:18, Bernhard Bauer wrote:
> Could we use an observer here that would clear the cached value in
> FeatureUtilities? That would mean we wouldn't incur the overhead of the atomic
> access every time isDocumentMode is called (which seems to be often).

Done. But maybe I could also have used a normal int? reset() should be called
only at test setup so I don't think there will be a concurrency issue there.

[Bernhard Bauer, 2015-07-27 12:45:55]

https://codereview.chromium.org/1254973003/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/CommandLine.java (right):

https://codereview.chromium.org/1254973003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/CommandLine.java:137:
sResetCounter.incrementAndGet();
On 2015/07/27 11:35:35, dgn wrote:
> On 2015/07/27 11:04:18, Bernhard Bauer wrote:
> > Could we use an observer here that would clear the cached value in
> > FeatureUtilities? That would mean we wouldn't incur the overhead of the
atomic
> > access every time isDocumentMode is called (which seems to be often).
> 
> Done. But maybe I could also have used a normal int? reset() should be called
> only at test setup so I don't think there will be a concurrency issue there.

Maybe... the question is whether the value is also _read_ on the same thread.
But using the observer seems more idiomatic to me anyway.

https://codereview.chromium.org/1254973003/diff/20001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
(right):

https://codereview.chromium.org/1254973003/diff/20001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:75:
CommandLine.notifyResetListeners();
Could you move this into reset(), and notify the observers asynchronously by
posting to a handler?

[dgn, 2015-07-27 13:11:37]

PTAL

https://codereview.chromium.org/1254973003/diff/20001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
(right):

https://codereview.chromium.org/1254973003/diff/20001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java:75:
CommandLine.notifyResetListeners();
On 2015/07/27 12:45:55, Bernhard Bauer wrote:
> Could you move this into reset(), and notify the observers asynchronously by
> posting to a handler?

Done.

[Bernhard Bauer, 2015-07-27 13:17:06]

LGTM, thanks!

[dgn, 2015-07-27 13:17:49]

The CQ bit was checked by dgn@chromium.org

[commit-bot: I haz the power, 2015-07-27 13:18:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254973003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254973003/40001

[commit-bot: I haz the power, 2015-07-27 14:29:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-27 14:30:13]

Patchset 3 (id:??) landed as
https://crrev.com/bef1072f72220ebc4c9a790f572c71ba86bf1777
Cr-Commit-Position: refs/heads/master@{#340457}

[jdduke (slow), 2015-07-27 15:05:23]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-07-27 15:05:23]

https://codereview.chromium.org/1254973003/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/CommandLine.java (right):

https://codereview.chromium.org/1254973003/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/CommandLine.java:154: public static void
addResetListener(ResetListener listener) {
Hmm, I'd really like to understand use-cases that legitimately need dynamic
CommandLine-change observation outside of tests. I don't think we should be
complicating this interface with listeners only to supporting testing. Rather,
we should modify tests to be consistent in how they rely on command-line flags.

[dgn, 2015-07-27 15:12:29]

https://codereview.chromium.org/1254973003/diff/40001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/CommandLine.java (right):

https://codereview.chromium.org/1254973003/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/CommandLine.java:154: public static void
addResetListener(ResetListener listener) {
On 2015/07/27 15:05:23, jdduke wrote:
> Hmm, I'd really like to understand use-cases that legitimately need dynamic
> CommandLine-change observation outside of tests. I don't think we should be
> complicating this interface with listeners only to supporting testing. Rather,
> we should modify tests to be consistent in how they rely on command-line
flags.

It should not be used out of tests, yes. An annotation is missing. And the
correct solution would be to fix the tests indeed. There I just wanted to send a
fix that would make the tests pass without reverting cls or disabling them on
all L+ bots.

[jdduke (slow), 2015-07-27 15:51:12]

On 2015/07/27 15:12:29, dgn wrote:
>
https://codereview.chromium.org/1254973003/diff/40001/base/android/java/src/o...
> File base/android/java/src/org/chromium/base/CommandLine.java (right):
> 
>
https://codereview.chromium.org/1254973003/diff/40001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/CommandLine.java:154: public static
void
> addResetListener(ResetListener listener) {
> On 2015/07/27 15:05:23, jdduke wrote:
> > Hmm, I'd really like to understand use-cases that legitimately need dynamic
> > CommandLine-change observation outside of tests. I don't think we should be
> > complicating this interface with listeners only to supporting testing.
Rather,
> > we should modify tests to be consistent in how they rely on command-line
> flags.
> 
> It should not be used out of tests, yes. An annotation is missing. And the
> correct solution would be to fix the tests indeed. There I just wanted to send
a
> fix that would make the tests pass without reverting cls or disabling them on
> all L+ bots.

Perfect, sounds good.