Let Android port access properly sized images

Let Android port access properly sized images

Depends on assets in http://crrev.com/15136005, which had to be separated because of the trybots.

With the way things are currently set up, Chrome on Android can't access properly
sized image resources:
* The APK only packages the mostly unused 100% resources (none of
  the larger 200% assets)
* The bitmaps don't scale properly when they're converted to Java bitmaps because
  the Java code knows nothing about the resource's expected dimensions on different
  devices
* The Chromium resource loading system doesn't allow us to take advantage of
  Android's resource loading schemes (drawable directories for different screen
  sizes, RTL support, etc.).

Instead of #ifdefing out practically everything in the theme_resources.grd file,
this CL introduces a map between the chromium resource IDs and Android drawables,
allowing us to load up the correctly sized Android assets.

The template system is bootstrapped with the InfoBar and WebsiteSettingsUI icon
assets.

TBR=jochen
BUG=237034, 238668
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201328

[gone, 2013-05-14 21:06:36]

Strangely, the issue I was having with the map being incorrect went away after a
full rebuild.  There might be an issue where the APK is not rebuilt after the
generated Java file is created, but I don't know what that would be caused by. 
PTAL at this CL when you've got a chance.

[joth, 2013-05-15 17:19:24]

+benm

[gone, 2013-05-16 00:33:02]

Actually having some issues getting the GYP changes correct... It seems that
changing the resource_id.h file properly kicks off a rebuild of ResourceID.java,
but the files that _should_ depend on ResourceID.java don't get built properly. 
Ideas?

[gone, 2013-05-16 01:12:15]

On 2013/05/16 00:33:02, dfalcantara wrote:
> Actually having some issues getting the GYP changes correct... It seems that
> changing the resource_id.h file properly kicks off a rebuild of
ResourceID.java,
> but the files that _should_ depend on ResourceID.java don't get built
properly. 
> Ideas?

Did some digging and found that the WebView gets around this by explicitly
declaring generated template files in its Android.mk file -- but that doesn't
exist for Chromium.

[cjhopman, 2013-05-16 19:52:48]

On 2013/05/16 00:33:02, dfalcantara wrote:
> Actually having some issues getting the GYP changes correct... It seems that
> changing the resource_id.h file properly kicks off a rebuild of
ResourceID.java,
> but the files that _should_ depend on ResourceID.java don't get built
properly. 
> Ideas?

This is a general problem with the way these java files are generated (e.g. with
java_cpp_template.gypi).

[gone, 2013-05-16 20:09:33]

On 2013/05/16 19:52:48, cjhopman wrote:
> On 2013/05/16 00:33:02, dfalcantara wrote:
> > Actually having some issues getting the GYP changes correct... It seems that
> > changing the resource_id.h file properly kicks off a rebuild of
> ResourceID.java,
> > but the files that _should_ depend on ResourceID.java don't get built
> properly. 
> > Ideas?
> 
> This is a general problem with the way these java files are generated (e.g.
with
> java_cpp_template.gypi).

Ah well... in that case I'll just forge ahead with it then.

[gone, 2013-05-16 20:26:42]

Adding Tommy here because he used templates for
https://codereview.chromium.org/12313075

[gone, 2013-05-16 22:48:14]

Alright, uploaded a fixed java_cpp_template.gypi file that properly triggers
rebuilding.  PTAL.

[gone, 2013-05-16 23:16:13]

On 2013/05/16 22:48:14, dfalcantara wrote:
> Alright, uploaded a fixed java_cpp_template.gypi file that properly triggers
> rebuilding.  PTAL.

Also ended up squashing the followup CL because I had to separate out the PNG
images.

[gone, 2013-05-17 17:31:53]

Haven't heard anything from the WebView guys so I moved them to CC.  Adding
yfriedman since he wrote the Android WebsiteSettingsUI bits.

[joth, 2013-05-17 21:05:05]

AFAICT as this is only chrome/ code it won't affect webview... thanks for ccing
us though as it's useful to keep abreast of resource changes.

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
File chrome/browser/android/resource_id.h (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
chrome/browser/android/resource_id.h:6: #define
CHROME_BROWSER_ANDROID_RESOURCE_ID_H_
normally don't use include guards in these ID list headers. (comment that
include guard is left out on purpose)

[gone, 2013-05-17 21:06:25]

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
File chrome/browser/android/resource_id.h (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
chrome/browser/android/resource_id.h:6: #define
CHROME_BROWSER_ANDROID_RESOURCE_ID_H_
On 2013/05/17 21:05:05, joth wrote:
> normally don't use include guards in these ID list headers. (comment that
> include guard is left out on purpose)

Yeah, turns out that removing the guards still works in these cases, though.  As
a bonus it doesn't trip up the presubmit scripts...

[gone, 2013-05-17 22:04:31]

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
File chrome/browser/android/resource_id.h (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
chrome/browser/android/resource_id.h:6: #define
CHROME_BROWSER_ANDROID_RESOURCE_ID_H_
On 2013/05/17 21:06:25, dfalcantara wrote:
> On 2013/05/17 21:05:05, joth wrote:
> > normally don't use include guards in these ID list headers. (comment that
> > include guard is left out on purpose)
> 
> Yeah, turns out that removing the guards still works in these cases, though. 
As
> a bonus it doesn't trip up the presubmit scripts...

