[Android] Add a low-end device mode override flag

[Android] Add a low-end device mode override flag

The current low-end device optimizations are restricted to devices that meet
certain memory and OS version criteria.  Add a flag that overrides this
detection, simplifying testing and allowing power users to take advantage
of the optimizations.

BUG=320747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237584

[jdduke (slow), 2013-11-06 19:24:43]

bulach@: Does this look sensible? If so I'll send out for further OWNER review.
Thanks.

[bulach, 2013-11-07 16:18:03]

the idea looks reasonable, thanks! however david just changed it so it goes
native => java rather than the other way around:
https://chromiumcodereview.appspot.com/59033008/

you'll probably need to change in the java side rather than in the .cc?

[jdduke (slow), 2013-11-07 16:24:09]

On 2013/11/07 16:18:03, bulach wrote:
> the idea looks reasonable, thanks! however david just changed it so it goes
> native => java rather than the other way around:
> https://chromiumcodereview.appspot.com/59033008/
> 
> you'll probably need to change in the java side rather than in the .cc?

Ah, I see... I assume we can still query flags before native is initialized?

[jdduke (slow), 2013-11-07 16:31:26]

On 2013/11/07 16:24:09, jdduke wrote:
> On 2013/11/07 16:18:03, bulach wrote:
> > the idea looks reasonable, thanks! however david just changed it so it goes
> > native => java rather than the other way around:
> > https://chromiumcodereview.appspot.com/59033008/
> > 
> > you'll probably need to change in the java side rather than in the .cc?
> 
> Ah, I see... I assume we can still query flags before native is initialized?

Yeah it looks like this will still work, thanks for the heads up.  I'll ping for
review again upon rebase.

[jdduke (slow), 2013-11-07 22:57:19]

Per discussion with dtrainor@, rather than having an override setter on SysUtils
for low-end device mode, we might as well move CommandLine.java to base,
factoring out the content-specific switches.  As soon as crrev.com/59033008 I'll
rebase and have SysUtils.java query CommandLine.java for the override flag.

[digit1, 2013-11-08 08:19:10]

On 2013/11/07 22:57:19, jdduke wrote:
> Per discussion with dtrainor@, rather than having an override setter on
SysUtils
> for low-end device mode, we might as well move CommandLine.java to base,
> factoring out the content-specific switches.  As soon as crrev.com/59033008
I'll
> rebase and have SysUtils.java query CommandLine.java for the override flag.

Having SysUtils directly query CommandLine is a bad idea imho, because it
introduces coupling, which makes testing more difficult.
What about adding a setLowEndDeviceFlag() method to SysUtils instead, and call
it at the right time.

[David Trainor- moved to gerrit, 2013-11-08 09:48:57]

On 2013/11/08 08:19:10, digit1 wrote:
> On 2013/11/07 22:57:19, jdduke wrote:
> > Per discussion with dtrainor@, rather than having an override setter on
> SysUtils
> > for low-end device mode, we might as well move CommandLine.java to base,
> > factoring out the content-specific switches.  As soon as crrev.com/59033008
> I'll
> > rebase and have SysUtils.java query CommandLine.java for the override flag.
> 
> Having SysUtils directly query CommandLine is a bad idea imho, because it
> introduces coupling, which makes testing more difficult.
> What about adding a setLowEndDeviceFlag() method to SysUtils instead, and call
> it at the right time.

I see your point.  The only downside with that is that every activity we create
would have to remember call set, which can lead to scenarios where this could be
silently failing until someone actually tried to use the flag for that new
activity.

[David Trainor- moved to gerrit, 2013-11-08 09:49:56]

