Always persist bookmark IDs....

Always persist bookmark IDs.
Remove the preference to persist IDs.
NOTE that we need to save the file the first time with IDs
since existing bookmark files won't have IDs and the file
won't be saved until something changes in the bookmark model.
So we need to explicitly save once when we assign ids for the
first time.

TEST=NONE
BUG=16068
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20532

[Aaron Boodman, 2009-07-08 04:28:54]

Great! I didn't know we were definitely doing this.

[sky, 2009-07-08 15:32:28]

Because we're going to persist the ids it's more likely that we might exceed max
int. As such, please convert the id to an int64.

http://codereview.chromium.org/149310/diff/29/35
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/149310/diff/29/35#newcode123
Line 123: int id_;
Convert this to int64.

[Munjal (Google), 2009-07-08 20:42:33]

Adding Erik to code review as suggested by Aaron.

Erik, please note that I changed bookmark ID serialization in JSON values in
bookmark extension to strings after my changes to make bookmark id an int64.
Please review that part.

http://codereview.chromium.org/149310/diff/29/35
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/149310/diff/29/35#newcode123
Line 123: int id_;
On 2009/07/08 15:32:28, sky wrote:
> Convert this to int64.

Done.

[sky, 2009-07-08 21:07:39]

LGTM with the following changes.

Wait for Erik to comment on the extension changes though.

http://codereview.chromium.org/149310/diff/41/1019
File chrome/browser/bookmarks/bookmark_codec.h (right):

http://codereview.chromium.org/149310/diff/41/1019#newcode165
Line 165: // Whether or not IDs were reassigned by the codec.
nit: newline between 164 and 165.

http://codereview.chromium.org/149310/diff/41/1021
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/149310/diff/41/1021#newcode56
Line 56: // Returns a unique id for this node.
nit: you should explicitly mention the ID is maintained after restarts.

[Erik does not do reviews, 2009-07-08 21:51:26]

http://codereview.chromium.org/149310/diff/41/1018
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/149310/diff/41/1018#newcode22
Line 22: if (IsIdAssigned(id))
I don't understand why this is necessary.  The file is checksummed, and should
have only written ids out in one of two states, none at all, or all unique.  For
large bookmark files, I could imagine that this set could actually take a
measurable amount of time.

http://codereview.chromium.org/149310/diff/41/1019
File chrome/browser/bookmarks/bookmark_codec.h (right):

http://codereview.chromium.org/149310/diff/41/1019#newcode51
Line 51: // All IDs assigned so far.
newline above comment

http://codereview.chromium.org/149310/diff/41/1019#newcode165
Line 165: // Whether or not IDs were reassigned by the codec.
newline above comment

http://codereview.chromium.org/149310/diff/41/1023
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/149310/diff/41/1023#newcode273
Line 273: int64 id = 0;
we don't need the = 0 here

http://codereview.chromium.org/149310/diff/41/1023#newcode276
Line 276: EXTENSION_FUNCTION_VALIDATE(StringToInt64(id_string, &id));
unfortunately, I don't think we can use EXTENSION_FUNCTION_VALIDATE as-is for
this case.  This is designed for API verification that should have already been
covered by the renderer.  The bummer is that JS can't handle 64 bit ints.  There
are three choices here:
* add a new type to our renderer API validation that can verify 64bit ints
represented as strings
* change the bookmark storage system to use 54 bit capped ints (doubles)
* replace these EXTENSION_FUNCTION_VALIDATE calls with non-fatal error handling
(see the various error_ = samples below)

