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

Issue 59033008: android: Make org.chromium.base.SysUtils.isLowEndDevice() work without native code. (Closed)

Created:
7 years, 1 month ago by digit1
Modified:
7 years, 1 month ago
CC:
chromium-reviews, craigdh+watch_chromium.org, jam, joi+watch-content_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, darin-cc_chromium.org, klundberg+watch_chromium.org, erikwright+watch_chromium.org, frankf+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

android: Make org.chromium.base.SysUtils.isLowEndDevice() work without native code. This patch modifies the implementation if SysUtils.isLowEndDevice() to not rely on any native code, which is required to run it before the native libraries are actually loaded (e.g. when running certain tests). + Simplify the content linker that doesn't need a specific native method anymore to replicate the yet-unloaded native SysUtils::IsLowEndDevice(). BUG=309926 R=dtrainor@chromium.org,tedchoc@chromium.org,frankf@chromium.org,bulach@chromium.org TEST=build/android/test_runner.py linker Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233739

Patch Set 1 #

Total comments: 2

Patch Set 2 : Findbugs issue + add new base unittest. #

Patch Set 3 : Check the result of amountOfPhsysicalMemoryKB() properly. #

Total comments: 8

Patch Set 4 : Formatting #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -128 lines) Patch
M base/android/java/src/org/chromium/base/SysUtils.java View 1 2 3 1 chunk +85 lines, -3 lines 2 comments Download
M base/android/sys_utils.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M base/android/sys_utils.cc View 1 2 chunks +7 lines, -19 lines 0 comments Download
A base/android/sys_utils_unittest.cc View 1 1 chunk +23 lines, -0 lines 0 comments Download
M base/base.gyp View 1 1 chunk +1 line, -0 lines 0 comments Download
M build/android/pylib/linker/setup.py View 1 chunk +1 line, -2 lines 0 comments Download
M build/android/pylib/linker/test_case.py View 1 chunk +0 lines, -64 lines 0 comments Download
M content/common/android/linker/linker_jni.cc View 3 chunks +1 line, -36 lines 0 comments Download
M content/public/android/java/src/org/chromium/content/app/Linker.java View 3 chunks +3 lines, -4 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
digit1
PTAL. This definitely needs a proper unit test, i.e. one that would actually use sysconf(_SC_PHYS_PAGES) ...
7 years, 1 month ago (2013-11-06 11:11:56 UTC) #1
bulach
lgtm but please let others chime in.. thanks david! I have one suggestion for testing ...
7 years, 1 month ago (2013-11-06 12:16:39 UTC) #2
digit1
Very good suggestion, I'm currently addressing the findbugs issue, will upload something fresh soon.
7 years, 1 month ago (2013-11-06 12:24:35 UTC) #3
digit1
Done, see the new SysUtils.AmountOfPhysicalMemory base unittests :-) To answer the /proc/meminfo question, I assume ...
7 years, 1 month ago (2013-11-06 13:58:51 UTC) #4
David Trainor- moved to gerrit
On 2013/11/06 13:58:51, digit1 wrote: > Done, see the new SysUtils.AmountOfPhysicalMemory base unittests :-) > ...
7 years, 1 month ago (2013-11-06 17:45:29 UTC) #5
digit1
mark@chromium.org: Please review changes in jar@chromium.org: Please review changes in base/base.gyp Thanks.
7 years, 1 month ago (2013-11-06 17:48:24 UTC) #6
Mark Mentovai
LGTM. You don’t need two people to review a trivial one-line addition to a .gyp ...
7 years, 1 month ago (2013-11-06 17:54:08 UTC) #7
digit1
On 2013/11/06 17:54:08, Mark Mentovai wrote: > LGTM. You don’t need two people to review ...
7 years, 1 month ago (2013-11-06 18:03:25 UTC) #8
Mark Mentovai
Well, I did glance at the rest of the changes in base, but I did ...
7 years, 1 month ago (2013-11-06 18:04:48 UTC) #9
Ted C
LGTM (w/ a style nits) Yay for not having to pass in a context. https://codereview.chromium.org/59033008/diff/150001/base/android/java/src/org/chromium/base/SysUtils.java ...
7 years, 1 month ago (2013-11-06 18:07:52 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/59033008/150001
7 years, 1 month ago (2013-11-06 18:09:57 UTC) #11
frankf
late lgtm. Brilliant. @digit: how about re-implementing all other awkward java APIs :)
7 years, 1 month ago (2013-11-06 18:56:11 UTC) #12
digit1
https://codereview.chromium.org/59033008/diff/150001/base/android/java/src/org/chromium/base/SysUtils.java File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/59033008/diff/150001/base/android/java/src/org/chromium/base/SysUtils.java#newcode39 base/android/java/src/org/chromium/base/SysUtils.java:39: * an error trying to access the information. Done. ...
7 years, 1 month ago (2013-11-06 20:52:12 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/59033008/350001
7 years, 1 month ago (2013-11-07 15:08:15 UTC) #14
commit-bot: I haz the power
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&number=99241
7 years, 1 month ago (2013-11-07 20:09:00 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/59033008/350001
7 years, 1 month ago (2013-11-07 21:47:56 UTC) #16
commit-bot: I haz the power
Change committed as 233739
7 years, 1 month ago (2013-11-08 01:05:02 UTC) #17
jdduke (slow)
https://codereview.chromium.org/59033008/diff/350001/base/android/java/src/org/chromium/base/SysUtils.java File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/59033008/diff/350001/base/android/java/src/org/chromium/base/SysUtils.java#newcode44 base/android/java/src/org/chromium/base/SysUtils.java:44: public static int amountOfPhysicalMemoryKB() { If the query is ...
7 years, 1 month ago (2013-11-08 16:41:00 UTC) #18
digit1
https://codereview.chromium.org/59033008/diff/350001/base/android/java/src/org/chromium/base/SysUtils.java File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/59033008/diff/350001/base/android/java/src/org/chromium/base/SysUtils.java#newcode44 base/android/java/src/org/chromium/base/SysUtils.java:44: public static int amountOfPhysicalMemoryKB() { This is covered by ...
7 years, 1 month ago (2013-11-08 16:49:05 UTC) #19
jdduke (slow)
7 years, 1 month ago (2013-11-08 16:53:32 UTC) #20
Message was sent while issue was closed.
On 2013/11/08 16:49:05, digit1 wrote:
>
https://codereview.chromium.org/59033008/diff/350001/base/android/java/src/or...
> File base/android/java/src/org/chromium/base/SysUtils.java (right):
> 
>
https://codereview.chromium.org/59033008/diff/350001/base/android/java/src/or...
> base/android/java/src/org/chromium/base/SysUtils.java:44: public static int
> amountOfPhysicalMemoryKB() {
> This is covered by lines 50-55, in short: it's too much pain for too little
> benefits. See https://chromiumcodereview.appspot.com/46673008/ for an example.

Of course, sorry I should have read further! Thanks.

Powered by Google App Engine
This is Rietveld 408576698