AndroidProvider: Add history for bookmarks if it doesn't exist.

AndroidProvider: Add history for bookmarks if it doesn't exist.


BUG=http://b/6696843

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

[sky, 2012-08-07 18:31:43]

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
File chrome/browser/history/android/android_provider_backend.cc (right):

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
chrome/browser/history/android/android_provider_backend.cc:769:
url_row.set_last_visit(Time::UnixEpoch());
You sure you don't want a time of now? And do you want to mark it hidden?

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
File chrome/browser/history/android/android_provider_backend.h (right):

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
chrome/browser/history/android/android_provider_backend.h:210: // The tables
will no be updated before the insertion if the |update_tables|
Can you make this more explicit, eg:
rename update_tables to something like invoke_ensureInitialized_and_updated
and a comment like. If |invoke_ensure_initialized_and_updated| is true
EnsureInitializedAndUpdated() is invoked prior to insertion.

[michaelbai, 2012-08-07 19:21:32]

PTAL

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
File chrome/browser/history/android/android_provider_backend.cc (right):

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
chrome/browser/history/android/android_provider_backend.cc:769:
url_row.set_last_visit(Time::UnixEpoch());
- Set the time to Time::UnixEpoch() consistent with what I did in line 461, the
Android CTS has test case for this. 

- What's the 'hidden' used for? I am not sure about making it hidden, as
HistoryBackend::DeleteAllHistory doesn't make the bookmark hidden, but
AddPageNoVisitForBookmark set it. Let me know if you think it should be hidden
here.


On 2012/08/07 18:31:43, sky wrote:
> You sure you don't want a time of now? And do you want to mark it hidden?

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
File chrome/browser/history/android/android_provider_backend.h (right):

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
chrome/browser/history/android/android_provider_backend.h:210: // The tables
will no be updated before the insertion if the |update_tables|
On 2012/08/07 18:31:43, sky wrote:
> Can you make this more explicit, eg:
> rename update_tables to something like invoke_ensureInitialized_and_updated
> and a comment like. If |invoke_ensure_initialized_and_updated| is true
> EnsureInitializedAndUpdated() is invoked prior to insertion.

Done.

[sky, 2012-08-07 20:43:08]

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
File chrome/browser/history/android/android_provider_backend.cc (right):

http://codereview.chromium.org/10825214/diff/1/chrome/browser/history/android...
chrome/browser/history/android/android_provider_backend.cc:769:
url_row.set_last_visit(Time::UnixEpoch());
On 2012/08/07 19:21:32, michaelbai wrote:
> - Set the time to Time::UnixEpoch() consistent with what I did in line 461,
the
> Android CTS has test case for this. 

Add a comment why then.

> - What's the 'hidden' used for? I am not sure about making it hidden, as
> HistoryBackend::DeleteAllHistory doesn't make the bookmark hidden, but
> AddPageNoVisitForBookmark set it. Let me know if you think it should be hidden
> here.

For one it's used by omnibox related code.

> 
> On 2012/08/07 18:31:43, sky wrote:
> > You sure you don't want a time of now? And do you want to mark it hidden?
>

[michaelbai, 2012-08-08 21:32:54]

Added the comment. PTAL

[sky, 2012-08-08 22:01:35]

What about the hidden setting? Seems like we want that.

[michaelbai, 2012-08-08 22:53:24]

Added set_hidden

[sky, 2012-08-09 00:21:25]

LGTM

[commit-bot: I haz the power, 2012-08-09 00:52:38]

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

[commit-bot: I haz the power, 2012-08-09 02:18:07]

Try job failure for 10825214-10003 (retry) on linux_rel for step
"nacl_integration".
It's a second try, previously, step "nacl_integration" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-09 06:16:25]

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

[commit-bot: I haz the power, 2012-08-09 07:36:50]

Change committed as 150765