Makes sure bookmark ids are unique on reading from file. If not unique...

Makes sure bookmark ids are unique on reading from file. If not unique
we reassign them. The particular scenario that can lead to this is
when migrating bookmarks out of the db we never reset the ids of the
newly created nodes, so that we wrote all to the file with ids of
0. I'm not patching that code so that I have coverage of the unit
test, and we'll handle fix up during reading anyway.

BUG=24060
TEST=covered by unit tests


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

[Munjal (Google), 2009-10-07 19:31:47]

http://codereview.chromium.org/268001/diff/1/2
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/268001/diff/1/2#newcode195
Line 195: ids_.count(id) != 0)) {
May be rename ids_missing_ to ids_valid_ (and assign true to it in Decode). Then
we can slightly change ifs to be:

if (ids_valid_) {
  if (value.GetString(...) && StringToInt64(...) && ids_.count(...)) {
    ids_.insert(id);
  } else {
    ids_valid_ = false;
  }
}

If that seems better to you in understanding the code, can you make that change?

http://codereview.chromium.org/268001/diff/1/4
File chrome/browser/bookmarks/bookmark_codec.h (right):

http://codereview.chromium.org/268001/diff/1/4#newcode143
Line 143: bool ids_missing_;
Should we rename this to something like ids_invalid_ or ids_valid_ (per my
comment in the .cc file)?

[sky, 2009-10-07 19:52:24]

Sure thing. Renamed to ids_valid_ and new snapshot uploaded.

  -Scott

On 2009/10/07 19:31:47, munjal wrote:
> http://codereview.chromium.org/268001/diff/1/2
> File chrome/browser/bookmarks/bookmark_codec.cc (right):
> 
> http://codereview.chromium.org/268001/diff/1/2#newcode195
> Line 195: ids_.count(id) != 0)) {
> May be rename ids_missing_ to ids_valid_ (and assign true to it in Decode).
Then
> we can slightly change ifs to be:
> 
> if (ids_valid_) {
>   if (value.GetString(...) && StringToInt64(...) && ids_.count(...)) {
>     ids_.insert(id);
>   } else {
>     ids_valid_ = false;
>   }
> }
> 
> If that seems better to you in understanding the code, can you make that
change?
> 
> http://codereview.chromium.org/268001/diff/1/4
> File chrome/browser/bookmarks/bookmark_codec.h (right):
> 
> http://codereview.chromium.org/268001/diff/1/4#newcode143
> Line 143: bool ids_missing_;
> Should we rename this to something like ids_invalid_ or ids_valid_ (per my
> comment in the .cc file)?

[Munjal (Google), 2009-10-07 21:17:56]

LGTM