On 2013/11/08 09:48:57, David Trainor wrote:
> On 2013/11/08 08:19:10, digit1 wrote:
> > On 2013/11/07 22:57:19, jdduke wrote:
> > > Per discussion with dtrainor@, rather than having an override setter on
> > SysUtils
> > > for low-end device mode, we might as well move CommandLine.java to base,
> > > factoring out the content-specific switches.  As soon as
crrev.com/59033008
> > I'll
> > > rebase and have SysUtils.java query CommandLine.java for the override
flag.
> > 
> > Having SysUtils directly query CommandLine is a bad idea imho, because it
> > introduces coupling, which makes testing more difficult.
> > What about adding a setLowEndDeviceFlag() method to SysUtils instead, and
call
> > it at the right time.
> 
> I see your point.  The only downside with that is that every activity we
create
> would have to remember call set, which can lead to scenarios where this could
be
> silently failing until someone actually tried to use the flag for that new
> activity.

We did talk about the option of using a set method though.  If you guys think
that's the better approach I'm okay with it :).

[David Trainor- moved to gerrit, 2013-11-08 09:49:59]

On 2013/11/08 09:48:57, David Trainor wrote:
> On 2013/11/08 08:19:10, digit1 wrote:
> > On 2013/11/07 22:57:19, jdduke wrote:
> > > Per discussion with dtrainor@, rather than having an override setter on
> > SysUtils
> > > for low-end device mode, we might as well move CommandLine.java to base,
> > > factoring out the content-specific switches.  As soon as
crrev.com/59033008
> > I'll
> > > rebase and have SysUtils.java query CommandLine.java for the override
flag.
> > 
> > Having SysUtils directly query CommandLine is a bad idea imho, because it
> > introduces coupling, which makes testing more difficult.
> > What about adding a setLowEndDeviceFlag() method to SysUtils instead, and
call
> > it at the right time.
> 
> I see your point.  The only downside with that is that every activity we
create
> would have to remember call set, which can lead to scenarios where this could
be
> silently failing until someone actually tried to use the flag for that new
> activity.

We did talk about the option of using a set method though.  If you guys think
that's the better approach I'm okay with it :).

[digit1, 2013-11-08 10:38:17]

On 2013/11/08 09:49:56, David Trainor wrote:
> > I see your point.  The only downside with that is that every activity we
> create
> > would have to remember call set, which can lead to scenarios where this
could
> be
> > silently failing until someone actually tried to use the flag for that new
> > activity.
> 

I understand, but I think this is a consequence of our current code base's lack
of coherent
initialization entry points :) I already encountered this issue when writing the
content linker
and its tests. I ended up using a three-state setting (auto-detect / not-low-end
/ is-low-end)
to make testing easier.

> We did talk about the option of using a set method though.  If you guys think
> that's the better approach I'm okay with it :).

[jdduke (slow), 2013-11-08 15:48:55]

On 2013/11/08 10:38:17, digit1 wrote:
> On 2013/11/08 09:49:56, David Trainor wrote:
> > > I see your point.  The only downside with that is that every activity we
> > create
> > > would have to remember call set, which can lead to scenarios where this
> could
> > be
> > > silently failing until someone actually tried to use the flag for that new
> > > activity.
> > 
> 
> I understand, but I think this is a consequence of our current code base's
lack
> of coherent
> initialization entry points :) I already encountered this issue when writing
the
> content linker
> and its tests. I ended up using a three-state setting (auto-detect /
not-low-end
> / is-low-end)
> to make testing easier.
> 
> > We did talk about the option of using a set method though.  If you guys
think
> > that's the better approach I'm okay with it :).

I'm not sure I quite understand the "coupling" argument against using
CommandLine here.  Logging, tracing, threading and messaging components in base
all depend on CommandLine, and if using it is considered excessive coupling, I
think our codebase has a problem =/  

In any case, the Java version of CommandLine really ought to be in base, so
whether we move it in this patch or separately I think it still needs to happen.

[jdduke (slow), 2013-11-08 21:54:24]

digit1@: I've rebased to your changes, what do you think? If you're strongly
against a CommandLine dependency in SysUtils, we can figure out a way to set the
override flag in a consistent way for all platforms/processes/apps... I'd really
hate to just have the setter accompany every place we initialize the
CommandLine, and it doesn't really fit as part of CommandLine initialization
either.

