Upstream some more content/ DEPS-related cleanup.

Upstream some more content/ DEPS-related cleanup.

This eliminates differences vs. downstream.

CommandLine, TraceEvent -> content/common, since they are used from browser, renderer, app

LibraryLoader.java -> content/app to match native

Remove JNIHelper which has bitmap helper tools. It was misleadingly
named and incorrectly included (nothing depends on it upstream).
It yet needs to find its right place.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142768

[John Grabowski, 2012-06-15 01:13:26]

Can you be more specific in your description?
E.g. "Move files from content/browser to content/common because blah blah used
by shell blah"?

[Yaron, 2012-06-15 01:33:38]

On 2012/06/15 01:13:26, John Grabowski wrote:
> Can you be more specific in your description?
> E.g. "Move files from content/browser to content/common because blah blah used
> by shell blah"?

I think I"ll nuke http://codereview.chromium.org/10535161/ as this does it and
more. The other changes that I had overlap with intel changes with the same
effect too

[Yaron, 2012-06-15 17:40:35]

http://codereview.chromium.org/10532155/diff/1/content/app/android/content_jn...
File content/app/android/content_jni_registrar.cc (right):

http://codereview.chromium.org/10532155/diff/1/content/app/android/content_jn...
content/app/android/content_jni_registrar.cc:31: { "BitmapHolder",
RegisterBitmapHolder },
Nit: sort

http://codereview.chromium.org/10532155/diff/1/content/app/android/library_lo...
File content/app/android/library_loader_hooks.cc (right):

http://codereview.chromium.org/10532155/diff/1/content/app/android/library_lo...
content/app/android/library_loader_hooks.cc:92:
"org/chromium/content/app/LibraryLoader";
If you rebase this function is gone (replaced with JNI generator)

http://codereview.chromium.org/10532155/diff/1/content/common/android/command...
File content/common/android/command_line.cc (right):

