Definition of data types used by Android Content Provider.

Definition of data types used by Android Content Provider.

Just like the history_types, this is Android corresponding one.

BUG=
TEST=


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

[michaelbai, 2012-03-01 04:43:32]

Please review it

[sky, 2012-03-01 20:51:32]

Don't you want to add this to a gyp file?

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

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.cc:73: parent_id_(-1) {
Don't we use 0 to mean non-existant?
Initialize url_id_ too.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.cc:85:
BookmarkIDMapping::const_iterator i = g_bookmark_id_mapping.Get().find(name);
If you only use g_bookmark_id_mapping on one thread, how about populating it
here. That avoids the lazy instance stuff. Same comment for g_search_id_mapping.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.cc:93: : id_(0) {
Initialize template_url_id_ too.

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

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:160: void
SetValid(BookmarkColumnID id) {
Valid is confusing in this context, maybe SetValueExplicitlySet. Same comment
for IsValid. Also, these should be unix_hacker_style since they're inlined.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:264: public:
remove public.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:272: std::string raw_url;
Description.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:283: explicit
AndroidStatement(sql::Statement* statement, int favicon_index);
no explicit.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:284: virtual
~AndroidStatement();
Does this need to be virtual?

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:292: int favicon_index()
{
const

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

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types_unittest.cc:5: #include
"chrome/browser/history/android/android_history_types.h"
newline between 5 and 6, and update header to 2012

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types_unittest.cc:13:
TEST_F(AndroidHistoryTypesTest, TestGetBookmarkColumnID) {
Use TEST(AndroidHistoryTypesTest. That way you don't need to do the class
AndroidHistoryTypesTest.

[michaelbai, 2012-03-02 00:04:53]

As to gyp. Currently, we don't get unit_tests build on Android in upstream.

As a temporary solution, I though I could link all my code only to unit_tests in
all platform as it doesn't use any android specific code.

But, thing became a little tricky since I moved all the files to android
directory because all the files in android directory will be removed in other
platforms.

I am looking into how to make it work again, so I didn't change any gyp file.

PTAL

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

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.cc:73: parent_id_(-1) {
On 2012/03/01 20:51:32, sky wrote:
> Don't we use 0 to mean non-existant?
> Initialize url_id_ too.

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.cc:85:
BookmarkIDMapping::const_iterator i = g_bookmark_id_mapping.Get().find(name);
On 2012/03/01 20:51:32, sky wrote:
> If you only use g_bookmark_id_mapping on one thread, how about populating it
> here. That avoids the lazy instance stuff. Same comment for
g_search_id_mapping.

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.cc:93: : id_(0) {
On 2012/03/01 20:51:32, sky wrote:
> Initialize template_url_id_ too.

Done.

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

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:160: void
SetValid(BookmarkColumnID id) {
On 2012/03/01 20:51:32, sky wrote:
> Valid is confusing in this context, maybe SetValueExplicitlySet. Same comment
> for IsValid. Also, these should be unix_hacker_style since they're inlined.

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:264: public:
On 2012/03/01 20:51:32, sky wrote:
> remove public.

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:272: std::string raw_url;
On 2012/03/01 20:51:32, sky wrote:
> Description.

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:283: explicit
AndroidStatement(sql::Statement* statement, int favicon_index);
On 2012/03/01 20:51:32, sky wrote:
> no explicit.

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:284: virtual
~AndroidStatement();
On 2012/03/01 20:51:32, sky wrote:
> Does this need to be virtual?

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types.h:292: int favicon_index()
{
On 2012/03/01 20:51:32, sky wrote:
> const

Done.

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

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types_unittest.cc:5: #include
"chrome/browser/history/android/android_history_types.h"
On 2012/03/01 20:51:32, sky wrote:
> newline between 5 and 6, and update header to 2012

Done.

http://codereview.chromium.org/9558015/diff/1/chrome/browser/history/android/...
chrome/browser/history/android/android_history_types_unittest.cc:13:
TEST_F(AndroidHistoryTypesTest, TestGetBookmarkColumnID) {
On 2012/03/01 20:51:32, sky wrote:
> Use TEST(AndroidHistoryTypesTest. That way you don't need to do the class
> AndroidHistoryTypesTest.

Done.

[sky, 2012-03-02 00:21:57]

LGTM

[commit-bot: I haz the power, 2012-03-02 00:33:15]

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

[commit-bot: I haz the power, 2012-03-02 03:42:58]

Change committed as 124588