http://codereview.chromium.org/149310/diff/41/1023#newcode363
Line 363: if (args->GetString(0, &id_string) || StringToInt64(id_string, &id)) {
should be &&

[Munjal (Google), 2009-07-09 02:21:22]

http://codereview.chromium.org/149310/diff/41/1018
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/149310/diff/41/1018#newcode22
Line 22: if (IsIdAssigned(id))
On 2009/07/08 21:51:27, Erik Kay wrote:
> I don't understand why this is necessary.  The file is checksummed, and should
> have only written ids out in one of two states, none at all, or all unique. 
For
> large bookmark files, I could imagine that this set could actually take a
> measurable amount of time.

This is not changed in this CL. Also, this is needed to handle cases where the
user may have manually modified the bookmarks file and may have duplicated the
IDs. That can cause bad things to happen in Chrome if we don't guarantee unique
IDs regardless of what the user does offline.

http://codereview.chromium.org/149310/diff/41/1019
File chrome/browser/bookmarks/bookmark_codec.h (right):

http://codereview.chromium.org/149310/diff/41/1019#newcode51
Line 51: // All IDs assigned so far.
On 2009/07/08 21:51:27, Erik Kay wrote:
> newline above comment

Done.

http://codereview.chromium.org/149310/diff/41/1019#newcode165
Line 165: // Whether or not IDs were reassigned by the codec.
On 2009/07/08 21:07:39, sky wrote:
> nit: newline between 164 and 165.

Done.

http://codereview.chromium.org/149310/diff/41/1019#newcode165
Line 165: // Whether or not IDs were reassigned by the codec.
On 2009/07/08 21:51:27, Erik Kay wrote:
> newline above comment
> 

Done.

http://codereview.chromium.org/149310/diff/41/1021
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/149310/diff/41/1021#newcode56
Line 56: // Returns a unique id for this node.
On 2009/07/08 21:07:39, sky wrote:
> nit: you should explicitly mention the ID is maintained after restarts.

Done.

http://codereview.chromium.org/149310/diff/41/1023
File chrome/browser/extensions/extension_bookmarks_module.cc (right):

http://codereview.chromium.org/149310/diff/41/1023#newcode273
Line 273: int64 id = 0;
On 2009/07/08 21:51:27, Erik Kay wrote:
> we don't need the = 0 here

Done.

http://codereview.chromium.org/149310/diff/41/1023#newcode276
Line 276: EXTENSION_FUNCTION_VALIDATE(StringToInt64(id_string, &id));
On 2009/07/08 21:51:27, Erik Kay wrote:
> unfortunately, I don't think we can use EXTENSION_FUNCTION_VALIDATE as-is for
> this case.  This is designed for API verification that should have already
been
> covered by the renderer.  The bummer is that JS can't handle 64 bit ints. 
There
> are three choices here:
> * add a new type to our renderer API validation that can verify 64bit ints
> represented as strings
> * change the bookmark storage system to use 54 bit capped ints (doubles)
> * replace these EXTENSION_FUNCTION_VALIDATE calls with non-fatal error
handling
> (see the various error_ = samples below)

Done. Chose approach 3 per our chat.

http://codereview.chromium.org/149310/diff/41/1023#newcode363
Line 363: if (args->GetString(0, &id_string) || StringToInt64(id_string, &id)) {
On 2009/07/08 21:51:27, Erik Kay wrote:
> should be &&

Done.

[Munjal (Google), 2009-07-09 05:12:47]

Adding Evan to code review GTK files for Linux.

[Evan Martin, 2009-07-09 05:21:01]

int64s in chrome are a pet peeve of mine

http://codereview.chromium.org/149310/diff/116/1102
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/149310/diff/116/1102#newcode21
Line 21: // If the given ID is already assigned, generate new ID.
What are these supposed to be unique among?
A single user?  All users ever?

If it's a single user, consider there are 365 days in a year.  Say I create an
obscenely excessive 1000 new bookmarks a day every day.  Keeping up that rate,
it would then take me 11,767 years to run out of id space.

[sky, 2009-07-09 15:57:04]

On Wed, Jul 8, 2009 at 10:21 PM, <evan@chromium.org> wrote:
> int64s in chrome are a pet peeve of mine
>
>
> http://codereview.chromium.org/149310/diff/116/1102
> File chrome/browser/bookmarks/bookmark_codec.cc (right):
>
> http://codereview.chromium.org/149310/diff/116/1102#newcode21
> Line 21: // If the given ID is already assigned, generate new ID.
> What are these supposed to be unique among?
> A single user? =A0All users ever?

A single user.

> If it's a single user, consider there are 365 days in a year. =A0Say I
> create an obscenely excessive 1000 new bookmarks a day every day.
> Keeping up that rate, it would then take me 11,767 years to run out of
> id space.

It's my understanding some extensions repeatedly create a bunch of
bookmarks for various things. I didn't want to worry about the
possibility of recycling ids with such extensions.

  -Scott

[Evan Martin, 2009-07-09 16:12:36]

GTK change looks fine, if you don't cause new crashes.

BTW, there are more people working on Linux than just me -- looking at the svn
blame of the file would help you find someone who has actually worked on this
file before.  :)

[Erik does not do reviews, 2009-07-09 16:17:27]

http://codereview.chromium.org/149310/diff/41/1018
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/149310/diff/41/1018#newcode22
Line 22: if (IsIdAssigned(id))
On 2009/07/09 02:21:22, munjal wrote:
> On 2009/07/08 21:51:27, Erik Kay wrote:
> > I don't understand why this is necessary.  The file is checksummed, and
should
> > have only written ids out in one of two states, none at all, or all unique. 
> For
> > large bookmark files, I could imagine that this set could actually take a
> > measurable amount of time.
> 
> This is not changed in this CL. Also, this is needed to handle cases where the
> user may have manually modified the bookmarks file and may have duplicated the
> IDs. That can cause bad things to happen in Chrome if we don't guarantee
unique
> IDs regardless of what the user does offline.

I understand that it's not changed in this CL.  That doesn't mean it's not
wrong. ;-)

