412695 - Don't write empty/zero values in bookmarks.plist for last-visited date, visit count, and status

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Mark Mentovai]

We can save some more space in bookmarks.plist.

Comment 1

[Mark Mentovai]

Attachment: Check for nonzero values before adding these keys

Comment 2

[Mark Mentovai]

Hmm, looks like we're also writing empty UUID strings for folders - we don't need to do that either.

Comment 3

[Mark Mentovai]

Attachment: Handles folders too

We currently do assign a UUID to newly-created folders.  Do we need to?

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #3)
> Created an attachment (id=297442) [details]
> Handles folders too
> 
> We currently do assign a UUID to newly-created folders.  Do we need to?

I think they were using the UUIDs for the .Mac syncing over in bug 228123.  That effort seems permanently stalled, but the schema seemed still useful and Markus has expressed interest in working on the bug, so if the UUID is useful for that, I'd not be too quick to remove it.

Comment 5

[Stuart Morgan]

Comment on attachment 297442 [details] [diff] [review]
Handles folders too

Oh, yuck. savedLastVisit is just |return mLastVisit|, which means we violated the "never return nil" aspect of those methods (same with savedURL). I don't understand how writing bookmarks even works correctly right now, since we should be getting nil insert exceptions. Could you fix savedURL as part of this patch?

All the if (foo) checks you added should used the normal accessor, rather than the savedFoo accessor, since nil is now okay. savedLastVisit can be removed entirely.

r=me with those changes.

We lucked out here that old builds can handle not having the lastVisit in the file without ending up with a nil date, otherwise doing this would be really tricky without breaking our one-release backwards compatibility policy for files we write.

Comment 6

[Mark Mentovai]

Bookmark writing "worked" because +[NSMutableDictionary dictionaryWithObjectsAndKeys:] stops when it hits a nil object instead of throwing an exception.

>   NSMutableDictionary* itemDict = [NSMutableDictionary dictionaryWithObjectsAndKeys:
>                 [self savedTitle], BMTitleKey,
>                   [self savedURL], BMURLKey,
>-            [self savedLastVisit], BMLastVisitKey,
>-       [self savedNumberOfVisits], BMNumberVisitsKey,
>-               [self savedStatus], BMStatusKey,
>                                    nil];

When savedLastVisit was nil, we'd just skip writing savedNumberOfVisits and savedStatus.  savedNumberOfVisits would be 0 (now) in that case anyway, and so would savedStatus, since separators are the only Bookmark objects with a nonzero status, and they're handled specially above.

I'll fix savedURL, dump savedLastVisit, and use the accessors capable of returning native types and nil pointers for the conditional checks.

Comment 7

[Stuart Morgan]

Oh, right, duh. Blind luck FTW!

Comment 8

[Mark Mentovai]

Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b2.

Comment 9

[Mark Mentovai]

Actually, we do use the UUID for things like "last used folder", I just didn't see UUIDs being populated for folders after the very first launch because they're not assigned until they're needed.