Er, ADDING the guards still works.  Either way works fine.

[Yaron, 2013-05-17 22:36:16]

https://chromiumcodereview.appspot.com/14772036/diff/42001/build/android/java...
File build/android/java_cpp_template.gypi (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/build/android/java...
build/android/java_cpp_template.gypi:44: 'additional_input_paths': [
Can you add a comment about this (something like: "ensure dependents are rebuilt
when sources to this rule change")

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
File chrome/android/java/ResourceID.template (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/ResourceID.template:18: } else {
No need for "else" since you return above.

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/ResourceID.template:19: assert false : "enumeratedID was out
of range.";
add the failing id to the string?

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:68:
private void addSection(int enumeratedIconID, String headline, String
description) {
Nit: "ID" should be "Id" througout

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
File chrome/browser/android/resource_mapper.cc (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:14: int
ResourceMapper::MapFromChromiumID(int resource_id) {
So every time you map and id you reconstruct the map? That seems wasteful. Can
we lazily construct it once?

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/chrome_brow...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/chrome_brow...
chrome/chrome_browser.gypi:2801: 'resource_id_java',
You shouldn't need this here. Only _java targets should have to depend o nthat.

[gone, 2013-05-20 18:48:10]

Accidentally re-based while trying to chase down some other thing :/

https://chromiumcodereview.appspot.com/14772036/diff/42001/build/android/java...
File build/android/java_cpp_template.gypi (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/build/android/java...
build/android/java_cpp_template.gypi:44: 'additional_input_paths': [
On 2013/05/17 22:36:16, Yaron wrote:
> Can you add a comment about this (something like: "ensure dependents are
rebuilt
> when sources to this rule change")

Done.

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
File chrome/android/java/ResourceID.template (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/ResourceID.template:18: } else {
On 2013/05/17 22:36:16, Yaron wrote:
> No need for "else" since you return above.

Done.

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/ResourceID.template:19: assert false : "enumeratedID was out
of range.";
On 2013/05/17 22:36:16, Yaron wrote:
> add the failing id to the string?

Done.

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:68:
private void addSection(int enumeratedIconID, String headline, String
description) {
On 2013/05/17 22:36:16, Yaron wrote:
> Nit: "ID" should be "Id" througout

Usage in the codebase seems really inconsistent :/

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
File chrome/browser/android/resource_mapper.cc (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:14: int
ResourceMapper::MapFromChromiumID(int resource_id) {
On 2013/05/17 22:36:16, Yaron wrote:
> So every time you map and id you reconstruct the map? That seems wasteful. Can
> we lazily construct it once?

Done.  Would it make more sense as a static member variable, though?  Not seeing
static maps defined in header files or anything...

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/chrome_brow...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/chrome_brow...
chrome/chrome_browser.gypi:2801: 'resource_id_java',
On 2013/05/17 22:36:16, Yaron wrote:
> You shouldn't need this here. Only _java targets should have to depend o
nthat.

Done.

[Yaron, 2013-05-20 20:54:25]

lgtm

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://chromiumcodereview.appspot.com/14772036/diff/42001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:68:
private void addSection(int enumeratedIconID, String headline, String
description) {
On 2013/05/20 18:48:10, dfalcantara wrote:
> On 2013/05/17 22:36:16, Yaron wrote:
> > Nit: "ID" should be "Id" througout
> 
> Usage in the codebase seems really inconsistent :/

Cases with ID are wrong:
http://source.android.com/source/code-style.html#treat-acronyms-as-words (also
according to internal style guide)

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
File chrome/browser/android/resource_mapper.cc (right):

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:15: std::map<int, int> g_id_map;
Don't indent scope under namespace

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:20: int next_id = 0;
How about DCHECK(g_id_map.empty());

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:25: }
  // namespace

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
File chrome/browser/android/resource_mapper.h (right):

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
Nit: drop "(c)"

[gone, 2013-05-20 21:28:04]

Added jochen@ as TBR for the single-line change in chrome.gyp.

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
File chrome/browser/android/resource_mapper.cc (right):

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:15: std::map<int, int> g_id_map;
On 2013/05/20 20:54:25, Yaron wrote:
> Don't indent scope under namespace

Done.

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:20: int next_id = 0;
On 2013/05/20 20:54:25, Yaron wrote:
> How about DCHECK(g_id_map.empty());

Done.

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.cc:25: }
On 2013/05/20 20:54:25, Yaron wrote:
>   // namespace

Done.

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
File chrome/browser/android/resource_mapper.h (right):

https://chromiumcodereview.appspot.com/14772036/diff/54001/chrome/browser/and...
chrome/browser/android/resource_mapper.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/05/20 20:54:25, Yaron wrote:
> Nit: drop "(c)"

Done.

[commit-bot: I haz the power, 2013-05-21 01:11:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/14772036/67001

[commit-bot: I haz the power, 2013-05-21 01:42:00]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 02:30:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/14772036/80001

[commit-bot: I haz the power, 2013-05-21 03:10:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/14772036/92001

[commit-bot: I haz the power, 2013-05-21 10:01:59]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-21 15:26:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/14772036/92001

[commit-bot: I haz the power, 2013-05-21 17:32:33]

Change committed as 201328