For the immediate milestone, exclude History and Thumbnails from being backed...

For the immediate milestone, exclude History and Thumbnails from being backed up by Time Machine.  The bug describes appropriate long-term solutions to this problem.

BUG=25959
TEST=Launch Chrome.  Inspect the following files found in ~/Library/Application Support/Chromium/Default/: a) History, b) History-journal, c) Thumbnails, and d) Thumbnails-journal and verify that they are not being backed up by Time Machine.


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

[mrossetti, 2009-11-10 23:06:48]

Reviewers: pink,

Description:
For the immediate milestone, exclude History and Thumbnails from being  
backed up
by Time Machine.  The bug describes appropriate long-term solutions to this
problem.

BUG=259595
TEST=Launch Chrome.  Inspect the following files found in  
~/Library/Application
Support/Chromium/Default/: a) History, b) History-journal, c) Thumbnails,  
and d)
Thumbnails-journal and verify that they are not being backed up by Time  
Machine.


Please review this at http://codereview.chromium.org/387016

SVN Base: svn://svn.chromium.org/chrome/trunk/src/

Affected files:
   M     base/mac_util.h
   M     base/mac_util.mm
   M     base/mac_util_unittest.mm
   M     chrome/browser/history/history_database.cc
   M     chrome/browser/history/thumbnail_database.cc

[pink (ping after 24hrs), 2009-11-10 23:15:15]

LGTM

The ifdefs are unfortunate. I wish we could do something about them.

http://codereview.chromium.org/387016/diff/1/3
File base/mac_util.mm (right):

http://codereview.chromium.org/387016/diff/1/3#newcode244
Line 244: // Note that we always set |excludeByPath| to true.  This prevents a
problem
might want to rephrase this. unless you're looking at the OS header file
|excludeByPath| is undefined and unknown to the reader.

http://codereview.chromium.org/387016/diff/1/6
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/387016/diff/1/6#newcode70
Line 70: // Exclude the history file and its journal from backups.
thumbnail database? copy/paste error?

[pink (ping after 24hrs), 2009-11-10 23:15:33]

LGTM

The ifdefs are unfortunate. I wish we could do something about them.


http://codereview.chromium.org/387016/diff/1/3
File base/mac_util.mm (right):

http://codereview.chromium.org/387016/diff/1/3#newcode244
Line 244: // Note that we always set |excludeByPath| to true.  This
prevents a problem
might want to rephrase this. unless you're looking at the OS header file
|excludeByPath| is undefined and unknown to the reader.

http://codereview.chromium.org/387016/diff/1/6
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/387016/diff/1/6#newcode70
Line 70: // Exclude the history file and its journal from backups.
thumbnail database? copy/paste error?

http://codereview.chromium.org/387016

[mrossetti, 2009-11-11 02:14:03]

http://codereview.chromium.org/387016/diff/1/3
File base/mac_util.mm (right):

http://codereview.chromium.org/387016/diff/1/3#newcode244
Line 244: // Note that we always set |excludeByPath| to true.  This prevents a
problem
On 2009/11/10 23:15:15, pink wrote:
> might want to rephrase this. unless you're looking at the OS header file
> |excludeByPath| is undefined and unknown to the reader.

Done.

http://codereview.chromium.org/387016/diff/1/6
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/387016/diff/1/6#newcode70
Line 70: // Exclude the history file and its journal from backups.
On 2009/11/10 23:15:15, pink wrote:
> thumbnail database? copy/paste error?

Done.

[mrossetti, 2009-11-11 02:14:38]

http://codereview.chromium.org/387016/diff/1/3
File base/mac_util.mm (right):

http://codereview.chromium.org/387016/diff/1/3#newcode244
Line 244: // Note that we always set |excludeByPath| to true.  This
prevents a problem
On 2009/11/10 23:15:15, pink wrote:
> might want to rephrase this. unless you're looking at the OS header
file
> |excludeByPath| is undefined and unknown to the reader.

Done.

http://codereview.chromium.org/387016/diff/1/6
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/387016/diff/1/6#newcode70
Line 70: // Exclude the history file and its journal from backups.
On 2009/11/10 23:15:15, pink wrote:
> thumbnail database? copy/paste error?

Done.

http://codereview.chromium.org/387016

[TVL, 2009-11-11 02:51:02]

driveby

http://codereview.chromium.org/387016/diff/5001/5003
File base/mac_util.mm (right):

http://codereview.chromium.org/387016/diff/5001/5003#newcode241
Line 241: NSString* filePath =
CHECK/DCHECK for file_path.IsAbsolute()?

http://codereview.chromium.org/387016/diff/5001/5003#newcode250
Line 250: CSBackupSetItemExcluded((CFURLRef)url, exclude, true) == noErr;
drive by: don't the docs talk about needing to run as admin?  (To change the
backup exclusion status of a path, your application must be running with
administrator privileges.)  does that mean for non admins, we'll still back
stuff up?

[TVL, 2009-11-11 02:51:20]

driveby


http://codereview.chromium.org/387016/diff/5001/5003
File base/mac_util.mm (right):

http://codereview.chromium.org/387016/diff/5001/5003#newcode241
Line 241: NSString* filePath =
CHECK/DCHECK for file_path.IsAbsolute()?

http://codereview.chromium.org/387016/diff/5001/5003#newcode250
Line 250: CSBackupSetItemExcluded((CFURLRef)url, exclude, true) ==
noErr;
drive by: don't the docs talk about needing to run as admin?  (To change
the backup exclusion status of a path, your application must be running
with administrator privileges.)  does that mean for non admins, we'll
still back stuff up?

http://codereview.chromium.org/387016

[mrossetti_google.com, 2009-11-11 05:18:04]

Tests with a non-admin account show that setting the exclusion on a file
lives only as long as the application which sets the exclusion is running,
at which point the exclusion is no longer active.  This fix is bogus for
non-admin accounts.  Sigh.

On Tue, Nov 10, 2009 at 7:51 PM, <thomasvl@chromium.org> wrote:

> driveby
>
>
> http://codereview.chromium.org/387016/diff/5001/5003
>
> File base/mac_util.mm (right):
>
> http://codereview.chromium.org/387016/diff/5001/5003#newcode241
> Line 241: NSString* filePath =
> CHECK/DCHECK for file_path.IsAbsolute()?
>
> http://codereview.chromium.org/387016/diff/5001/5003#newcode250
> Line 250: CSBackupSetItemExcluded((CFURLRef)url, exclude, true) ==
> noErr;
> drive by: don't the docs talk about needing to run as admin?  (To change
> the backup exclusion status of a path, your application must be running
> with administrator privileges.)  does that mean for non admins, we'll
> still back stuff up?
>
>
> http://codereview.chromium.org/387016
>

[TVL, 2009-11-12 20:34:17]

fyi bug id is 25959, you seem to have an extra digit in the cl description.  ;)

Is it worth going ahead and landing this moving the blocker on the bug to m5 and
continuing to work on this?

[pink (ping after 24hrs), 2009-11-12 20:37:45]

I believe that's what Mike and I=A0decided. It doesn't work correctly
for non-admins, but that's such a small number of cases, I'd rather do
something positive for the 95% case than nothing for everyone.

On Thu, Nov 12, 2009 at 3:34 PM, <thomasvl@chromium.org> wrote:
>
> fyi bug id is 25959, you seem to have an extra digit in the cl descriptio=
n. =A0;)
>
> Is it worth going ahead and landing this moving the blocker on the bug to=
 m5 and
> continuing to work on this?
>
> http://codereview.chromium.org/387016



--
Mike Pinkerton
Mac Weenie
pinkerton@google.com