We need to save bookmarks file when the persist_ids settings changes...

We need to save bookmarks file when the persist_ids settings changes
or when the file is detected to be changed externally.


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

[sky, 2009-05-28 16:23:26]

http://codereview.chromium.org/114055/diff/1/3
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/114055/diff/1/3#newcode456
Line 456: if (details->computed_checksum() != details->stored_checksum())
Do we only need to do this if persist_ids_ is true?

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

http://codereview.chromium.org/114055/diff/1/4#newcode422
Line 422: void SetFileChanged();
Please rename this to ScheduleSave.

http://codereview.chromium.org/114055/diff/1/4#newcode444
Line 444: bool file_changed_;
I don't think you need this.

[Munjal (Google), 2009-05-28 18:30:11]

http://codereview.chromium.org/114055/diff/1/3
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/114055/diff/1/3#newcode456
Line 456: if (details->computed_checksum() != details->stored_checksum())
On 2009/05/28 16:23:26, sky wrote:
> Do we only need to do this if persist_ids_ is true?

Done. But I did this in SetFileChanged. That way, file_changed still correctly
represents if file has changed externally or not. But we schedule a save only if
persist IDs is true.

Initially I thought one potential issue is that the value of persist_ids may
have changed (since the setter is public). But upon thinking more I realized
it's not an issue since we decode very early, and if the value of persist_ids
changes, we schedule a save there also.

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

http://codereview.chromium.org/114055/diff/1/4#newcode422
Line 422: void SetFileChanged();
On 2009/05/28 16:23:26, sky wrote:
> Please rename this to ScheduleSave.

As we discussed, this records the fact that the file changed. In doing that, it
schedules a save if needed.

http://codereview.chromium.org/114055/diff/1/4#newcode444
Line 444: bool file_changed_;
On 2009/05/28 16:23:26, sky wrote:
> I don't think you need this.

I am missing a getter to expose this publicly. I just added that.

[sky, 2009-05-28 19:13:16]

LGTM with the following change.

http://codereview.chromium.org/114055/diff/2004/2007
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/114055/diff/2004/2007#newcode447
Line 447: bool file_changed_;
manulaly -> manually
Can you also add this is set after loading is completed and once set the value
is never changed.

[Munjal (Google), 2009-05-28 19:17:52]

http://codereview.chromium.org/114055/diff/2004/2007
File chrome/browser/bookmarks/bookmark_model.h (right):

http://codereview.chromium.org/114055/diff/2004/2007#newcode447
Line 447: bool file_changed_;
On 2009/05/28 19:13:17, sky wrote:
> manulaly -> manually
> Can you also add this is set after loading is completed and once set the value
> is never changed.

Done. I used "externally" instead of "manually" as that's what I use in other
places.