If the user manually modified the bookmarks file, the checksum on the file will
be off, right?  My point is that we already handle this case elsewhere much more
efficiently, so why go through all of this extra work here?

[sky, 2009-07-09 16:19:40]

On 2009/07/09 16:17:27, Erik Kay wrote:
> http://codereview.chromium.org/149310/diff/41/1018
> File chrome/browser/bookmarks/bookmark_codec.cc (right):
> 
> http://codereview.chromium.org/149310/diff/41/1018#newcode22
> Line 22: if (IsIdAssigned(id))
> On 2009/07/09 02:21:22, munjal wrote:
> > On 2009/07/08 21:51:27, Erik Kay wrote:
> > > I don't understand why this is necessary.  The file is checksummed, and
> should
> > > have only written ids out in one of two states, none at all, or all
unique. 
> > For
> > > large bookmark files, I could imagine that this set could actually take a
> > > measurable amount of time.
> > 
> > This is not changed in this CL. Also, this is needed to handle cases where
the
> > user may have manually modified the bookmarks file and may have duplicated
the
> > IDs. That can cause bad things to happen in Chrome if we don't guarantee
> unique
> > IDs regardless of what the user does offline.
> 
> I understand that it's not changed in this CL.  That doesn't mean it's not
> wrong. ;-)
> 
> If the user manually modified the bookmarks file, the checksum on the file
will
> be off, right?  My point is that we already handle this case elsewhere much
more
> efficiently, so why go through all of this extra work here?

I agree with Erik. I think we should nuke the IDGenerator and if the checksums
differ reset all the ids.

[Erik does not do reviews, 2009-07-09 16:23:09]

