Expose a low-end device mode override flags for non-android OSs as well

Expose a low-end device mode override flags for non-android OSs as well

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

[c.shu, 2014-04-24 15:48:26]

This patch is based on the work from Viatcheslav Ostapenko.
https://codereview.chromium.org/119493005/
Thanks in advance for the review.

[piman, 2014-04-24 19:50:07]

https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
File base/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode17
base/sys_utils.cc:17: bool detectLowEndDevice() {
nit: style

https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode42
base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
This won't work in subprocesses unless:
1- command-line flags are passed through to those subprocesses
2- this is evaluated/cached before the sandbox is turned on
(AmountOfPhysicalMemoryMB is done via sysconf, by openning /proc/meminfo, which
is not allowed in the sandbox).

[c.shu, 2014-04-24 20:08:59]

On 2014/04/24 19:50:07, piman wrote:
> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
> File base/sys_utils.cc (right):
> 
> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode17
> base/sys_utils.cc:17: bool detectLowEndDevice() {
> nit: style
> 
> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode42
> base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
> This won't work in subprocesses unless:
> 1- command-line flags are passed through to those subprocesses
> 2- this is evaluated/cached before the sandbox is turned on
> (AmountOfPhysicalMemoryMB is done via sysconf, by openning /proc/meminfo,
which
> is not allowed in the sandbox).

If base::SysInfo::AmountOfPhysicalMemoryMB() cannot return the memory size
reliably, shall we bring back the original jni code for Android and always
return false by default for non-Android?

[ostap, 2014-04-29 06:13:31]

On 2014/04/24 19:50:07, piman wrote:
> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
> File base/sys_utils.cc (right):
> 
> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode17
> base/sys_utils.cc:17: bool detectLowEndDevice() {
> nit: style
> 
> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode42
> base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
> This won't work in subprocesses unless:
> 1- command-line flags are passed through to those subprocesses
> 2- this is evaluated/cached before the sandbox is turned on
> (AmountOfPhysicalMemoryMB is done via sysconf, by openning /proc/meminfo,
which
> is not allowed in the sandbox).

Does it mean that there is a bug in V8Initializer.cpp/initializeV8Common() ? It
calls physicalMemoryMB() and numberOfProcessors() from blink::Platform which
itself calls SysInfo functions.

[ostap, 2014-04-29 06:20:28]

On 2014/04/29 06:13:31, ostap wrote:
> On 2014/04/24 19:50:07, piman wrote:
> > https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
> > File base/sys_utils.cc (right):
> > 
> > https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode17
> > base/sys_utils.cc:17: bool detectLowEndDevice() {
> > nit: style
> > 
> > https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode42
> > base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
> > This won't work in subprocesses unless:
> > 1- command-line flags are passed through to those subprocesses
> > 2- this is evaluated/cached before the sandbox is turned on
> > (AmountOfPhysicalMemoryMB is done via sysconf, by openning /proc/meminfo,
> which
> > is not allowed in the sandbox).
> 
> Does it mean that there is a bug in V8Initializer.cpp/initializeV8Common() ?
It
> calls physicalMemoryMB() and numberOfProcessors() from blink::Platform which
> itself calls SysInfo functions.

I don't think it will help because Java code itself read /proc/meminfo .
Check base/android/java/src/org/chromium/base/SysUtils.java .

I would investigate caching of values before sandbos is turned on. Since it is
already cached in SysInfo by lazy instances, it means that it is enough to call
SysInfo::AmountOfPhysicalMemory() before sandbox initialized.

[piman, 2014-04-29 22:17:09]

On Mon, Apr 28, 2014 at 11:13 PM, <sl.ostapenko@samsung.com> wrote:

> On 2014/04/24 19:50:07, piman wrote:
>
>> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
>> File base/sys_utils.cc (right):
>>
>
>  https://codereview.chromium.org/258663002/diff/1/base/sys_
>> utils.cc#newcode17
>> base/sys_utils.cc:17: bool detectLowEndDevice() {
>> nit: style
>>
>
>  https://codereview.chromium.org/258663002/diff/1/base/sys_
>> utils.cc#newcode42
>> base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
>> This won't work in subprocesses unless:
>> 1- command-line flags are passed through to those subprocesses
>> 2- this is evaluated/cached before the sandbox is turned on
>> (AmountOfPhysicalMemoryMB is done via sysconf, by openning /proc/meminfo,
>>
> which
>
>> is not allowed in the sandbox).
>>
>
> Does it mean that there is a bug in V8Initializer.cpp/initializeV8Common()
> ? It
> calls physicalMemoryMB() and numberOfProcessors() from blink::Platform
> which
> itself calls SysInfo functions.
>

I wouldn't be surprised if this was the case indeed.


>
> https://codereview.chromium.org/258663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2014-04-30 17:06:09]

On 2014/04/29 22:17:09, piman wrote:
> On Mon, Apr 28, 2014 at 11:13 PM, <mailto:sl.ostapenko@samsung.com> wrote:
> 
> > On 2014/04/24 19:50:07, piman wrote:
> >
> >> https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
> >> File base/sys_utils.cc (right):
> >>
> >
> >  https://codereview.chromium.org/258663002/diff/1/base/sys_
> >> utils.cc#newcode17
> >> base/sys_utils.cc:17: bool detectLowEndDevice() {
> >> nit: style
> >>
> >
> >  https://codereview.chromium.org/258663002/diff/1/base/sys_
> >> utils.cc#newcode42
> >> base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
> >> This won't work in subprocesses unless:
> >> 1- command-line flags are passed through to those subprocesses
> >> 2- this is evaluated/cached before the sandbox is turned on
> >> (AmountOfPhysicalMemoryMB is done via sysconf, by openning /proc/meminfo,
> >>
> > which
> >
> >> is not allowed in the sandbox).
> >>
> >
> > Does it mean that there is a bug in V8Initializer.cpp/initializeV8Common()
> > ? It
> > calls physicalMemoryMB() and numberOfProcessors() from blink::Platform
> > which
> > itself calls SysInfo functions.
> >
> 
> I wouldn't be surprised if this was the case indeed.

We invoke the sysinfo getters before turning on the sandbox the cache the
values.

> 
> 
> >
> > https://codereview.chromium.org/258663002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[c.shu, 2014-04-30 21:19:01]

Addressed the command-line flag forwarding issue. The
getting-sysinfo-before-sandbox should already be working based on jochen's
comments. Thanks.

[c.shu, 2014-06-05 15:35:54]

On 2014/04/30 21:19:01, c.shu wrote:
> Addressed the command-line flag forwarding issue. The
> getting-sysinfo-before-sandbox should already be working based on jochen's
> comments. Thanks.

Would any reviewers comment on the latest CL? Thanks.

[jochen (gone - plz use gerrit), 2014-06-06 07:09:37]

the beef of this CL is in base/ - I recommend to explicitly ask a base owner to
review the CL.

Happy to approve the mechanical updates in the other dirs afterwards

[brettw, 2014-06-09 22:49:57]

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc
File base/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode14
base/sys_utils.cc:14: namespace {
blank lines around this, also check other namespaces in this file.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode20
base/sys_utils.cc:20: return true;
Check indenting.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode42
base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
I think this leaky stuff is overkill for this use case. The singletons require a
lot of work and this is only a bool.

There is no problem with running the query multiple times in the case of a race
condition. I know this is OK on x86, does this also work on ARM? :
  static bool queried = false;
  static bool is_low_end = false;
  if (!queried) {
    is_low_end = Detect...
    queried = true
  }
  return is_low_end;

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h
File base/sys_utils.h (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h#newcode8
base/sys_utils.h:8: #include <stddef.h>
Blank line between these

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h#newcode19
base/sys_utils.h:19: static size_t AmountOfPhysicalMemoryKB();
It seems like these should be merged with base/sys_info.h which has this kind of
thing in it.

https://codereview.chromium.org/258663002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/web_cache_manager.cc (right):

https://codereview.chromium.org/258663002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/web_cache_manager.cc:326: if
(base::SysUtils::IsLowEndDevice())
Can you put {} in this block since it's more than one line?

[brettw, 2014-06-09 22:49:58]

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc
File base/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode14
base/sys_utils.cc:14: namespace {
blank lines around this, also check other namespaces in this file.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode20
base/sys_utils.cc:20: return true;
Check indenting.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode42
base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
I think this leaky stuff is overkill for this use case. The singletons require a
lot of work and this is only a bool.

There is no problem with running the query multiple times in the case of a race
condition. I know this is OK on x86, does this also work on ARM? :
  static bool queried = false;
  static bool is_low_end = false;
  if (!queried) {
    is_low_end = Detect...
    queried = true
  }
  return is_low_end;

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h
File base/sys_utils.h (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h#newcode8
base/sys_utils.h:8: #include <stddef.h>
Blank line between these

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h#newcode19
base/sys_utils.h:19: static size_t AmountOfPhysicalMemoryKB();
It seems like these should be merged with base/sys_info.h which has this kind of
thing in it.

https://codereview.chromium.org/258663002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/web_cache_manager.cc (right):

https://codereview.chromium.org/258663002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/web_cache_manager.cc:326: if
(base::SysUtils::IsLowEndDevice())
Can you put {} in this block since it's more than one line?

[brettw, 2014-06-09 22:53:31]

Also, it's not clear to me if piman's comments are still outstanding.

[piman, 2014-06-10 01:23:05]

On Mon, Jun 9, 2014 at 3:49 PM, <brettw@chromium.org> wrote:

>
> https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc
> File base/sys_utils.cc (right):
>
> https://codereview.chromium.org/258663002/diff/20001/base/
> sys_utils.cc#newcode14
> base/sys_utils.cc:14: namespace {
> blank lines around this, also check other namespaces in this file.
>
> https://codereview.chromium.org/258663002/diff/20001/base/
> sys_utils.cc#newcode20
> base/sys_utils.cc:20: return true;
> Check indenting.
>
> https://codereview.chromium.org/258663002/diff/20001/base/
> sys_utils.cc#newcode42
> base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
> I think this leaky stuff is overkill for this use case. The singletons
> require a lot of work and this is only a bool.
>
> There is no problem with running the query multiple times in the case of
> a race condition. I know this is OK on x86, does this also work on ARM?
>

https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...


> :
>   static bool queried = false;
>   static bool is_low_end = false;
>   if (!queried) {
>     is_low_end = Detect...
>     queried = true
>   }
>   return is_low_end;
>
> https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h
> File base/sys_utils.h (right):
>
> https://codereview.chromium.org/258663002/diff/20001/base/
> sys_utils.h#newcode8
> base/sys_utils.h:8: #include <stddef.h>
> Blank line between these
>
> https://codereview.chromium.org/258663002/diff/20001/base/
> sys_utils.h#newcode19
> base/sys_utils.h:19: static size_t AmountOfPhysicalMemoryKB();
> It seems like these should be merged with base/sys_info.h which has this
> kind of thing in it.
>
> https://codereview.chromium.org/258663002/diff/20001/
> chrome/browser/renderer_host/web_cache_manager.cc
> File chrome/browser/renderer_host/web_cache_manager.cc (right):
>
> https://codereview.chromium.org/258663002/diff/20001/
> chrome/browser/renderer_host/web_cache_manager.cc#newcode326
> chrome/browser/renderer_host/web_cache_manager.cc:326: if
> (base::SysUtils::IsLowEndDevice())
> Can you put {} in this block since it's more than one line?
>
> https://codereview.chromium.org/258663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ostap, 2014-06-10 17:05:57]

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc
File base/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode42
base/sys_utils.cc:42: return g_lazy_low_end_device.Get().value();
On 2014/06/09 22:49:57, brettw wrote:
> I think this leaky stuff is overkill for this use case. The singletons require
a
> lot of work and this is only a bool.
> 
> There is no problem with running the query multiple times in the case of a
race
> condition. I know this is OK on x86, does this also work on ARM? :
>   static bool queried = false;
>   static bool is_low_end = false;
>   if (!queried) {
>     is_low_end = Detect...
>     queried = true
>   }
>   return is_low_end;

There are several places in base/sys-*.cc where LazyInstance is used for
primitive types. I would keep LazyInstance here for consistency.

[brettw, 2014-06-10 17:12:58]

On 2014/06/10 17:05:57, ostap wrote:
> There are several places in base/sys-*.cc where LazyInstance is used for
> primitive types. I would keep LazyInstance here for consistency.

OK

[c.shu, 2014-06-10 17:36:26]

https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc
File base/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/1/base/sys_utils.cc#newcode17
base/sys_utils.cc:17: bool detectLowEndDevice() {
On 2014/04/24 19:50:08, piman wrote:
> nit: style

Done.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc
File base/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode14
base/sys_utils.cc:14: namespace {
On 2014/06/09 22:49:57, brettw wrote:
> blank lines around this, also check other namespaces in this file.

Done.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.cc#newcode20
base/sys_utils.cc:20: return true;
On 2014/06/09 22:49:57, brettw wrote:
> Check indenting.

Done.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h
File base/sys_utils.h (right):

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h#newcode8
base/sys_utils.h:8: #include <stddef.h>
On 2014/06/09 22:49:57, brettw wrote:
> Blank line between these

Done.

https://codereview.chromium.org/258663002/diff/20001/base/sys_utils.h#newcode19
base/sys_utils.h:19: static size_t AmountOfPhysicalMemoryKB();
On 2014/06/09 22:49:57, brettw wrote:
> It seems like these should be merged with base/sys_info.h which has this kind
of
> thing in it.

Right, this function simply returns SysInfo::AmountOfPhysicalMemory() / 1024.
Maybe we can even remove it.

https://codereview.chromium.org/258663002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/web_cache_manager.cc (right):

https://codereview.chromium.org/258663002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/web_cache_manager.cc:326: if
(base::SysUtils::IsLowEndDevice())
On 2014/06/09 22:49:57, brettw wrote:
> Can you put {} in this block since it's more than one line?

Done.

[c.shu, 2014-06-10 17:38:15]

On 2014/06/09 22:53:31, brettw wrote:
> Also, it's not clear to me if piman's comments are still outstanding.

I tested the commandline flags and they were propagated to subprocesses and
worked fine with sandbox.
Thanks for the review, guys.

[piman, 2014-06-10 23:03:51]

lgtm for content/ and gpu/

[willchan no longer on Chromium, 2014-06-11 17:54:57]

I've removed myself as a reviewer as I'm traveling (mostly on vacation).


On Tue, Jun 10, 2014 at 4:03 PM, <piman@chromium.org> wrote:

> lgtm for content/ and gpu/
>
> https://codereview.chromium.org/258663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2014-06-11 21:02:41]

https://codereview.chromium.org/258663002/diff/100001/base/android/sys_utils_...
File base/android/sys_utils_android.cc (right):

https://codereview.chromium.org/258663002/diff/100001/base/android/sys_utils_...
base/android/sys_utils_android.cc:24: }  // namespace android
Blank line above here.

https://codereview.chromium.org/258663002/diff/100001/base/android/sys_utils_...
base/android/sys_utils_android.cc:34: }  // namespace base
Blank line above here

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc#n...
base/base_switches.cc:21: const char kEnableLowEndDeviceMode[] =
"enable-low-end-device-mode";
Can you align the = on this one and the next one you added for consistency?
(unless it makes it >80 cols)

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc#n...
base/base_switches.cc:71: const char kDetectLowEndDevice[] =
"detect-low-end-device";
I'm not super excited about having 3 arguments to control this. Is it possible
to do something like:
--low-end-device-mode=0  meaning not low end
--low-end-device-mode=1  meaning low end
with undefined/empty meaning autodetect.

https://codereview.chromium.org/258663002/diff/100001/base/sys_utils.h
File base/sys_utils.h (right):

https://codereview.chromium.org/258663002/diff/100001/base/sys_utils.h#newcode17
base/sys_utils.h:17: static bool IsLowEndDevice();
When I suggested moving this to sys_info, I was expecting this function to move,
so you would call base::SysInfo::IsLowEndDevice

[c.shu, 2014-06-12 21:12:49]

https://codereview.chromium.org/258663002/diff/100001/base/android/sys_utils_...
File base/android/sys_utils_android.cc (right):

https://codereview.chromium.org/258663002/diff/100001/base/android/sys_utils_...
base/android/sys_utils_android.cc:24: }  // namespace android
On 2014/06/11 21:02:40, brettw wrote:
> Blank line above here.

Done.

https://codereview.chromium.org/258663002/diff/100001/base/android/sys_utils_...
base/android/sys_utils_android.cc:34: }  // namespace base
On 2014/06/11 21:02:40, brettw wrote:
> Blank line above here

Done.

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc#n...
base/base_switches.cc:21: const char kEnableLowEndDeviceMode[] =
"enable-low-end-device-mode";
On 2014/06/11 21:02:40, brettw wrote:
> Can you align the = on this one and the next one you added for consistency?
> (unless it makes it >80 cols)

Done.

[c.shu, 2014-06-12 21:18:11]

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/258663002/diff/100001/base/base_switches.cc#n...
base/base_switches.cc:71: const char kDetectLowEndDevice[] =
"detect-low-end-device";
On 2014/06/11 21:02:40, brettw wrote:
> I'm not super excited about having 3 arguments to control this. Is it possible
> to do something like:
> --low-end-device-mode=0  meaning not low end
> --low-end-device-mode=1  meaning low end
> with undefined/empty meaning autodetect.

I agree, brettw. I have a concern though the
enable-low-end-device/disable-low-end-device are already used on Android. Would
it cause some problem for some scripts if we changed the behavior?

[brettw, 2014-06-12 21:58:11]

I don't know how they're used on Android. Can't we update those uses?

[c.shu, 2014-06-12 22:02:44]

On 2014/06/12 21:58:11, brettw wrote:
> I don't know how they're used on Android. Can't we update those uses?

I can give it a try.

[brettw, 2014-06-18 19:37:30]

LGTM with more comment:

https://codereview.chromium.org/258663002/diff/160001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/258663002/diff/160001/base/sys_info.h#newcode133
base/sys_info.h:133: // Returns true iff this is a low-end device.
iff -> if. Easier to read and no less precise in this case. Can you write a
sentence here about what a "low end device" means and what one might want to do
with this information?

[c.shu, 2014-06-18 19:42:00]

On 2014/06/18 19:37:30, brettw wrote:
> LGTM with more comment:
> 
> https://codereview.chromium.org/258663002/diff/160001/base/sys_info.h
> File base/sys_info.h (right):
> 
>
https://codereview.chromium.org/258663002/diff/160001/base/sys_info.h#newcode133
> base/sys_info.h:133: // Returns true iff this is a low-end device.
> iff -> if. Easier to read and no less precise in this case. Can you write a
> sentence here about what a "low end device" means and what one might want to
do
> with this information?

Thanks for the review. I will take care of it.

[c.shu, 2014-06-19 13:57:00]

Can any chrome and ui owners approve the trivial changes there? Thanks in
advance.

[Yaron, 2014-06-23 18:48:00]

https://codereview.chromium.org/258663002/diff/200001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/258663002/diff/200001/base/android/java/src/o...
base/android/java/src/org/chromium/base/SysUtils.java:35: * Note: the only
reason this is public is for testability reason.
I don't see that this is actually true. Please remove the comment from
"Note:..." and on and mark this as private.

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils.cc
File base/android/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils....
base/android/sys_utils.cc:32: bool SysInfo::IsLowEndDevice() {
I think this should move to base/sys_info_android.cc since there's already code
there implementing SysInfo

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils.h
File base/android/sys_utils.h (right):

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils....
base/android/sys_utils.h:5: #ifndef BASE_SYS_UTILS_H_
why is the header guard changed? I'm a little confused by reitveld since it's
marked as "D" but appears to be an edit.

[c.shu, 2014-06-23 18:56:40]

Thanks for the review, Yaron. I will post an update soon.

https://codereview.chromium.org/258663002/diff/200001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/258663002/diff/200001/base/android/java/src/o...
base/android/java/src/org/chromium/base/SysUtils.java:35: * Note: the only
reason this is public is for testability reason.
On 2014/06/23 18:48:00, Yaron wrote:
> I don't see that this is actually true. Please remove the comment from
> "Note:..." and on and mark this as private.

This is kind of out of the scope of this CL but I will take care of that.

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils.cc
File base/android/sys_utils.cc (right):

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils....
base/android/sys_utils.cc:32: bool SysInfo::IsLowEndDevice() {
On 2014/06/23 18:48:00, Yaron wrote:
> I think this should move to base/sys_info_android.cc since there's already
code
> there implementing SysInfo

sounds good.

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils.h
File base/android/sys_utils.h (right):

https://codereview.chromium.org/258663002/diff/200001/base/android/sys_utils....
base/android/sys_utils.h:5: #ifndef BASE_SYS_UTILS_H_
On 2014/06/23 18:48:00, Yaron wrote:
> why is the header guard changed? I'm a little confused by reitveld since it's
> marked as "D" but appears to be an edit.

This file was deleted in the first several iterations and restored in the late
versions. No meaningful change for removing "_ANDROID". I will restore it.

[c.shu, 2014-06-25 20:49:02]

There are a few trivial changes in chrome and ui that don't have lgtm but jochen
has sort of pre-approved them. I am using the TBR to get the CL through.

[c.shu, 2014-06-25 20:49:14]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-06-25 20:50:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/c.shu@samsung.com/258663002/220001

[Nico, 2014-06-25 20:52:03]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2014-06-25 20:52:15]

Please see "[chromium-dev] Reminder: reviewer unresponsiveness is not an
acceptable reason to TBR [Was: When can I use TBR ?]".

If Jochen likes this, he can approve it.

[c.shu, 2014-06-25 20:59:20]

On 2014/06/25 20:52:15, Nico (away) wrote:
> Please see "[chromium-dev] Reminder: reviewer unresponsiveness is not an
> acceptable reason to TBR [Was: When can I use TBR ?]".
> 
> If Jochen likes this, he can approve it.

What left in the CL that need lgtm are the changes in chrome and ui that replace
android::SysUtils::IsLowEndDevice() with SysInfo::IsLowEndDevice(). I think they
are trivial and doesn't hurt to TBR.

[jochen (gone - plz use gerrit), 2014-06-26 14:54:46]

lgtm

[c.shu, 2014-06-26 16:51:59]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-06-26 16:53:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/c.shu@samsung.com/258663002/220001

[commit-bot: I haz the power, 2014-06-26 16:55:31]

Change committed as 280024