Completed the code path from AndroidProviderService to HistoryBackend.

Completed the code path from AndroidProviderService to HistoryBackend.

- Added AndroidProviderService.
- Added methods in HistoryService and HistoryBackend to support Android
  content provider.
- Also fixed a issue in testing_profile.cc.

BUG=
TEST=Added the new tests


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

[sky, 2012-04-25 15:57:24]

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
File chrome/browser/android_provider_service.cc (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.cc:175:
request->ForwardResultAsync(request->handle(), false, 0);
These methods are buying you anything. Inline the calls.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
File chrome/browser/android_provider_service.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:5: #ifndef
CHROME_BROWSER_ANDROID_PROVIDER_SERVICE_H_
This class is named too generically, additionally chrome/browser is too high
level for this. How about moving it to chrome/browser/history/android and naming
it android_history_provider_service?

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:17: // for Android content provider.
the Android cont...

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:18: // The methods of this class
should run in UI thread to cooperate with the
should -> must
on the UI thread

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:20: // To be a subclass of
RefCountedThreadSafe is convenient for the caller which
I don't get this comment. Your earlier comment says this is used on the UI
thread, but then mention the Java thread. Are they separate? Are they the same?
Either way, just because two threads are using the same objects doesn't
necessitate the object being ref counted thread safe. What needs to be clear is
who owns the object. If there is no clear owner (typically bad), then ref
counted makes sense.
Is there one clear owner of this object?

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:64: void QueryHistoryAndBookmarks(
Add descriptions for all of the methods.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history.h:888: #if defined(OS_ANDROID)
Why do any of these methods need to be here? If you're going to make
AndroidProviderService a friend, it can invoke Schedule directly.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history.h:948: const std::string& selection,
spacing is off

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1173: #if defined(OS_ANDROID)
Move all of this into the file history_backend_android.cc

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history_marshaling.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history_marshaling.h:91: #if defined(OS_ANDROID)
move to history_marshalling_android

http://codereview.chromium.org/10217010/diff/2001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:198: if
(!file_util::PathExists(profile_path_))
Why is this needed?

[michaelbai, 2012-04-26 05:32:47]

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
File chrome/browser/android_provider_service.cc (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.cc:175:
request->ForwardResultAsync(request->handle(), false, 0);
On 2012/04/25 15:57:25, sky wrote:
> These methods are buying you anything. Inline the calls.

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
File chrome/browser/android_provider_service.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:5: #ifndef
CHROME_BROWSER_ANDROID_PROVIDER_SERVICE_H_
On 2012/04/25 15:57:25, sky wrote:
> This class is named too generically, additionally chrome/browser is too high
> level for this. How about moving it to chrome/browser/history/android and
naming
> it android_history_provider_service?

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:17: // for Android content provider.
On 2012/04/25 15:57:25, sky wrote:
> the Android cont...

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:18: // The methods of this class
should run in UI thread to cooperate with the
On 2012/04/25 15:57:25, sky wrote:
> should -> must
> on the UI thread

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:20: // To be a subclass of
RefCountedThreadSafe is convenient for the caller which
You are right! I removed RefCounted, it is not necessary.

On 2012/04/25 15:57:25, sky wrote:
> I don't get this comment. Your earlier comment says this is used on the UI
> thread, but then mention the Java thread. Are they separate? Are they the
same?
> Either way, just because two threads are using the same objects doesn't
> necessitate the object being ref counted thread safe. What needs to be clear
is
> who owns the object. If there is no clear owner (typically bad), then ref
> counted makes sense.
> Is there one clear owner of this object?

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/android_prov...
chrome/browser/android_provider_service.h:64: void QueryHistoryAndBookmarks(
On 2012/04/25 15:57:25, sky wrote:
> Add descriptions for all of the methods.

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history.h:888: #if defined(OS_ANDROID)
I followed the pattern of the FaviconService, if you think this is redundant, I
could move them to the service class.

Or, I could move the implementation to history_android.cc

On 2012/04/25 15:57:25, sky wrote:
> Why do any of these methods need to be here? If you're going to make
> AndroidProviderService a friend, it can invoke Schedule directly.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history.h:948: const std::string& selection,
On 2012/04/25 15:57:25, sky wrote:
> spacing is off

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1173: #if defined(OS_ANDROID)
On 2012/04/25 15:57:25, sky wrote:
> Move all of this into the file history_backend_android.cc

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history_marshaling.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history_marshaling.h:91: #if defined(OS_ANDROID)
On 2012/04/25 15:57:25, sky wrote:
> move to history_marshalling_android

Done.

http://codereview.chromium.org/10217010/diff/2001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:198: if
(!file_util::PathExists(profile_path_))
For the profile created by TestingProfileManager::CreateTestingProfile(const
std::string& name), the directory is not created, and
TestingProfile::CreateHistoryService() will fail

On 2012/04/25 15:57:25, sky wrote:
> Why is this needed?

[michaelbai, 2012-04-27 18:53:51]

PTAL

[sky, 2012-04-27 21:37:21]

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10217010/diff/2001/chrome/browser/history/hist...
chrome/browser/history/history.h:888: #if defined(OS_ANDROID)
On 2012/04/26 05:32:48, michaelbai wrote:
> I followed the pattern of the FaviconService, if you think this is redundant,
I
> could move them to the service class.
> 
> Or, I could move the implementation to history_android.cc
> 
> On 2012/04/25 15:57:25, sky wrote:
> > Why do any of these methods need to be here? If you're going to make
> > AndroidProviderService a friend, it can invoke Schedule directly.
> 

I would rather put them in AndroidHistoryProviderService.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
File chrome/browser/history/android/android_history_provider_service.cc (right):

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.cc:111: }
This leaks if hs is NULL.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
File chrome/browser/history/android/android_history_provider_service.h (right):

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:69: void
QueryHistoryAndBookmarks(
How come none of these return a Handle? Do you never need to cancel them?

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:71: const
std::string selection,
const std::string&

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:73: const
std::string sort_order,
const std::string&

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:77: // Runs
the given update and the number of the row updated is returned from
from -> to the

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:115: // Moved
the statement's current row from |current_pos| to |destination| in DB
Moved -> Moves

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:116: //
thread. The new position will be returned in the callback and might not be
will be -> is returned to the callback.
The result supplied to the callback is constrained by the number of rows.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:124: // Asks
to close the statement in database thread. The ownership of
Closes the statement on the db thread. AndroidHistoryProviderService takes
ownership of |statement|.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:129: //
Inserts the given valus and return the SearchTermID of the inserted row
valus -> values
return -> returns

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:135: // Runs
the given update and returns the number of the updated rows from the
of the updated -> of update

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:166: const
std::string selection,
const std::string&

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:168: const
std::string sort_order,
const std::string&

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
File chrome/browser/history/android/android_history_provider_service_unittest.cc
(right):

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service_unittest.cc:40:
~AndroidHistoryProviderServiceTest() {
virtual

[michaelbai, 2012-04-27 23:26:04]

PTAL

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
File chrome/browser/history/android/android_history_provider_service.cc (right):

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.cc:111: }
On 2012/04/27 21:37:22, sky wrote:
> This leaks if hs is NULL.

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
File chrome/browser/history/android/android_history_provider_service.h (right):

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:69: void
QueryHistoryAndBookmarks(
It was there, it seemed that someone removed them during the merge.

On 2012/04/27 21:37:22, sky wrote:
> How come none of these return a Handle? Do you never need to cancel them?

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:71: const
std::string selection,
On 2012/04/27 21:37:22, sky wrote:
> const std::string&

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:73: const
std::string sort_order,
On 2012/04/27 21:37:22, sky wrote:
> const std::string&

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:77: // Runs
the given update and the number of the row updated is returned from
On 2012/04/27 21:37:22, sky wrote:
> from -> to the

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:115: // Moved
the statement's current row from |current_pos| to |destination| in DB
On 2012/04/27 21:37:22, sky wrote:
> Moved -> Moves

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:116: //
thread. The new position will be returned in the callback and might not be
On 2012/04/27 21:37:22, sky wrote:
> will be -> is returned to the callback.
> The result supplied to the callback is constrained by the number of rows.

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:124: // Asks
to close the statement in database thread. The ownership of
On 2012/04/27 21:37:22, sky wrote:
> Closes the statement on the db thread. AndroidHistoryProviderService takes
> ownership of |statement|.

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:129: //
Inserts the given valus and return the SearchTermID of the inserted row
On 2012/04/27 21:37:22, sky wrote:
> valus -> values
> return -> returns

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:135: // Runs
the given update and returns the number of the updated rows from the
On 2012/04/27 21:37:22, sky wrote:
> of the updated -> of update

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:166: const
std::string selection,
On 2012/04/27 21:37:22, sky wrote:
> const std::string&

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service.h:168: const
std::string sort_order,
On 2012/04/27 21:37:22, sky wrote:
> const std::string&

Done.

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
File chrome/browser/history/android/android_history_provider_service_unittest.cc
(right):

http://codereview.chromium.org/10217010/diff/12001/chrome/browser/history/and...
chrome/browser/history/android/android_history_provider_service_unittest.cc:40:
~AndroidHistoryProviderServiceTest() {
On 2012/04/27 21:37:22, sky wrote:
> virtual

Done.

[sky, 2012-04-27 23:35:52]

LGTM

[commit-bot: I haz the power, 2012-05-04 18:21:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10217010/21001

[commit-bot: I haz the power, 2012-05-04 18:21:41]

Can't apply patch for file
chrome/browser/history/android/android_provider_backend_unittest.cc.
While running patch -p1 --forward --force;
patching file
chrome/browser/history/android/android_provider_backend_unittest.cc
Hunk #1 FAILED at 75.
Hunk #2 FAILED at 1644.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/history/android/android_provider_backend_unittest.cc.rej

[commit-bot: I haz the power, 2012-05-04 18:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/10217010/26002

[commit-bot: I haz the power, 2012-05-04 20:49:51]

Change committed as 135414