On 2009/07/09 15:57:04, sky wrote:
> On Wed, Jul 8, 2009 at 10:21 PM, <evan@chromium.org> wrote:
> > int64s in chrome are a pet peeve of mine
> >
> >
> > http://codereview.chromium.org/149310/diff/116/1102
> > File chrome/browser/bookmarks/bookmark_codec.cc (right):
> >
> > http://codereview.chromium.org/149310/diff/116/1102#newcode21
> > Line 21: // If the given ID is already assigned, generate new ID.
> > What are these supposed to be unique among?
> > A single user? =A0All users ever?
> 
> A single user.
> 
> > If it's a single user, consider there are 365 days in a year. =A0Say I
> > create an obscenely excessive 1000 new bookmarks a day every day.
> > Keeping up that rate, it would then take me 11,767 years to run out of
> > id space.
> 
> It's my understanding some extensions repeatedly create a bunch of
> bookmarks for various things. I didn't want to worry about the
> possibility of recycling ids with such extensions.
> 
>   -Scott

I agree with the point that in the Firefox world it's common for extension
authors to store lots of random state in bookmarks to take advantage of weave,
or other cloud bookmark sync systems as a cheap way for them to get cloud
storage of their state as well.  While this isn't something that we particularly
want to encourage, it's going to happen.

On the flip side, int32 is still a large space to use up even if you assume
really aggressive extensions.  Even assuming 31 bits, for ten years of use,
you'd need code that's creating over 1/2 million bookmarks per day in order to
run out of ids.

