Implement ID persistence for bookmarks:...

Implement ID persistence for bookmarks:
- Bookmark codec now takes in a ctor argument persist_ids
- If it's true, it will serialize IDs of bookmarks when encoding, and
  deserialize already serialized IDs (if present) when decoding.
- During decoding, unique-ify the IDs if they are not unique.
- Add unit tests for all new code.


Coming up in a separate changelist:
- Move ID generation logic to bookmark model, and make it non-static.


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

[sky, 2009-04-30 15:53:14]

http://codereview.chromium.org/99217/diff/31/1022
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/99217/diff/31/1022#newcode207
Line 207: id = id_generator_.GetUniqueID(id);
shouldn't this assignment be outside of this if block? Otherwise this ends up
letting BookmarkNode assign the id, which may conflict with ids from the
generator.

http://codereview.chromium.org/99217/diff/31/1024
File chrome/browser/bookmarks/bookmark_codec.h (right):

http://codereview.chromium.org/99217/diff/31/1024#newcode25
Line 25: class UniqueIDGenerator {
does this class need to be public? Should it be defined as a nested class in
BookmarkCodec?

http://codereview.chromium.org/99217/diff/31/1024#newcode38
Line 38: int current_max() const { return current_max_; }
nit: blank line after this.

http://codereview.chromium.org/99217/diff/31/1024#newcode44
Line 44: };
nit: DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/99217/diff/31/1024#newcode58
Line 58: BookmarkCodec(bool persist_ids);
explicit

http://codereview.chromium.org/99217/diff/31/1026
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/99217/diff/31/1026#newcode53
Line 53: static void SetNextId(int next_id);
This should be private and accessed via a friend. I don't want anyone
accidentally invoking this.

http://codereview.chromium.org/99217/diff/31/1026#newcode74
Line 74: void set_id(int id) { id_ = id; }
This should be private too.

http://codereview.chromium.org/99217/diff/31/1026#newcode127
Line 127: GURL url_;
please keep this const. It makes it harder for folks to screw things up.

http://codereview.chromium.org/99217/diff/31/1028
File chrome/browser/bookmarks/bookmark_model_testutils.h (right):

