Upstream ResourceManager

Upstream ResourceManager

And expose ResourceManager through the Compositor interface.
https://chrome-internal-review.googlesource.com/184331/ is a counter part in downstream.

BUG=426939
Committed: https://crrev.com/d5de7e4469af397027ca13927c4426baec270d4e
Cr-Commit-Position: refs/heads/master@{#306830}

[Jaekyun Seok (inactive), 2014-11-17 13:41:37]

jaekyun@chromium.org changed reviewers:
+ changwan@chromium.org, dtrainor@chromium.org

[Jaekyun Seok (inactive), 2014-11-17 13:41:39]

Please review this CL to upstream ResourceManager.

[David Trainor- moved to gerrit, 2014-11-18 18:53:54]

Looking good!  A few nits (specifically around using ResourceKind instead of
ResourceType), but in general lgtm.  You'll have to find owners for a lot of
these files though.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/a...
File content/browser/android/resource_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/a...
content/browser/android/resource_manager_impl.cc:36: return
gfx::Rect(aperture.x() * left_scale, aperture.y() * top_scale,
New line for the second parameter?

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/r...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/r...
content/browser/renderer_host/compositor_impl_android.cc:177:
resource_manager_.reset(NULL);
nullptr?

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content_public/resources/BitmapResources.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/BitmapResources.java:20:
public class BitmapResources {
Does anything actually use this file?

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java:92:
public void preloadStaticResources(Context context, StaticResourcePreloads
preloads) {
Can we change this to something like preloadResources(int resType, int[]
syncIds, int[] asyncIds)?  What do you think?

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java:14:
private final int mResId;
Storing the resId feels implementation specific (ie: The LayerTitleCache needs
it, but not the ResourceManager).  I'm fine with it being here for now, but we
should see if there's a better way to do this in the future.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java:16:
private Rect mSize = new Rect();
This should be final so we can't call new again and churn GC.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java:17:
private static final Rect EMPTY_RECT = new Rect();
Static should go up top (sorry I missed that downstream).

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
content/public/browser/android/resource_manager.h:25: enum ResourceKind {
Why use Kind instead of Type?

[Jaekyun Seok (inactive), 2014-11-18 23:34:02]

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/a...
File content/browser/android/resource_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/a...
content/browser/android/resource_manager_impl.cc:36: return
gfx::Rect(aperture.x() * left_scale, aperture.y() * top_scale,
On 2014/11/18 18:53:53, David Trainor wrote:
> New line for the second parameter?

This is a result from "git cl format".

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/r...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/browser/r...
content/browser/renderer_host/compositor_impl_android.cc:177:
resource_manager_.reset(NULL);
On 2014/11/18 18:53:53, David Trainor wrote:
> nullptr?

Done.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content_public/resources/BitmapResources.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/BitmapResources.java:20:
public class BitmapResources {
On 2014/11/18 18:53:53, David Trainor wrote:
> Does anything actually use this file?

No, I will remove this file.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java:92:
public void preloadStaticResources(Context context, StaticResourcePreloads
preloads) {
On 2014/11/18 18:53:53, David Trainor wrote:
> Can we change this to something like preloadResources(int resType, int[]
> syncIds, int[] asyncIds)?  What do you think?

Sounds good. I will change codes.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java:14:
private final int mResId;
On 2014/11/18 18:53:53, David Trainor wrote:
> Storing the resId feels implementation specific (ie: The LayerTitleCache needs
> it, but not the ResourceManager).  I'm fine with it being here for now, but we
> should see if there's a better way to do this in the future.

I will do that later.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java:16:
private Rect mSize = new Rect();
On 2014/11/18 18:53:53, David Trainor wrote:
> This should be final so we can't call new again and churn GC.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content_public/resources/dynamics/BitmapDynamicResource.java:17:
private static final Rect EMPTY_RECT = new Rect();
On 2014/11/18 18:53:53, David Trainor wrote:
> Static should go up top (sorry I missed that downstream).

Done.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
content/public/browser/android/resource_manager.h:25: enum ResourceKind {
On 2014/11/18 18:53:53, David Trainor wrote:
> Why use Kind instead of Type?

Because ResourceType is already being used in
https://cs.corp.google.com/#clankium/src/content/public/common/resource_type.h .

[Jaekyun Seok (inactive), 2014-11-18 23:40:25]

jaekyun@chromium.org changed reviewers:
+ jdduke@chromium.org, mkosiba@chromium.org, sievers@chromium.org,
tedchoc@chromium.org

[Jaekyun Seok (inactive), 2014-11-18 23:40:25]

Please review this CL.

This change is to upstream ResourceManager and expose it through Compositor.

Thanks,

[jdduke (slow), 2014-11-19 00:09:16]

Yay!

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
File content/browser/android/resource_manager_impl.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/android/resource_manager_impl.h:27: virtual
~ResourceManagerImpl();
Nit: ~ResourceManagerImpl() override;

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
File content/browser/android/ui_resource_android_impl.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/android/ui_resource_android_impl.h:8: #include <jni.h>
Don't think we need the <jni.h> include.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/android/ui_resource_android_impl.h:28: virtual
~UIResourceAndroidImpl();
Nit: ~UIResourceAndroidImpl() override;

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/renderer_host/compositor_impl_android.h:68: virtual
ResourceManager* GetResourceManager() override;
Would be great if we could return a reference.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/renderer_host/compositor_impl_android.h:149:
scoped_ptr<content::ResourceManagerImpl> resource_manager_;
Does this need to be a scoped_ptr? Could it be a value member variable? Or does
it need to be destroyed before other explicit destruction operations?

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
File content/public/browser/android/compositor.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/compositor.h:74: virtual ResourceManager*
GetResourceManager() = 0;
dtrainor@: So eventually we'll call this, and get rid of the
SystemUIResourceManager?

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:8: #include <android/bitmap.h>
Can we get away with not including these headers? (bitmap.h, jni.h)

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:11: #include "base/id_map.h"
id_map.h looks unused.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:14: #include
"content/public/browser/android/ui_resource_android.h"
 Also, UIResourceAndroid is forward declared, so do we need to include it?

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:15: #include
"third_party/skia/include/core/SkBitmap.h"
I don't see SkBitmap used here.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
File content/public/browser/android/ui_resource_android.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/ui_resource_android.h:12: #include
"content/common/content_export.h"
Lots of unused headers here, can we get rid of them? We should only include what
we use.

[David Trainor- moved to gerrit, 2014-11-19 00:47:53]

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
content/public/browser/android/resource_manager.h:25: enum ResourceKind {
On 2014/11/18 23:34:02, Jaekyun Seok wrote:
> On 2014/11/18 18:53:53, David Trainor wrote:
> > Why use Kind instead of Type?
> 
> Because ResourceType is already being used in
> https://cs.corp.google.com/#clankium/src/content/public/common/resource_type.h
.

Ah good point!  Can we use ANDROID_RESOURCE_TYPE then?  RESOURCE_KIND feels
awkward.

[Jaekyun Seok (inactive), 2014-11-19 02:41:38]

PTAL.

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/80001/content/public/br...
content/public/browser/android/resource_manager.h:25: enum ResourceKind {
On 2014/11/19 00:47:53, David Trainor wrote:
> On 2014/11/18 23:34:02, Jaekyun Seok wrote:
> > On 2014/11/18 18:53:53, David Trainor wrote:
> > > Why use Kind instead of Type?
> > 
> > Because ResourceType is already being used in
> >
https://cs.corp.google.com/#clankium/src/content/public/common/resource_type.h
> .
> 
> Ah good point!  Can we use ANDROID_RESOURCE_TYPE then?  RESOURCE_KIND feels
> awkward.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
File content/browser/android/resource_manager_impl.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/android/resource_manager_impl.h:27: virtual
~ResourceManagerImpl();
On 2014/11/19 00:09:15, jdduke wrote:
> Nit: ~ResourceManagerImpl() override;

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
File content/browser/android/ui_resource_android_impl.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/android/ui_resource_android_impl.h:8: #include <jni.h>
On 2014/11/19 00:09:15, jdduke wrote:
> Don't think we need the <jni.h> include.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/android/ui_resource_android_impl.h:28: virtual
~UIResourceAndroidImpl();
On 2014/11/19 00:09:15, jdduke wrote:
> Nit: ~UIResourceAndroidImpl() override;

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/renderer_host/compositor_impl_android.h:68: virtual
ResourceManager* GetResourceManager() override;
On 2014/11/19 00:09:15, jdduke wrote:
> Would be great if we could return a reference.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/browser/...
content/browser/renderer_host/compositor_impl_android.h:149:
scoped_ptr<content::ResourceManagerImpl> resource_manager_;
On 2014/11/19 00:09:15, jdduke wrote:
> Does this need to be a scoped_ptr? Could it be a value member variable? Or
does
> it need to be destroyed before other explicit destruction operations?

I will declare this as a value member variable.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:8: #include <android/bitmap.h>
On 2014/11/19 00:09:16, jdduke wrote:
> Can we get away with not including these headers? (bitmap.h, jni.h)

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:11: #include "base/id_map.h"
On 2014/11/19 00:09:16, jdduke wrote:
> id_map.h looks unused.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:14: #include
"content/public/browser/android/ui_resource_android.h"
On 2014/11/19 00:09:16, jdduke wrote:
>  Also, UIResourceAndroid is forward declared, so do we need to include it?

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/resource_manager.h:15: #include
"third_party/skia/include/core/SkBitmap.h"
On 2014/11/19 00:09:16, jdduke wrote:
> I don't see SkBitmap used here.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
File content/public/browser/android/ui_resource_android.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/ui_resource_android.h:12: #include
"content/common/content_export.h"
On 2014/11/19 00:09:16, jdduke wrote:
> Lots of unused headers here, can we get rid of them? We should only include
what
> we use.

Done.

https://chromiumcodereview.appspot.com/731133002/diff/100001/content/public/b...
content/public/browser/android/ui_resource_android.h:12: #include
"content/common/content_export.h"
On 2014/11/19 00:09:16, jdduke wrote:
> Lots of unused headers here, can we get rid of them? We should only include
what
> we use.

Done.

[mkosiba (inactive), 2014-11-19 09:39:41]

android_webview/ LGTM

[David Trainor- moved to gerrit, 2014-11-19 17:57:19]

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/public/a...
File
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java
(right):

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/public/a...
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java:92:
public void preloadStaticResources(Context context, int[] syncIds, int[]
asyncIds) {
Remove context parameter, pass in a resource type parameter so we can call
mResourceLoaders.get(type) then call preload on that.  We don't need to assume
static.

[jdduke (slow), 2014-11-19 18:26:48]

The non-Java content/ portion lgtm with a couple nits (I'm not really an owner
for these files technically, so sorry for the drive-by =/).

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/browser/...
File content/browser/android/resource_manager_impl.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/browser/...
content/browser/android/resource_manager_impl.h:12: #include
"third_party/skia/include/core/SkBitmap.h"
Looks like both gfx::Rect and SkBitmap are unused here? Can we remove the
includes?

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/browser/...
content/browser/android/resource_manager_impl.h:23:
ResourceManagerImpl(UIResourceProvider* ui_resource_provider);
Nit: explicit.

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/browser/...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/browser/...
content/browser/renderer_host/compositor_impl_android.h:38: class
ResourceManager;
Nit: We're already including the header, no need to forward declare (same for
UIResourceProvider).

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/public/b...
File content/public/browser/android/compositor.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/public/b...
content/public/browser/android/compositor.h:11: #include
"content/public/browser/android/resource_manager.h"
Do we need this include if we forward declare the class?

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/public/b...
File content/public/browser/android/resource_manager.h (right):

https://chromiumcodereview.appspot.com/731133002/diff/140001/content/public/b...
content/public/browser/android/resource_manager.h:36: gfx::Rect Border(const
gfx::Size& bounds);
Hmm, it's a little odd to declare this here, but define in a different
(non-public) source file. Don't have a strong opinion here though.

[David Trainor- moved to gerrit, 2014-11-20 00:55:19]

Talked to tedchoc@ and he suggested moving this to ui/android/resources
(ui/android/java/.../resources for the java components).

[Jaekyun Seok (inactive), 2014-11-21 12:35:33]

PTAL.

I moved codes under ui/android/, but I have the following ambiguous things.

1. Is ui/android/ a right place for native files because I can see some android
native files in ui/base/android/.

2. Should we sill expose ResourceManager through Compositor?

3. I'm struggling to fix build breakage of android_clang_dbg, but I failed. I
inserted dependency on ui/android/ui_android.gyp:ui_android as many as possible
to fix the following linking error:

FAILED: if [ ! -e lib/libcontent.cr.so -o ! -e lib/libcontent.cr.so.TOC ]; then
/b/build/goma/gomacc ../../third_party/llvm-build/Release+Asserts/bin/clang++
-shared -Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC
-Wl,--dynamic-linker,/system/bin/linker -fuse-ld=gold
--gcc-toolchain=/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/..
-Wl,--no-undefined -target arm-linux-androideabi
--sysroot=/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-nostdlib -Wl,--icf=safe
-L/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/libs/armeabi-v7a/thumb
-Wl,-shared,-Bsymbolic
/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/lib/crtbegin_so.o
-Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -o lib/libcontent.cr.so
-Wl,-soname=libcontent.cr.so @lib/libcontent.cr.so.rsp && { readelf -d
lib/libcontent.cr.so | grep SONAME ; nm -gD -f p lib/libcontent.cr.so | cut
-f1-2 -d' '; } > lib/libcontent.cr.so.TOC; else /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -shared -Wl,-z,now
-Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,noexecstack -fPIC
-Wl,--dynamic-linker,/system/bin/linker -fuse-ld=gold
--gcc-toolchain=/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/..
-Wl,--no-undefined -target arm-linux-androideabi
--sysroot=/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-nostdlib -Wl,--icf=safe
-L/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/libs/armeabi-v7a/thumb
-Wl,-shared,-Bsymbolic
/b/build/slave/android_clang_dbg/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/lib/crtbegin_so.o
-Wl,--warn-shared-textrel -Wl,-O1 -Wl,--as-needed -o lib/libcontent.cr.so
-Wl,-soname=libcontent.cr.so @lib/libcontent.cr.so.rsp && { readelf -d
lib/libcontent.cr.so | grep SONAME ; nm -gD -f p lib/libcontent.cr.so | cut
-f1-2 -d' '; } > lib/libcontent.cr.so.tmp && if ! cmp -s
lib/libcontent.cr.so.tmp lib/libcontent.cr.so.TOC; then mv
lib/libcontent.cr.so.tmp lib/libcontent.cr.so.TOC ; fi; fi
obj/content/app/android/content.library_loader_hooks.o:../../content/app/android/library_loader_hooks.cc:function
content::EnsureJniRegistered(_JNIEnv*): error: undefined reference to
'ui::android::RegisterUIAndroidJni(_JNIEnv*)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Does anyone know how to fix it? And is there any easy way to reproduce the build
breakage on my local computer?

https://codereview.chromium.org/731133002/diff/140001/content/browser/android...
File content/browser/android/resource_manager_impl.h (right):

https://codereview.chromium.org/731133002/diff/140001/content/browser/android...
content/browser/android/resource_manager_impl.h:12: #include
"third_party/skia/include/core/SkBitmap.h"
On 2014/11/19 18:26:48, jdduke wrote:
> Looks like both gfx::Rect and SkBitmap are unused here? Can we remove the
> includes?

Done.

https://codereview.chromium.org/731133002/diff/140001/content/browser/android...
content/browser/android/resource_manager_impl.h:23:
ResourceManagerImpl(UIResourceProvider* ui_resource_provider);
On 2014/11/19 18:26:48, jdduke wrote:
> Nit: explicit.

Done.

https://codereview.chromium.org/731133002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/731133002/diff/140001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:38: class
ResourceManager;
On 2014/11/19 18:26:48, jdduke wrote:
> Nit: We're already including the header, no need to forward declare (same for
> UIResourceProvider).

Done.

https://codereview.chromium.org/731133002/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java
(right):

https://codereview.chromium.org/731133002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/resources/ResourceManager.java:92:
public void preloadStaticResources(Context context, int[] syncIds, int[]
asyncIds) {
On 2014/11/19 17:57:19, David Trainor wrote:
> Remove context parameter, pass in a resource type parameter so we can call
> mResourceLoaders.get(type) then call preload on that.  We don't need to assume
> static.

Done.

https://codereview.chromium.org/731133002/diff/140001/content/public/browser/...
File content/public/browser/android/compositor.h (right):

https://codereview.chromium.org/731133002/diff/140001/content/public/browser/...
content/public/browser/android/compositor.h:11: #include
"content/public/browser/android/resource_manager.h"
On 2014/11/19 18:26:48, jdduke wrote:
> Do we need this include if we forward declare the class?

Done.

https://codereview.chromium.org/731133002/diff/140001/content/public/browser/...
File content/public/browser/android/resource_manager.h (right):

https://codereview.chromium.org/731133002/diff/140001/content/public/browser/...
content/public/browser/android/resource_manager.h:36: gfx::Rect Border(const
gfx::Size& bounds);
On 2014/11/19 18:26:48, jdduke wrote:
> Hmm, it's a little odd to declare this here, but define in a different
> (non-public) source file. Don't have a strong opinion here though.

These functions are being called from outside of ResourceManagerImpl, and so I
believe that we should keep these here.

[Jaekyun Seok (inactive), 2014-11-24 08:55:29]

PTAL.

I fixed the clang build failure.

[Ted C, 2014-11-24 19:09:55]

https://codereview.chromium.org/731133002/diff/200001/ui/android/resources/re...
File ui/android/resources/resource_manager.h (right):

https://codereview.chromium.org/731133002/diff/200001/ui/android/resources/re...
ui/android/resources/resource_manager.h:19: namespace android {
Do any of the other top level folders add a namespace?  I didn't see chromeos or
ios (or events, etc...).  So I would drop this android namespace unless there is
an explicit need for it.

https://codereview.chromium.org/731133002/diff/200001/ui/android/ui_android.gyp
File ui/android/ui_android.gyp (right):

https://codereview.chromium.org/731133002/diff/200001/ui/android/ui_android.g...
ui/android/ui_android.gyp:35: '../..',
?? this looks really generic.  Do we need to include all of this?

[Jaekyun Seok (inactive), 2014-11-24 23:36:56]

PTAL.

https://codereview.chromium.org/731133002/diff/200001/ui/android/resources/re...
File ui/android/resources/resource_manager.h (right):

https://codereview.chromium.org/731133002/diff/200001/ui/android/resources/re...
ui/android/resources/resource_manager.h:19: namespace android {
On 2014/11/24 19:09:55, Ted C wrote:
> Do any of the other top level folders add a namespace?  I didn't see chromeos
or
> ios (or events, etc...).  So I would drop this android namespace unless there
is
> an explicit need for it.

I will remove it.

https://codereview.chromium.org/731133002/diff/200001/ui/android/ui_android.gyp
File ui/android/ui_android.gyp (right):

https://codereview.chromium.org/731133002/diff/200001/ui/android/ui_android.g...
ui/android/ui_android.gyp:35: '../..',
On 2014/11/24 19:09:55, Ted C wrote:
> ?? this looks really generic.  Do we need to include all of this?

I referred to other gyp files like ui/accelerometer/ui_accelerometer.gyp. But
this seems unnecessary. I will remove it.

[Ted C, 2014-11-24 23:38:18]

On 2014/11/24 23:36:56, Jaekyun Seok wrote:
> PTAL.
> 
>
https://codereview.chromium.org/731133002/diff/200001/ui/android/resources/re...
> File ui/android/resources/resource_manager.h (right):
> 
>
https://codereview.chromium.org/731133002/diff/200001/ui/android/resources/re...
> ui/android/resources/resource_manager.h:19: namespace android {
> On 2014/11/24 19:09:55, Ted C wrote:
> > Do any of the other top level folders add a namespace?  I didn't see
chromeos
> or
> > ios (or events, etc...).  So I would drop this android namespace unless
there
> is
> > an explicit need for it.
> 
> I will remove it.
> 
>
https://codereview.chromium.org/731133002/diff/200001/ui/android/ui_android.gyp
> File ui/android/ui_android.gyp (right):
> 
>
https://codereview.chromium.org/731133002/diff/200001/ui/android/ui_android.g...
> ui/android/ui_android.gyp:35: '../..',
> On 2014/11/24 19:09:55, Ted C wrote:
> > ?? this looks really generic.  Do we need to include all of this?
> 
> I referred to other gyp files like ui/accelerometer/ui_accelerometer.gyp. But
> this seems unnecessary. I will remove it.

lgtm

[Jaekyun Seok (inactive), 2014-11-27 01:21:08]

Daniel, could you please review this CL, especially changes on content/*?

[no sievers, 2014-12-02 00:25:07]

https://codereview.chromium.org/731133002/diff/220001/ui/android/BUILD.gn
File ui/android/BUILD.gn (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/BUILD.gn#new...
ui/android/BUILD.gn:32: jni_package = "android"
This is a bit too generic. How about "ui_android"?

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS#newcode2
ui/android/DEPS:2: "+cc",
Can you add "-cc/blink"?

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS#newcode4
ui/android/DEPS:4: "+content/public/browser/android/ui_resource_provider.h",
As a follow-up, can we eliminate these base class interfaces (or move them to
ui/), so we don't have to depend on content/ here?

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/re...
File ui/android/resources/resource_manager.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/re...
ui/android/resources/resource_manager.h:47: gfx::Rect aperture;
nit: As a follow-up, more stuff that could be merged into the class itself now
that everything gets upstreamed.

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
File ui/android/resources/ui_resource_android.cc (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
ui/android/resources/ui_resource_android.cc:7: #include "base/logging.h"
nit: unused

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
ui/android/resources/ui_resource_android.cc:15: if (!bitmap_object)
Why do we need to handle this? Should it be DCHECK()? Or should it at least
return null?

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
File ui/android/resources/ui_resource_android.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
ui/android/resources/ui_resource_android.h:29: virtual ~UIResourceAndroid()
override;
nit: no 'virtual' if 'override'

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android.gyp
File ui/android/ui_android.gyp (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android.g...
ui/android/ui_android.gyp:42: 'jni_gen_package': 'ui',
Should this be "ui_android" also since this is its own component?

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_e...
File ui/android/ui_android_export.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_e...
ui/android/ui_android_export.h:5: #ifndef UI_BASE_UI_ANDROID_EXPORT_H_
UI_ANDROID_UI_ANDROID_EXPORT_H here and below

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_e...
ui/android/ui_android_export.h:13: #if defined(WIN32)
nit: WIN32 doesn't really make sense here since it's an Android-only folder.
Maybe just #error in here?

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
File ui/android/ui_android_jni_registrar.cc (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
ui/android/ui_android_jni_registrar.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit: "Copyright 2014 The..."

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
File ui/android/ui_android_jni_registrar.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
ui/android/ui_android_jni_registrar.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: remove (c)

[no sievers, 2014-12-02 00:26:02]

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS#newcode2
ui/android/DEPS:2: "+cc",
Actually, even better, only allow cc/resources specifically.

[Jaekyun Seok (inactive), 2014-12-02 03:13:13]

PTAL.

https://codereview.chromium.org/731133002/diff/220001/ui/android/BUILD.gn
File ui/android/BUILD.gn (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/BUILD.gn#new...
ui/android/BUILD.gn:32: jni_package = "android"
On 2014/12/02 00:25:06, sievers wrote:
> This is a bit too generic. How about "ui_android"?

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS
File ui/android/DEPS (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS#newcode2
ui/android/DEPS:2: "+cc",
On 2014/12/02 00:25:06, sievers wrote:
> Can you add "-cc/blink"?

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/DEPS#newcode4
ui/android/DEPS:4: "+content/public/browser/android/ui_resource_provider.h",
On 2014/12/02 00:25:06, sievers wrote:
> As a follow-up, can we eliminate these base class interfaces (or move them to
> ui/), so we don't have to depend on content/ here?

I will do that in a separate CL as a follow-up.

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/re...
File ui/android/resources/resource_manager.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/re...
ui/android/resources/resource_manager.h:47: gfx::Rect aperture;
On 2014/12/02 00:25:06, sievers wrote:
> nit: As a follow-up, more stuff that could be merged into the class itself now
> that everything gets upstreamed.

I will do that in a separate CL as a follow-up.

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
File ui/android/resources/ui_resource_android.cc (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
ui/android/resources/ui_resource_android.cc:7: #include "base/logging.h"
On 2014/12/02 00:25:06, sievers wrote:
> nit: unused

DCHECK requires this.

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
ui/android/resources/ui_resource_android.cc:15: if (!bitmap_object)
On 2014/12/02 00:25:06, sievers wrote:
> Why do we need to handle this? Should it be DCHECK()? Or should it at least
> return null?

This method isn't used at all. I will remove it.

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
File ui/android/resources/ui_resource_android.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/resources/ui...
ui/android/resources/ui_resource_android.h:29: virtual ~UIResourceAndroid()
override;
On 2014/12/02 00:25:06, sievers wrote:
> nit: no 'virtual' if 'override'

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android.gyp
File ui/android/ui_android.gyp (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android.g...
ui/android/ui_android.gyp:42: 'jni_gen_package': 'ui',
On 2014/12/02 00:25:06, sievers wrote:
> Should this be "ui_android" also since this is its own component?

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_e...
File ui/android/ui_android_export.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_e...
ui/android/ui_android_export.h:5: #ifndef UI_BASE_UI_ANDROID_EXPORT_H_
On 2014/12/02 00:25:06, sievers wrote:
> UI_ANDROID_UI_ANDROID_EXPORT_H here and below

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_e...
ui/android/ui_android_export.h:13: #if defined(WIN32)
On 2014/12/02 00:25:06, sievers wrote:
> nit: WIN32 doesn't really make sense here since it's an Android-only folder.
> Maybe just #error in here?

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
File ui/android/ui_android_jni_registrar.cc (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
ui/android/ui_android_jni_registrar.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/12/02 00:25:06, sievers wrote:
> nit: "Copyright 2014 The..."

Done.

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
File ui/android/ui_android_jni_registrar.h (right):

https://codereview.chromium.org/731133002/diff/220001/ui/android/ui_android_j...
ui/android/ui_android_jni_registrar.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/12/02 00:25:06, sievers wrote:
> nit: remove (c)

Done.

[no sievers, 2014-12-02 17:52:09]

lgtm

[Jaekyun Seok (inactive), 2014-12-02 21:13:37]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-12-02 21:14:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731133002/260001

[commit-bot: I haz the power, 2014-12-02 21:22:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-02 21:22:47]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jaekyun Seok (inactive), 2014-12-02 21:39:59]

jaekyun@chromium.org changed reviewers:
+ bsalomon@google.com, danakj@chromium.org

[Jaekyun Seok (inactive), 2014-12-02 21:40:00]

Dana and Brian, could you please review ui/android/DEPS as owners of
dependencies added to it?

Thanks,

[no sievers, 2014-12-02 22:04:44]

On 2014/12/02 21:40:00, Jaekyun Seok wrote:
> Dana and Brian, could you please review ui/android/DEPS as owners of
> dependencies added to it?
> 
> Thanks,

Maybe you should also add an OWNERS file in ui/android.

[jdduke (slow), 2014-12-02 22:06:40]

On 2014/12/02 22:04:44, sievers wrote:
> On 2014/12/02 21:40:00, Jaekyun Seok wrote:
> > Dana and Brian, could you please review ui/android/DEPS as owners of
> > dependencies added to it?
> > 
> > Thanks,
> 
> Maybe you should also add an OWNERS file in ui/android.

I believe we already have one.

[no sievers, 2014-12-02 22:09:01]

On 2014/12/02 22:06:40, jdduke wrote:
> On 2014/12/02 22:04:44, sievers wrote:
> > On 2014/12/02 21:40:00, Jaekyun Seok wrote:
> > > Dana and Brian, could you please review ui/android/DEPS as owners of
> > > dependencies added to it?
> > > 
> > > Thanks,
> > 
> > Maybe you should also add an OWNERS file in ui/android.
> 
> I believe we already have one.

Then:

+# for UI resources
+jaekyun@chromium.org

[danakj, 2014-12-02 23:57:19]

On 2014/12/02 21:40:00, Jaekyun Seok wrote:
> Dana and Brian, could you please review ui/android/DEPS as owners of
> dependencies added to it?
> 
> Thanks,

the +cc DEPS LGTM

[Jaekyun Seok (inactive), 2014-12-03 00:09:58]

> Then:
> 
> +# for UI resources
> mailto:+jaekyun@chromium.org

I added David and me as owners of UI resources.

[Jaekyun Seok (inactive), 2014-12-04 01:16:14]

Brian, could you please review ui/android/DEPS as owners of skia?

Thanks,

[bsalomon, 2014-12-04 14:56:46]

On 2014/12/04 01:16:14, Jaekyun Seok wrote:
> Brian, could you please review ui/android/DEPS as owners of skia?
> 
> Thanks,

lgtm

[Jaekyun Seok (inactive), 2014-12-04 15:10:02]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-12-04 15:10:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731133002/320001

[commit-bot: I haz the power, 2014-12-04 16:19:22]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2014-12-04 16:20:28]

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