I don't feel too religious about this since the JSON storage means that it's the
same cost either way (it's all stringified).  The only real differences are the
small in memory difference and the impact on the JS API validation, neither of
which are large issues.

[Evan Martin, 2009-07-09 16:27:21]

On 2009/07/09 16:23:09, Erik Kay wrote:
> On 2009/07/09 15:57:04, sky wrote:
> > On Wed, Jul 8, 2009 at 10:21 PM, <evan@chromium.org> wrote:
> > > int64s in chrome are a pet peeve of mine
> > >
> > >
> > > http://codereview.chromium.org/149310/diff/116/1102
> > > File chrome/browser/bookmarks/bookmark_codec.cc (right):
> > >
> > > http://codereview.chromium.org/149310/diff/116/1102#newcode21
> > > Line 21: // If the given ID is already assigned, generate new ID.
> > > What are these supposed to be unique among?
> > > A single user? =A0All users ever?
> > 
> > A single user.
> > 
> > > If it's a single user, consider there are 365 days in a year. =A0Say I
> > > create an obscenely excessive 1000 new bookmarks a day every day.
> > > Keeping up that rate, it would then take me 11,767 years to run out of
> > > id space.
> > 
> > It's my understanding some extensions repeatedly create a bunch of
> > bookmarks for various things. I didn't want to worry about the
> > possibility of recycling ids with such extensions.
> > 
> >   -Scott
> 
> I agree with the point that in the Firefox world it's common for extension
> authors to store lots of random state in bookmarks to take advantage of weave,
> or other cloud bookmark sync systems as a cheap way for them to get cloud
> storage of their state as well.  While this isn't something that we
particularly
> want to encourage, it's going to happen.
> 
> On the flip side, int32 is still a large space to use up even if you assume
> really aggressive extensions.  Even assuming 31 bits, for ten years of use,
> you'd need code that's creating over 1/2 million bookmarks per day in order to
> run out of ids.
> 
> I don't feel too religious about this since the JSON storage means that it's
the
> same cost either way (it's all stringified).  The only real differences are
the
> small in memory difference and the impact on the JS API validation, neither of
> which are large issues.

I don't feel too strongly about it, it's just a minor pet peeve.

One other thing to consider is whether JSON can handle int64s properly.  JSON
numbers are doubles, which have an integral range of 2**53.  On the other hand,
using the same argument why int32 will never overflow I expect we'll never hit
this limit.

*shrug*

[Erik does not do reviews, 2009-07-09 16:33:15]

On Thu, Jul 9, 2009 at 9:27 AM, <evan@chromium.org> wrote:

> On 2009/07/09 16:23:09, Erik Kay wrote:
>
>> On 2009/07/09 15:57:04, sky wrote:
>> > On Wed, Jul 8, 2009 at 10:21 PM, <evan@chromium.org> wrote:
>> > > int64s in chrome are a pet peeve of mine
>> > >
>> > >
>> > > http://codereview.chromium.org/149310/diff/116/1102
>> > > File chrome/browser/bookmarks/bookmark_codec.cc (right):
>> > >
>> > > http://codereview.chromium.org/149310/diff/116/1102#newcode21
>> > > Line 21: // If the given ID is already assigned, generate new ID.
>> > > What are these supposed to be unique among?
>> > > A single user? =A0All users ever?
>> >
>> > A single user.
>> >
>> > > If it's a single user, consider there are 365 days in a year.
>>
> =A0Say I
>
>> > > create an obscenely excessive 1000 new bookmarks a day every day.
>> > > Keeping up that rate, it would then take me 11,767 years to run
>>
> out of
>
>> > > id space.
>> >
>> > It's my understanding some extensions repeatedly create a bunch of
>> > bookmarks for various things. I didn't want to worry about the
>> > possibility of recycling ids with such extensions.
>> >
>> >   -Scott
>>
>
>  I agree with the point that in the Firefox world it's common for
>>
> extension
>
>> authors to store lots of random state in bookmarks to take advantage
>>
> of weave,
>
>> or other cloud bookmark sync systems as a cheap way for them to get
>>
> cloud
>
>> storage of their state as well.  While this isn't something that we
>>
> particularly
>
>> want to encourage, it's going to happen.
>>
>
>  On the flip side, int32 is still a large space to use up even if you
>>
> assume
>
>> really aggressive extensions.  Even assuming 31 bits, for ten years of
>>
> use,
>
>> you'd need code that's creating over 1/2 million bookmarks per day in
>>
> order to
>
>> run out of ids.
>>
>
>  I don't feel too religious about this since the JSON storage means
>>
> that it's the
>
>> same cost either way (it's all stringified).  The only real
>>
> differences are the
>
>> small in memory difference and the impact on the JS API validation,
>>
> neither of
>
>> which are large issues.
>>
>
> I don't feel too strongly about it, it's just a minor pet peeve.
>
> One other thing to consider is whether JSON can handle int64s properly.
> JSON numbers are doubles, which have an integral range of 2**53.  On the
> other hand, using the same argument why int32 will never overflow I
> expect we'll never hit this limit.
>
> *shrug*


Yes, this was discussed earlier on in the review.  The code is storing them
as strings rather than doubles or ints (so an extra 2 chars for quotes).

Erik



>
>
> http://codereview.chromium.org/149310
>

[Munjal (Google), 2009-07-09 17:20:01]

On 2009/07/09 16:12:36, Evan Martin wrote:
> GTK change looks fine, if you don't cause new crashes.
> 
> BTW, there are more people working on Linux than just me -- looking at the svn
> blame of the file would help you find someone who has actually worked on this
> file before.  :)

I did look at who changed that file, IIRC estade and erg. But Both were not
available at the time (yesterday evening) and I saw you online, so thought I can
try asking you.

[Munjal (Google), 2009-07-09 17:20:24]

http://codereview.chromium.org/149310/diff/41/1018
File chrome/browser/bookmarks/bookmark_codec.cc (right):

http://codereview.chromium.org/149310/diff/41/1018#newcode22
Line 22: if (IsIdAssigned(id))
On 2009/07/09 16:17:27, Erik Kay wrote:
> On 2009/07/09 02:21:22, munjal wrote:
> > On 2009/07/08 21:51:27, Erik Kay wrote:
> > > I don't understand why this is necessary.  The file is checksummed, and
> should
> > > have only written ids out in one of two states, none at all, or all
unique. 
> > For
> > > large bookmark files, I could imagine that this set could actually take a
> > > measurable amount of time.
> > 
> > This is not changed in this CL. Also, this is needed to handle cases where
the
> > user may have manually modified the bookmarks file and may have duplicated
the
> > IDs. That can cause bad things to happen in Chrome if we don't guarantee
> unique
> > IDs regardless of what the user does offline.
> 
> I understand that it's not changed in this CL.  That doesn't mean it's not
> wrong. ;-)
> 
> If the user manually modified the bookmarks file, the checksum on the file
will
> be off, right?  My point is that we already handle this case elsewhere much
more
> efficiently, so why go through all of this extra work here?
> 

Sorry I didn't mean to say that we shouldn't change it if it's wrong in this CL.
I meant to say this was discussed in when the code was first added.

There are several reasons we do this instead of reassigning IDs if checksums are
different:
1. We need a separate pass over all the bookmarks to reassign IDs if checksums
differ.
2. We can end up changing a ton of IDs if checksums differ even if only, say one
ID, changed.

I understand both of these may not be a big deal, but ID generator is also not
bad in that case. It's efficient in not allocating the set if the IDs are
serial; though it's not very smart about using memory in all cases.

What I meant ot say when I said "this wasn't added in thsi CL" is all the above
was discussed when we first added the code and it was decided to do this.

We can reconsider, but let's do that in a separate CL if we do it.

[Erik does not do reviews, 2009-07-09 17:46:40]

LGTM with a bug and a TODO to fix this issue.

As we discussed offline, the problem is that we're paying the cost of the set
all of the time in order to handle a rare case that we already detect in another
place.  Instead, we should have a recovery mode when we detect that the checksum
is bad that does the work with minimizing changes to the ids.

[Aaron Boodman, 2009-07-09 17:58:32]

On Thu, Jul 9, 2009 at 9:23 AM, <erikkay@chromium.org> wrote:
> On 2009/07/09 15:57:04, sky wrote:
>>
>> On Wed, Jul 8, 2009 at 10:21 PM, <evan@chromium.org> wrote:
>> > int64s in chrome are a pet peeve of mine
>> >
>> >
>> > http://codereview.chromium.org/149310/diff/116/1102
>> > File chrome/browser/bookmarks/bookmark_codec.cc (right):
>> >
>> > http://codereview.chromium.org/149310/diff/116/1102#newcode21
>> > Line 21: // If the given ID is already assigned, generate new ID.
>> > What are these supposed to be unique among?
>> > A single user? =3DA0All users ever?
>
>> A single user.
>
>> > If it's a single user, consider there are 365 days in a year. =3DA0Say
>
> I
>>
>> > create an obscenely excessive 1000 new bookmarks a day every day.
>> > Keeping up that rate, it would then take me 11,767 years to run out
>
> of
>>
>> > id space.
>
>> It's my understanding some extensions repeatedly create a bunch of
>> bookmarks for various things. I didn't want to worry about the
>> possibility of recycling ids with such extensions.
>
>> =A0 -Scott
>
> I agree with the point that in the Firefox world it's common for
> extension authors to store lots of random state in bookmarks to take
> advantage of weave, or other cloud bookmark sync systems as a cheap way
> for them to get cloud storage of their state as well. =A0While this isn't
> something that we particularly want to encourage, it's going to happen.
>
> On the flip side, int32 is still a large space to use up even if you
> assume really aggressive extensions. =A0Even assuming 31 bits, for ten
> years of use, you'd need code that's creating over 1/2 million bookmarks
> per day in order to run out of ids.
>
> I don't feel too religious about this since the JSON storage means that
> it's the same cost either way (it's all stringified). =A0The only real
> differences are the small in memory difference and the impact on the JS
> API validation, neither of which are large issues.

FWIW, I'm convinced. I feel like we should just use int32.

- a

[Munjal (Google), 2009-07-09 18:26:58]

Adding erg to code review Linux stuff. Elliot, I am seeing a crash in linux
tests after my changes. Can you take a look?