http://codereview.chromium.org/99217/diff/31/1028#newcode12
Line 12: class BookmarkModelTestUtils {
If you're going with this capitalization, then the file should be
xxx_test_utils.

http://codereview.chromium.org/99217/diff/31/1023
File chrome/browser/bookmarks/bookmark_model_unittest.cc (left):

http://codereview.chromium.org/99217/diff/31/1023#oldcode121
Line 121: void AssertNodesEqual(BookmarkNode* expected, BookmarkNode* actual) {
Ha! I was never using this method. Nice!

http://codereview.chromium.org/99217/diff/31/1029
File chrome/chrome.gyp (right):

http://codereview.chromium.org/99217/diff/31/1029#newcode2185
Line 2185: 'browser/bookmarks/bookmark_codec_unittest.cc',
nit: try and keep these fields sorted, so that bookmark_coded_unittest should be
up a bit.

[Munjal (Google), 2009-04-30 20:48:54]

http://codereview.chromium.org/99217/diff/31/1022
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/99217/diff/31/1022#newcode207
Line 207: id = id_generator_.GetUniqueID(id);
On 2009/04/30 15:53:14, sky wrote:
> shouldn't this assignment be outside of this if block? Otherwise this ends up
> letting BookmarkNode assign the id, which may conflict with ids from the
> generator.

Done.

NOTE that this requires a corresponding change to the id generator since
otherwise ID generator will return 0 as a valid ID once.

There are two ways to fix that:
1. Either add another method to id generator to get the next ID.
2. Or treat 0 as a special value in id generator that will always product a new
ID.

I chose approach 2, and commented to that regard in id generator.

http://codereview.chromium.org/99217/diff/31/1024
File chrome/browser/bookmarks/bookmark_codec.h (right):

http://codereview.chromium.org/99217/diff/31/1024#newcode25
Line 25: class UniqueIDGenerator {
On 2009/04/30 15:53:14, sky wrote:
> does this class need to be public? Should it be defined as a nested class in
> BookmarkCodec?

Kept the class as is per our discussion.

http://codereview.chromium.org/99217/diff/31/1024#newcode38
Line 38: int current_max() const { return current_max_; }
On 2009/04/30 15:53:14, sky wrote:
> nit: blank line after this.

Done.

http://codereview.chromium.org/99217/diff/31/1024#newcode44
Line 44: };
On 2009/04/30 15:53:14, sky wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/99217/diff/31/1024#newcode58
Line 58: BookmarkCodec(bool persist_ids);
On 2009/04/30 15:53:14, sky wrote:
> explicit

Done.

http://codereview.chromium.org/99217/diff/31/1026
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/99217/diff/31/1026#newcode53
Line 53: static void SetNextId(int next_id);
On 2009/04/30 15:53:14, sky wrote:
> This should be private and accessed via a friend. I don't want anyone
> accidentally invoking this.

Done.

http://codereview.chromium.org/99217/diff/31/1026#newcode74
Line 74: void set_id(int id) { id_ = id; }
On 2009/04/30 15:53:14, sky wrote:
> This should be private too.

Done.

http://codereview.chromium.org/99217/diff/31/1026#newcode127
Line 127: GURL url_;
On 2009/04/30 15:53:14, sky wrote:
> please keep this const. It makes it harder for folks to screw things up.

Done.

http://codereview.chromium.org/99217/diff/31/1028
File chrome/browser/bookmarks/bookmark_model_testutils.h (right):

http://codereview.chromium.org/99217/diff/31/1028#newcode12
Line 12: class BookmarkModelTestUtils {
On 2009/04/30 15:53:14, sky wrote:
> If you're going with this capitalization, then the file should be
> xxx_test_utils.

Done.

http://codereview.chromium.org/99217/diff/31/1023
File chrome/browser/bookmarks/bookmark_model_unittest.cc (left):

http://codereview.chromium.org/99217/diff/31/1023#oldcode121
Line 121: void AssertNodesEqual(BookmarkNode* expected, BookmarkNode* actual) {
On 2009/04/30 15:53:14, sky wrote:
> Ha! I was never using this method. Nice!

Yeah, I realized that after I removed it from here and put it in utils. But I
think it's still better to keep it in a common place.

http://codereview.chromium.org/99217/diff/31/1029
File chrome/chrome.gyp (right):

http://codereview.chromium.org/99217/diff/31/1029#newcode2185
Line 2185: 'browser/bookmarks/bookmark_codec_unittest.cc',
On 2009/04/30 15:53:14, sky wrote:
> nit: try and keep these fields sorted, so that bookmark_coded_unittest should
be
> up a bit.

Done.

[sky, 2009-04-30 20:57:12]

http://codereview.chromium.org/99217/diff/35/38
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/99217/diff/35/38#newcode206
Line 206: id = id_generator_.GetUniqueID(id);
Doesn't this need to be after you extract the id? Eg:

if (value.GetString(kIdKey, &id_string) && !StringToInt(id_string, &id))
  return false;
id = id_generator_.GetUniqueId(id);

[Munjal (Google), 2009-04-30 22:06:38]

http://codereview.chromium.org/99217/diff/35/38
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/99217/diff/35/38#newcode206
Line 206: id = id_generator_.GetUniqueID(id);
On 2009/04/30 20:57:12, sky wrote:
> Doesn't this need to be after you extract the id? Eg:
> 
> if (value.GetString(kIdKey, &id_string) && !StringToInt(id_string, &id))
>   return false;
> id = id_generator_.GetUniqueId(id);
> 

Done. Damn, I am so sloppy sometimes.

[sky, 2009-04-30 22:15:45]

LGTM