[jdduke (slow), 2013-11-12 19:59:52]

On 2013/11/08 21:54:24, jdduke wrote:
> digit1@: I've rebased to your changes, what do you think? If you're strongly
> against a CommandLine dependency in SysUtils, we can figure out a way to set
the
> override flag in a consistent way for all platforms/processes/apps... I'd
really
> hate to just have the setter accompany every place we initialize the
> CommandLine, and it doesn't really fit as part of CommandLine initialization
> either.

Ping? digit1@, thoughts?

[jdduke (slow), 2013-11-20 16:11:00]

bulach@: OK, rebased to the CommandLine move. What do you think?

[bulach, 2013-11-20 16:31:11]

some nits and one question below:

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/BaseSwitches.java (right):

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
base/android/java/src/org/chromium/base/BaseSwitches.java:19: public static
final String ENABLE_LOW_END_DEVICE_MODE = "enable-low-end-device-mode";
nit: remove extra space before public

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SysUtils.java:104: while (sLowEndDevice
== null) {
not quite sure why use a loop here?

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6962: +        <message
name="IDS_FLAGS_LOW_END_DEVICE_MODE_DESCRIPTION" desc="Description of the flag
to disable the click delay.">
nit: desc="Description of the flag to tweak low end device optimizations"

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6963: +          Overrides low end device
detection for reducing memory consumption.
it's not just about memory, so how about:
Overrides low end device optimizations.

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6965: +        <message
name="IDS_FLAGS_LOW_END_DEVICE_MODE_NAME" desc="Name of the flag to disable the
click delay.">
nit: desc="Name of the flag to tweak low end device optimizations"

[jdduke (slow), 2013-11-20 18:16:33]

PTAL for owner review. Thanks!

jochen@: content/browser/renderer_host
ajwong@: base/

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/BaseSwitches.java (right):

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
base/android/java/src/org/chromium/base/BaseSwitches.java:19: public static
final String ENABLE_LOW_END_DEVICE_MODE = "enable-low-end-device-mode";
On 2013/11/20 16:31:12, bulach wrote:
> nit: remove extra space before public

Done.

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/62833002/diff/320001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SysUtils.java:104: while (sLowEndDevice
== null) {
On 2013/11/20 16:31:12, bulach wrote:
> not quite sure why use a loop here?

Only for ease of breaking upon |sLowEndDevice| initialization. I can switch to a
simple return model instead.

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6962: +        <message
name="IDS_FLAGS_LOW_END_DEVICE_MODE_DESCRIPTION" desc="Description of the flag
to disable the click delay.">
On 2013/11/20 16:31:12, bulach wrote:
> nit: desc="Description of the flag to tweak low end device optimizations"

Done.

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6963: +          Overrides low end device
detection for reducing memory consumption.
On 2013/11/20 16:31:12, bulach wrote:
> it's not just about memory, so how about:
> Overrides low end device optimizations.

Done.

https://codereview.chromium.org/62833002/diff/320001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6965: +        <message
name="IDS_FLAGS_LOW_END_DEVICE_MODE_NAME" desc="Name of the flag to disable the
click delay.">
On 2013/11/20 16:31:12, bulach wrote:
> nit: desc="Name of the flag to tweak low end device optimizations"

Done.

[jdduke (slow), 2013-11-20 18:17:17]

Oops, forgot to actually add ajwong@. PTAL for owner review. Thanks!

ajwong@: base/

[awong, 2013-11-21 00:06:18]

base LGTM

[jochen (gone - plz use gerrit), 2013-11-21 13:15:02]

lgtm

[jdduke (slow), 2013-11-21 16:10:17]

bulach@: Is there anything else you'd like to see here? Thanks.

[bulach, 2013-11-27 08:58:25]

lgtm, sorry about the delay!

[commit-bot: I haz the power, 2013-11-27 14:53:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/62833002/580001

[commit-bot: I haz the power, 2013-11-27 16:40:55]

Change committed as 237584