http://codereview.chromium.org/10532155/diff/1/content/common/android/command...
content/common/android/command_line.cc:16: void
ConvertJavaArrayOfStringsToVectorOfStrings(JNIEnv* env,
Consider moving this to base/android/jni_array.h
Also is this actually a dup of JavaByteArrayToByteVector and we're just using
jobectArray when it could be jbyteArray?

http://codereview.chromium.org/10532155/diff/1/content/public/android/java/or...
File content/public/android/java/org/chromium/content/common/TraceEvent.java
(left):

http://codereview.chromium.org/10532155/diff/1/content/public/android/java/or...
content/public/android/java/org/chromium/content/common/TraceEvent.java:48:
LibraryLoader.checkIsReady();
This was extraneous?

http://codereview.chromium.org/10532155/diff/1/content/public/browser/android...
File content/public/browser/android/bitmap_holder.h (right):

http://codereview.chromium.org/10532155/diff/1/content/public/browser/android...
content/public/browser/android/bitmap_holder.h:23: // TODO: Remove this and use
ScopedJavaLocalRef instead.
add your name

[no sievers, 2012-06-15 20:34:40]

http://codereview.chromium.org/10532155/diff/1/content/app/android/content_jn...
File content/app/android/content_jni_registrar.cc (right):

http://codereview.chromium.org/10532155/diff/1/content/app/android/content_jn...
content/app/android/content_jni_registrar.cc:31: { "BitmapHolder",
RegisterBitmapHolder },
On 2012/06/15 17:40:35, Yaron wrote:
> Nit: sort

Done.

http://codereview.chromium.org/10532155/diff/1/content/app/android/library_lo...
File content/app/android/library_loader_hooks.cc (right):

http://codereview.chromium.org/10532155/diff/1/content/app/android/library_lo...
content/app/android/library_loader_hooks.cc:92:
"org/chromium/content/app/LibraryLoader";
On 2012/06/15 17:40:35, Yaron wrote:
> If you rebase this function is gone (replaced with JNI generator)

Done.

http://codereview.chromium.org/10532155/diff/1/content/common/android/command...
File content/common/android/command_line.cc (right):

http://codereview.chromium.org/10532155/diff/1/content/common/android/command...
content/common/android/command_line.cc:16: void
ConvertJavaArrayOfStringsToVectorOfStrings(JNIEnv* env,
On 2012/06/15 17:40:35, Yaron wrote:
> Consider moving this to base/android/jni_array.h
> Also is this actually a dup of JavaByteArrayToByteVector and we're just using
> jobectArray when it could be jbyteArray?

Ah AppendJavaStringArrayToStringVector() you mean, good catch. I had to add an
ascii version though.

http://codereview.chromium.org/10532155/diff/1/content/public/android/java/or...
File content/public/android/java/org/chromium/content/common/TraceEvent.java
(left):

http://codereview.chromium.org/10532155/diff/1/content/public/android/java/or...
content/public/android/java/org/chromium/content/common/TraceEvent.java:48:
LibraryLoader.checkIsReady();
On 2012/06/15 17:40:35, Yaron wrote:
> This was extraneous?

yes, also removed downstream. Since this is getting called from LibraryLoader
the reverse dependency was unnecessary.

http://codereview.chromium.org/10532155/diff/1/content/public/browser/android...
File content/public/browser/android/bitmap_holder.h (right):

http://codereview.chromium.org/10532155/diff/1/content/public/browser/android...
content/public/browser/android/bitmap_holder.h:23: // TODO: Remove this and use
ScopedJavaLocalRef instead.
On 2012/06/15 17:40:35, Yaron wrote:
> add your name

Done.

[Yaron, 2012-06-15 20:38:55]

lgtm

[Ted C, 2012-06-15 20:52:17]

lgtm

nothing particularly major

http://codereview.chromium.org/10532155/diff/9001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10532155/diff/9001/content/content_browser.gyp...
content/content_browser.gypi:33: 'public/browser/android/bitmap_holder.h',
Last I talked about this with jam, he preferred not to have an android
directory.  Instead something like a prefix would be ok
(android_bitmap_holder.h) but maybe things have changed.

http://codereview.chromium.org/10532155/diff/9001/content/public/browser/andr...
File content/public/browser/android/bitmap_holder.h (right):

http://codereview.chromium.org/10532155/diff/9001/content/public/browser/andr...
content/public/browser/android/bitmap_holder.h:71: const gfx::Rect& sourceRect,
indent of this looks messed up.

[no sievers, 2012-06-15 20:53:30]

Adding jam for OWNERS

[jam, 2012-06-15 22:59:31]

http://codereview.chromium.org/10532155/diff/9001/content/public/browser/andr...
File content/public/browser/android/bitmap_holder.h (right):

http://codereview.chromium.org/10532155/diff/9001/content/public/browser/andr...
content/public/browser/android/bitmap_holder.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
if these helper files stuff is used outside of content, it belongs in base.
that's where helper classes that are common between two modules (in this case
content+chrome) belongs. content\public only has interfaces and structs. see
http://www.chromium.org/developers/content-module/content-api

[no sievers, 2012-06-16 00:01:32]

On 2012/06/15 22:59:31, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/10532155/diff/9001/content/public/browser/andr...
> content/public/browser/android/bitmap_holder.h:1: // Copyright (c) 2012 The
> Chromium Authors. All rights reserved.
> if these helper files stuff is used outside of content, it belongs in base.
> that's where helper classes that are common between two modules (in this case
> content+chrome) belongs. content\public only has interfaces and structs. see
> http://www.chromium.org/developers/content-module/content-api

Ok, that makes sense. bitmap_holder.cc has a skia dependency though, is that
legal from base? If I add skia as a dep to base.gyp I get a circular dependency
failure, although it's not immediately obvious where it comes from as skia seems
to depend on base only for the multi-dll build.

[no sievers, 2012-06-16 00:12:53]

On 2012/06/16 00:01:32, Daniel Sievers wrote:
> On 2012/06/15 22:59:31, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/10532155/diff/9001/content/public/browser/andr...
> > content/public/browser/android/bitmap_holder.h:1: // Copyright (c) 2012 The
> > Chromium Authors. All rights reserved.
> > if these helper files stuff is used outside of content, it belongs in base.
> > that's where helper classes that are common between two modules (in this
case
> > content+chrome) belongs. content\public only has interfaces and structs. see
> > http://www.chromium.org/developers/content-module/content-api
> 
> Ok, that makes sense. bitmap_holder.cc has a skia dependency though, is that
> legal from base? If I add skia as a dep to base.gyp I get a circular
dependency
> failure, although it's not immediately obvious where it comes from as skia
seems
> to depend on base only for the multi-dll build.

I think I can work around it, as the functionality in that file that needs skia
is not used from both externally and content (I can move that piece out).

[no sievers, 2012-06-16 00:37:32]

Ok removed for now... still need lgtm from jam for content/ changes.

[John Grabowski, 2012-06-16 00:44:41]

base/android LGTM

[jam, 2012-06-18 15:29:55]

lgtm

[commit-bot: I haz the power, 2012-06-18 17:28:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sievers@chromium.org/10532155/6005

[commit-bot: I haz the power, 2012-06-18 18:48:57]

Change committed as 142768