[Android] Upstream ChromeBrowserProvider, the Android port ContentProvider implementation.

[Android] Upstream ChromeBrowserProvider, the Android port ContentProvider implementation.


BUG=138755
TEST=3 new java tests coming as soon as we're ready to build them


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

[Leandro Graciá Gil, 2012-09-09 21:25:27]

yaron: the new Android code. It's basically what we have downstream with minor
presubmit style fixes.
michaelbai: FYI. Feel free to make any comments you might find appropriate.
jam: owner approval for the chrome_browser.gypi changes.

Thanks!

[jam, 2012-09-10 15:57:39]

lgtm

[Yaron, 2012-09-10 19:30:04]

http://codereview.chromium.org/10912172/diff/1/chrome/android/java/src/org/ch...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserProviderSuggestionsCursor.java
(right):

http://codereview.chromium.org/10912172/diff/1/chrome/android/java/src/org/ch...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserProviderSuggestionsCursor.java:16:
// For bookmarks/history suggestions, wrap the cursor returned in one that can
feed
This should be javadoc

http://codereview.chromium.org/10912172/diff/1/chrome/android/java/src/org/ch...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserProviderSuggestionsCursor.java:48:
case 0:
What constants do these map to? Can we used name constants form somewhere? Is
the problem that these come from native chrome? If so, you can use a similar
scheme as http://codereview.chromium.org/10912136/

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderBookmarkNodeTest.java
(right):

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderBookmarkNodeTest.java:30:
BookmarkTestUtils.getIcon("chrome/test/data/android/favicon.png"),
Add needed test data to this cl.

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java
(right):

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java:14:
// Base class for Chrome's ContentProvider tests.
Use class-style javadoc

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/blockin...
File chrome/browser/android/blocking_ui_thread_async_request.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/blockin...
chrome/browser/android/blocking_ui_thread_async_request.h:27: void
RunAsyncRequestOnUIThreadBlocking(base::Callback<Signature> request) {
Move to .cc file

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/bookmar...
File chrome/browser/android/bookmark_model_observer_task.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/bookmar...
chrome/browser/android/bookmark_model_observer_task.h:35: virtual void
Loaded(BookmarkModel* model, bool ids_reassigned) OVERRIDE;
Use chromium overridding style.

// BookmarkModelObserver:
..
(no-line breaks)

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
File chrome/browser/android/chrome_browser_provider.cc (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
chrome/browser/android/chrome_browser_provider.cc:1127: on_bookmark_changed_ =
GetMethodID(env, clazz, "onBookmarkChanged", "()V");
Shouldn't these just be @CalledByNative methods and invoked using the
jni_generator-generated mappings?

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
File chrome/browser/android/chrome_browser_provider.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
chrome/browser/android/chrome_browser_provider.h:220: bool
RegisterChromeBrowserProvider(JNIEnv* env);
Nit: make this a static member of above class

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/run_on_...
File chrome/browser/android/run_on_ui_thread_blocking.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/run_on_...
chrome/browser/android/run_on_ui_thread_blocking.h:15: class
RunOnUIThreadBlocking {
Hmm. This seems like a generally dangerous class, right? And it's only used by
browser provider because the user isn't actually waiting for it (well the chrome
browser isn't). can you at least comment it as such? Ideally it could be in a
sub-folder or something to indicate that it's not intended for re-use

[Leandro Gracia Gil, 2012-09-10 22:21:48]

http://codereview.chromium.org/10912172/diff/1/chrome/android/java/src/org/ch...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserProviderSuggestionsCursor.java
(right):

http://codereview.chromium.org/10912172/diff/1/chrome/android/java/src/org/ch...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserProviderSuggestionsCursor.java:16:
// For bookmarks/history suggestions, wrap the cursor returned in one that can
feed
On 2012/09/10 19:30:04, Yaron wrote:
> This should be javadoc

Done.

http://codereview.chromium.org/10912172/diff/1/chrome/android/java/src/org/ch...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBrowserProviderSuggestionsCursor.java:48:
case 0:
This is Michael's code, although I think the values correspond to the ones above
in COLS. I'll be adding some private constants to define these indices clearer.

On 2012/09/10 19:30:04, Yaron wrote:
> What constants do these map to? Can we used name constants form somewhere? Is
> the problem that these come from native chrome? If so, you can use a similar
> scheme as http://codereview.chromium.org/10912136/

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderBookmarkNodeTest.java
(right):

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderBookmarkNodeTest.java:30:
BookmarkTestUtils.getIcon("chrome/test/data/android/favicon.png"),
On 2012/09/10 19:30:04, Yaron wrote:
> Add needed test data to this cl.

Good point. Thanks.

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java
(right):

http://codereview.chromium.org/10912172/diff/1/chrome/android/javatests/src/o...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java:14:
// Base class for Chrome's ContentProvider tests.
On 2012/09/10 19:30:04, Yaron wrote:
> Use class-style javadoc

Done.

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/blockin...
File chrome/browser/android/blocking_ui_thread_async_request.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/blockin...
chrome/browser/android/blocking_ui_thread_async_request.h:27: void
RunAsyncRequestOnUIThreadBlocking(base::Callback<Signature> request) {
On 2012/09/10 19:30:04, Yaron wrote:
> Move to .cc file

Can't. It has a template argument and therefore needs to be in the header.

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/bookmar...
File chrome/browser/android/bookmark_model_observer_task.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/bookmar...
chrome/browser/android/bookmark_model_observer_task.h:35: virtual void
Loaded(BookmarkModel* model, bool ids_reassigned) OVERRIDE;
On 2012/09/10 19:30:04, Yaron wrote:
> Use chromium overridding style.
> 
> // BookmarkModelObserver:
> ..
> (no-line breaks)

Done.

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
File chrome/browser/android/chrome_browser_provider.cc (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
chrome/browser/android/chrome_browser_provider.cc:1127: on_bookmark_changed_ =
GetMethodID(env, clazz, "onBookmarkChanged", "()V");
Good point. I should have noticed these when refactoring. Done.

On 2012/09/10 19:30:04, Yaron wrote:
> Shouldn't these just be @CalledByNative methods and invoked using the
> jni_generator-generated mappings?

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
File chrome/browser/android/chrome_browser_provider.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/chrome_...
chrome/browser/android/chrome_browser_provider.h:220: bool
RegisterChromeBrowserProvider(JNIEnv* env);
On 2012/09/10 19:30:04, Yaron wrote:
> Nit: make this a static member of above class

Done.

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/run_on_...
File chrome/browser/android/run_on_ui_thread_blocking.h (right):

http://codereview.chromium.org/10912172/diff/1/chrome/browser/android/run_on_...
chrome/browser/android/run_on_ui_thread_blocking.h:15: class
RunOnUIThreadBlocking {
Moving the provider code to a subfolder and adding a comment here.

On 2012/09/10 19:30:04, Yaron wrote:
> Hmm. This seems like a generally dangerous class, right? And it's only used by
> browser provider because the user isn't actually waiting for it (well the
chrome
> browser isn't). can you at least comment it as such? Ideally it could be in a
> sub-folder or something to indicate that it's not intended for re-use

[Yaron, 2012-09-10 22:38:20]

Err my bad on the binary file additions. I think we need to land those as a
separate change and just dcommit it (else CQ barfs on the patch)

http://codereview.chromium.org/10912172/diff/2002/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java
(right):

http://codereview.chromium.org/10912172/diff/2002/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java:18:
public class ProviderTestBase extends ContentViewTestBase {
I just realized that ContentViewTestBase isn't upstream yet... This may be
blocked on David's test shell upstreaming so that we can have the appropriate
test fixture within which we can run these tests.

http://codereview.chromium.org/10912172/diff/2002/chrome/browser/android/prov...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

http://codereview.chromium.org/10912172/diff/2002/chrome/browser/android/prov...
chrome/browser/android/provider/chrome_browser_provider.cc:1549: env,
weak_java_provider_.get(env).obj());
Maybe deal wit as a follow-up but there's a bunch of weak_ref handling that we
assume just works. Can we be sure that the java side wasn't gc'd when these
notifications come through? (also in ::Observe belwo)

[Leandro Graciá Gil, 2012-09-13 01:33:28]

https://chromiumcodereview.appspot.com/10912172/diff/2002/chrome/android/java...
File
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java
(right):

https://chromiumcodereview.appspot.com/10912172/diff/2002/chrome/android/java...
chrome/android/javatests/src/org/chromium/chrome/browser/test/ProviderTestBase.java:18:
public class ProviderTestBase extends ContentViewTestBase {
On 2012/09/10 22:38:20, Yaron wrote:
> I just realized that ContentViewTestBase isn't upstream yet... This may be
> blocked on David's test shell upstreaming so that we can have the appropriate
> test fixture within which we can run these tests.

As discussed, since the testing code is not ready yet but it should be in the
next week or so, let's drop the tests for now and upstream the rest to prevent
further build breaks because of changes in the FaviconService and other related
code. I'll take care of upstreaming these tests back as soon as the testing
fixture is ready.

https://chromiumcodereview.appspot.com/10912172/diff/2002/chrome/browser/andr...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://chromiumcodereview.appspot.com/10912172/diff/2002/chrome/browser/andr...
chrome/browser/android/provider/chrome_browser_provider.cc:1549: env,
weak_java_provider_.get(env).obj());
On 2012/09/10 22:38:20, Yaron wrote:
> Maybe deal wit as a follow-up but there's a bunch of weak_ref handling that we
> assume just works. Can we be sure that the java side wasn't gc'd when these
> notifications come through? (also in ::Observe belwo)

Adding checks to make sure the java object is not null.

[Yaron, 2012-09-13 01:45:07]

lgtm

[Leandro Graciá Gil, 2012-09-13 01:50:07]

On 2012/09/13 01:45:07, Yaron wrote:
> lgtm

Thanks for the review.

[commit-bot: I haz the power, 2012-09-13 01:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leandrogracia@chromium.org/10912172/6003

[commit-bot: I haz the power, 2012-09-13 03:20:28]

Change committed as 156488