Closed Bug 818593 Opened 7 years ago Closed 6 years ago

Add file size to bookmarks restore UI

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: rnewman, Assigned: raymondlee)

References

Details

Attachments

(1 file, 12 obsolete files)

Users need some way to pick which bookmarks to restore. If we can do a bookmark count, too, that would be excellent.
I think the bookmark count is tricky, unless we store a meta file with this data along with the backup.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
This is the first part which adds the file size.  I am looking into second part to add bookmark count.
Attachment #715916 - Flags: review?(mak77)
Attachment #715916 - Attachment description: Add file size to restore bookmark menuitems → Part 1 - Add file size to restore bookmark menuitems
(In reply to Raymond Lee [:raymondlee] from comment #2)
> This is the first part which adds the file size.  I am looking into second
> part to add bookmark count.

I don't think it's easy at all to make that in a performant way for old backups, we should collect that count while adding stuff to the backup and then put it somewhere, maybe in the backup file name itself, provided the code should still be able to open old backups that don't have that additional data.
(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #3)
> (In reply to Raymond Lee [:raymondlee] from comment #2)
> > This is the first part which adds the file size.  I am looking into second
> > part to add bookmark count.
> 
> I don't think it's easy at all to make that in a performant way for old
> backups, we should collect that count while adding stuff to the backup and
> then put it somewhere, maybe in the backup file name itself, provided the
> code should still be able to open old backups that don't have that
> additional data.

saveBookmarksToJSONFile(aFile) -> _writeBackupFile(aFile) -> serializeNodeAsJSONToOutputStream()
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1882

We can collect the count in serializeNodeAsJSONToOutputStream() but file is already initialized with filename.  Therefore, I am thinking we can store all backup filenames and bookmark counts in a preferences as a JSON string e.g. {"bookmarks-2013-01-30.json": 123, "bookmarks-2013-02-04.json", : 100, ...}
(In reply to Raymond Lee [:raymondlee] from comment #4)
> Therefore, I am thinking we can store
> all backup filenames and bookmark counts in a preferences as a JSON string
> e.g. {"bookmarks-2013-01-30.json": 123, "bookmarks-2013-02-04.json", : 100,
> ...}

Would work, provided the user doesn't reset the profile with the reset profile feature or manually. If we consider this just a *hint* may even be fine to have some entry with the data and some without.
Comment on attachment 715916 [details] [diff] [review]
Part 1 - Add file size to restore bookmark menuitems

Review of attachment 715916 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/places.js
@@ +9,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "gStringBundle", function() {
> + return Services.strings.
> +   createBundle("chrome://browser/locale/places/places.properties");
> +});

there should be no need for this, just use PlacesUIUtils.getString and PlacesUIUtils.getFormattedString where needed
thus you can also remove Services.jsm and XPCOMUtils.jsm imports

@@ +368,5 @@
>  
>      // Populate menu with backups.
>      for (let i = 0; i < backupFiles.length; i++) {
>        let backupDate = PlacesUtils.backups.getDateForFile(backupFiles[i]);
> +      let size = this._convertByteUnits(backupFiles[i].fileSize);

ditto for the module.

@@ +379,5 @@
>                       dateSvc.FormatDate("",
>                                          Ci.nsIScriptableDateFormat.dateFormatLong,
>                                          backupDate.getFullYear(),
>                                          backupDate.getMonth() + 1,
> +                                        backupDate.getDate()) + 

trailing space

::: browser/locales/en-US/chrome/browser/places/places.properties
@@ +89,5 @@
>  lockPrompt.text=The bookmarks and history system will not be functional because one of %S's files is in use by another application. Some security software can cause this problem.
>  lockPromptInfoButton.label=Learn More
>  lockPromptInfoButton.accessKey=L
> +
> +fileSizeText=%1$S %2$S

please add a LOCALIZATION NOTE to express where this is used, and what 1 and 2 are going to be replaced by (may appear clear in english but in some locale they could invert them)

I'd also use a more specific name like backupFileSizeText for now

@@ +93,5 @@
> +fileSizeText=%1$S %2$S
> +bytes=bytes
> +kilobyte=KB
> +megabyte=MB
> +gigabyte=GB

may we use DownloadUtils.jsm instead?
It has a convertByteUnits util that is exactly what we need, and its string bundle too. And likely it's already loaded since we use it for the Downloads view.
Attachment #715916 - Flags: review?(mak77)
Attached patch v2 (obsolete) — Splinter Review
This patch contains the backup file size and bookmark count.
Attachment #716439 - Flags: review?(mak77)
Attachment #715916 - Attachment is obsolete: true
Comment on attachment 716439 [details] [diff] [review]
v2

Review of attachment 716439 [details] [diff] [review]:
-----------------------------------------------------------------

The only problem is that we store metadata for any backups, while would be better to store it only for automatic backups we do, since we don't track other backups stored elsewhere on the disk and the pref may grow without control, I suppose you may figure a way to tell the serializer if the backup has been created by PlacesUtils.backups.create... an optional aIsAutomaticBackup=false argument could do it I think.

::: browser/components/places/content/places.js
@@ +363,5 @@
>      for (let i = 0; i < backupFiles.length; i++) {
>        let backupDate = PlacesUtils.backups.getDateForFile(backupFiles[i]);
> +      let bookmarkCount = PlacesUtils.backups.getBookmarkCountForFile(backupFiles[i]);
> +      let [size, unit] = DownloadUtils.convertByteUnits(backupFiles[i].fileSize);
> +      let sizeString = DownloadsCommon.strings.sizeWithUnits(size, unit);

you should defineLazyModuleGetter DownloadUtils at the top, it's in scope cause we have the downloads overlay in Firefox but other consumers may not have it, better staying on the safe side.
For similar reasons you cannot use DownloadsCommon, it's a Firefox browser component module, Seamonkey doesn't have that component and relying on it like this is fragile regardless since comes from a parallel component. So we still need the backupFileSizeText string

@@ +364,5 @@
>        let backupDate = PlacesUtils.backups.getDateForFile(backupFiles[i]);
> +      let bookmarkCount = PlacesUtils.backups.getBookmarkCountForFile(backupFiles[i]);
> +      let [size, unit] = DownloadUtils.convertByteUnits(backupFiles[i].fileSize);
> +      let sizeString = DownloadsCommon.strings.sizeWithUnits(size, unit);
> +      let extraInfo;

let's name this sizeInfo

@@ +366,5 @@
> +      let [size, unit] = DownloadUtils.convertByteUnits(backupFiles[i].fileSize);
> +      let sizeString = DownloadsCommon.strings.sizeWithUnits(size, unit);
> +      let extraInfo;
> +      if (bookmarkCount != null) {
> +        extraInfo = " (" + sizeString + " - " + 

trailing space

::: toolkit/components/places/PlacesUtils.jsm
@@ +1975,5 @@
> +     * @param aURICount
> +     *        int The number of bookmarks in the backup.
> +     */
> +    _updateMetaDataForBackup:
> +    function PU_B__updateMetaDataForBackup(aAddOrUpdate, aFile, aURICount) {

why the need for aAddOrUpdate, could it just remove the data if it's === null?
Provided we don't need to store null data clearly, that is our case.
aURICount could instead be an object containing the metaData, like {uriCount: N}, so in future we may add more metadata if needed.
so _updateMetaDataForBackupFile(aFile, aMetaData)

@@ +1976,5 @@
> +     *        int The number of bookmarks in the backup.
> +     */
> +    _updateMetaDataForBackup:
> +    function PU_B__updateMetaDataForBackup(aAddOrUpdate, aFile, aURICount) {
> +      let prefName = "browser.bookmarks.backupsMetaData";

move this to PlacesUtils.backups.METADATA_PREF

@@ +1982,5 @@
> +      let metaString;
> +      let metaData;
> +
> +      try {
> +        metaString = preferences.getCharPref(prefName);

I suppose you may use Services.prefs?

@@ +1984,5 @@
> +
> +      try {
> +        metaString = preferences.getCharPref(prefName);
> +      } catch(e) { /* invalid pref data */ }
> + 

trailing space

@@ +1986,5 @@
> +        metaString = preferences.getCharPref(prefName);
> +      } catch(e) { /* invalid pref data */ }
> + 
> +      try {
> +        metaData = JSON.parse(metaString);

looks like this could just be moved inside the previous try/catch and let metaString kept inside the try scope

@@ +2021,5 @@
> +      } catch(e) { /* invalid pref data */ }
> +
> +      try {
> +        metaData = JSON.parse(metaString);
> +      } catch (e) { /* invalid state object - don't restore anything */ }

ditto, merge with the previous try

::: toolkit/components/places/tests/bookmarks/test_818593.js
@@ +1,1 @@
> +Cu.import("resource://gre/modules/NetUtil.jsm");

not needed, since head_common.js imports this for you already

rather, please add the license header and a comment with a brief description of what the test is testing

@@ +11,5 @@
> +  // save to file
> +  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIProperties);
> +  let backupFile = dirSvc.get("TmpD", Ci.nsILocalFile);
> +  backupFile.append("bookmark_" + Date.now() + ".json");

you can probably do
let backupFile = FileUtils.getFile("TmpD", ["bookmark_" + Date.now() + ".json"]);

btw, what's the point of Date.now(), why not just bookmarks.json?

@@ +12,5 @@
> +  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIProperties);
> +  let backupFile = dirSvc.get("TmpD", Ci.nsILocalFile);
> +  backupFile.append("bookmark_" + Date.now() + ".json");
> +  backupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, 0600);

this will complain octals are deprecated, rather use parseInt("0600", 8)

::: toolkit/components/places/tests/bookmarks/xpcshell.ini
@@ +28,5 @@
>  [test_removeItem.js]
>  [test_savedsearches.js]
>  [test_675416.js]
>  [test_711914.js]
> +[test_818593.js]

please give the tests meaningful names, we are trying to stop using bare bug numbers
Attachment #716439 - Flags: review?(mak77)
Attached patch v3 (obsolete) — Splinter Review
Addressed issues in comment 8
Attachment #716439 - Attachment is obsolete: true
Attachment #717011 - Flags: review?(mak77)
Attached patch v4 (obsolete) — Splinter Review
Minor changes
Attachment #717011 - Attachment is obsolete: true
Attachment #717011 - Flags: review?(mak77)
Attachment #717012 - Flags: review?(mak77)
Comment on attachment 717012 [details] [diff] [review]
v4

Review of attachment 717012 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't touch PU.backups.create arguments, apart that this looks good.

::: browser/components/nsBrowserGlue.js
@@ +1178,5 @@
>          maxBackups = Services.prefs.getIntPref("browser.bookmarks.max_backups");
>        }
>        catch(ex) { /* Use default. */ }
>  
> +      PlacesUtils.backups.create(maxBackups, false, true); // Don't force creation.

there is no need to change this line, create should always create automatic backups, so it doesn't need a new argument.

::: browser/components/places/content/places.js
@@ +370,5 @@
> +      let [size, unit] = DownloadUtils.convertByteUnits(backupFiles[i].fileSize);
> +      let sizeString = PlacesUtils.getFormattedString("backupFileSizeText",
> +                                                      [size, unit]);
> +      let sizeInfo;
> +      if (bookmarkCount != null) {

please move the bookmarkCount assign near its first usage, here.

::: toolkit/components/places/PlacesUtils.jsm
@@ +1478,5 @@
>     * @param   aResolveShortcuts
>     *          Converts folder shortcuts into actual folders. 
>     * @param   aExcludeItems
>     *          An array of item ids that should not be written to the backup.
> +   * @returns the number of URIs.

should be @return without the s

@@ +1766,5 @@
>     * Helper to create and manage backups.
>     */
>    backups: {
>  
> +    METADATA_PREF : "browser.bookmarks.backupsMetaData",

please semicolon towards the label, no whitespace there

@@ +1886,5 @@
>       * @param aFile
>       *        nsIFile where to save JSON backup.
> +     *
> +     * @param [optional] bool aIsAutomaticBackup
> +     *                        Updates meta data if it's automatic backup.

add "Defaults to false."

@@ +1994,5 @@
> +      if (!metaData) {
> +        metaData = {};
> +      }
> +
> +      if (aMetaData && aMetaData.uriCount != undefined) {

rather Object.keys(aMetaData).length > 0, don't make this depend on uriCount or any other specific entry

@@ +2048,3 @@
>       */
>      create:
> +    function PU_B_create(aMaxBackups, aForceBackup, aIsAutomaticBackup) {

The scope of backups.create is to do automatic backups, so it should not have an argument to do that, it should just always pass true to saveBookmarksToJSONFile
Indeed it doesn't take a path to a backup file, since it generates one.

::: toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js
@@ +18,5 @@
> +  let backupFile = FileUtils.getFile("TmpD", ["bookmark_.json"]);
> +  backupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, parseInt("0600", 8));
> +
> +  // 1) test with isAutomaticBackup flag equals to FALSE
> +  PlacesUtils.backups.saveBookmarksToJSONFile(backupFile, false);

the false argument is optional
Attachment #717012 - Flags: review?(mak77) → review+
please wait a moment to land while we complete the discussion in bug 818584, Richard suggest to store metadata in the filename, while I personally don't see a win in doing that, he has some points we should evaluate (btw, the changes here would be limited to the internal implementation)
sorry I forgot to follow-up here (please ping me next time :)), let's take Richard's suggestion to use the filename to store the count and (in future) the hash, since I don't think we'll add much more and we have to access the filesystem regardless.
Attached patch v5 (obsolete) — Splinter Review
Add bookmark count to the filename
Attachment #717012 - Attachment is obsolete: true
Attachment #725066 - Flags: review?(mak77)
Comment on attachment 725066 [details] [diff] [review]
v5

Review of attachment 725066 [details] [diff] [review]:
-----------------------------------------------------------------

please verify my previous comments in comment 11, you didn't apply the required changes to PlacesUtils.backups.create

::: toolkit/locales/en-US/chrome/places/places.properties
@@ +33,5 @@
> +
> +# LOCALIZATION NOTE
> +# The string is used for showing file size of each backup in the "fileRestorePopup" popup
> +# %1$S the size of the backup file size
> +# %2$S the unit of the backup file size

"is the file size unit"
Attachment #725066 - Flags: review?(mak77)
Attached patch v6 (obsolete) — Splinter Review
Addressed comments in comment 11 and fixed some tests.
Attachment #725066 - Attachment is obsolete: true
Attachment #725310 - Flags: review?(mak77)
Blocks: 818584
Comment on attachment 725310 [details] [diff] [review]
v6

Review of attachment 725310 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/places.js
@@ +369,5 @@
> +      let sizeString = PlacesUtils.getFormattedString("backupFileSizeText",
> +                                                      [size, unit]);
> +      let sizeInfo;
> +
> +      let bookmarkCount = PlacesUtils.backups.getBookmarkCountForFile(backupFiles[i]);

nit: remove newline

::: toolkit/components/places/PlacesUtils.jsm
@@ +1435,5 @@
>     * @param   aResolveShortcuts
>     *          Converts folder shortcuts into actual folders. 
>     * @param   aExcludeItems
>     *          An array of item ids that should not be written to the backup.
> +   * @return  the number of URIs.

the number of serialized uri nodes.

@@ +1732,5 @@
>        let localizedFilenamePrefix =
>          localizedFilename.substr(0, localizedFilename.indexOf("-"));
>        delete this._filenamesRegex;
>        return this._filenamesRegex =
> +        new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-([0-9-]+)(_[0-9]+)*\.(json|html)");

by renaming this method (removing the _) you are removing it's lazy getter behavior, see the delete above.

@@ +1847,3 @@
>       */
>      saveBookmarksToJSONFile:
> +    function PU_B_saveBookmarksToFile(aFile, aIsAutomaticBackup) {

So, since now we don't use anymore the prefs, the "generating ever-growing pref"  problem doesn't exist anymore.
Thus the condition is different, we should always store metadata in the filename if the user didn't change the filename, so if it's the default name we generated. This should avoid changes to the pseudo-API too.

::: toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js
@@ +21,5 @@
> +  // 1) test with isAutomaticBackup flag equals to FALSE
> +  PlacesUtils.backups.saveBookmarksToJSONFile(backupFile);
> +
> +  // it should return null because only write to preference when
> +  // isAutomaticBackup flag is TRUE

the test should be updated to take into account we don't write to prefs anymore and we don't distinguish "automatic"  backups, as I said above.

::: toolkit/components/places/tests/head_common.js
@@ +509,5 @@
> +  let bookmarksBackupDir = gProfD.clone();
> +  bookmarksBackupDir.append("bookmarkbackups");
> +  let files = bookmarksBackupDir.directoryEntries;
> +  let backup_date = new Date().toLocaleFormat("%Y-%m-%d");
> +  let rx = new RegExp("^bookmarks-" + backup_date + "(_.+)*\.json$");

I'd prefer a boolean stating if the backup is "automatic" or not, and check with a stricter regex

::: toolkit/components/places/tests/unit/test_utils_backups_create.js
@@ +68,3 @@
>      if (i > Math.floor(dates.length/2)) {
> +      let files = bookmarksBackupDir.directoryEntries;
> +      let rx = new RegExp("^" + PREFIX + dates[i] + "(_.+)*" + SUFFIX + "$");

the (_.+)* check is too loose, make it stricter please. backups created by create should always have the bookmark count, isn't it?
Attachment #725310 - Flags: review?(mak77)
Depends on: 852032
Depends on: 855638
I've tried to work on this but looking into the code. This bug also depends on patch for bug 860625
Depends on: 860625
Attached patch v7 (obsolete) — Splinter Review
Addressed the previous comments and switched to use PlacesBackups.
Attachment #725310 - Attachment is obsolete: true
Attachment #757340 - Flags: review?(mak77)
Attached patch v8 (obsolete) — Splinter Review
A test file is missing
Attachment #757340 - Attachment is obsolete: true
Attachment #757340 - Flags: review?(mak77)
Attachment #757768 - Flags: review?(mak77)
Comment on attachment 757768 [details] [diff] [review]
v8

Review of attachment 757768 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +562,5 @@
>     *          Converts folder shortcuts into actual folders.
>     * @param   aExcludeItems
>     *          An array of item ids that should not be written to the backup.
>     * @returns Task promise
> +   * @resolves the number of of serialized uri nodes.

typo: of of

::: toolkit/components/places/PlacesBackups.jsm
@@ +131,1 @@
>    saveBookmarksToJSONFile: function PB_saveBookmarksToJSONFile(aFile) {

So, this is called saveBookmarksToJSONFile, and that's what I'd expect as a consumer, that is stores my backup to the file I provide, instead now it changes the file name and thus stores the backup to another file.
The main issue is that after invoking this, I could not easily act on the backup, since some "random" data has been added to its name and my nsIFile pointer is dead.

There are 2 possibilities:

1. handle all of the moving in create(), by waiting for saveBookmartsToJSONFile to be resolved and make SBTJF resolve to the metadata, that is everything you need to do the moving. In any case when in saveBookmarkToJSONFile we copy the backup to the backups folder silently, we can add metadata there, since it's not a user's request. There's an edge case, that is when the user stores a manual backup in the backups folder... in such a case we'll likely keep it as is, it will miss the additional metadata.
2. when resolving the promise give back to the caller a nsIFile of the backup file and clarify in the javadoc that the input nsIFile is just used as an hint to gather a file name and a path, but the real file pointer will be given later.

Clearly 1 is a cleaner approach, but it may be very expensive from a refactoring point of view, I suggest evaluating how expensive it is and taking decision based on that. I can't guess off-hand.
please add a test to ensure any of the 2 approaches.

@@ +277,5 @@
> +    },
> +
> +   _getBackupFileForSameDate:
> +   function PB__getBackupFileForSameDate(aFilename) {
> +     let files = this.folder.directoryEntries;

as a side note this has relation with bug 859695, whatever of these 2 bugs lands first will bitrot the other, _getBackupFileForSameDate won't be able to be synchronous, maybe you already want to change it...

::: toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js
@@ +19,5 @@
> +  // save to file
> +  let backupFile = FileUtils.getFile("TmpD", ["bookmarks-2013-01-01.json"]);
> +  backupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, parseInt("0600", 8));
> +
> +  Task.spawn(function() {

why not using add_task?
Attachment #757768 - Flags: review?(mak77) → feedback+
Attached patch v9 (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #21)
> Comment on attachment 757768 [details] [diff] [review]
> v8
> 
> Review of attachment 757768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/BookmarkJSONUtils.jsm
> @@ +562,5 @@
> >     *          Converts folder shortcuts into actual folders.
> >     * @param   aExcludeItems
> >     *          An array of item ids that should not be written to the backup.
> >     * @returns Task promise
> > +   * @resolves the number of of serialized uri nodes.
> 
> typo: of of
> 
> ::: toolkit/components/places/PlacesBackups.jsm
> @@ +131,1 @@
> >    saveBookmarksToJSONFile: function PB_saveBookmarksToJSONFile(aFile) {
> 
> So, this is called saveBookmarksToJSONFile, and that's what I'd expect as a
> consumer, that is stores my backup to the file I provide, instead now it
> changes the file name and thus stores the backup to another file.
> The main issue is that after invoking this, I could not easily act on the
> backup, since some "random" data has been added to its name and my nsIFile
> pointer is dead.
> 
> There are 2 possibilities:
> 
> 1. handle all of the moving in create(), by waiting for
> saveBookmartsToJSONFile to be resolved and make SBTJF resolve to the
> metadata, that is everything you need to do the moving. In any case when in
> saveBookmarkToJSONFile we copy the backup to the backups folder silently, we
> can add metadata there, since it's not a user's request. There's an edge
> case, that is when the user stores a manual backup in the backups folder...
> in such a case we'll likely keep it as is, it will miss the additional
> metadata.
> 2. when resolving the promise give back to the caller a nsIFile of the
> backup file and clarify in the javadoc that the input nsIFile is just used
> as an hint to gather a file name and a path, but the real file pointer will
> be given later.
> 
> Clearly 1 is a cleaner approach, but it may be very expensive from a
> refactoring point of view, I suggest evaluating how expensive it is and
> taking decision based on that. I can't guess off-hand.
> please add a test to ensure any of the 2 approaches.

I have updated to use the approach 1.

> 
> @@ +277,5 @@
> > +    },
> > +
> > +   _getBackupFileForSameDate:
> > +   function PB__getBackupFileForSameDate(aFilename) {
> > +     let files = this.folder.directoryEntries;
> 
> as a side note this has relation with bug 859695, whatever of these 2 bugs
> lands first will bitrot the other, _getBackupFileForSameDate won't be able
> to be synchronous, maybe you already want to change it...
> 

I have updated this method to use OS.File and leave the others for bug 859695.  After using OS.File in placesBackups.js, I got issue in test_clearHistory_shutdown.js as I mentioned before in bug 859695 comment 5, and I have to update it. It's definitely something to do with the OS.File in _getBackupFileForSameDate() but I can't figure out why...

> +++ b/browser/components/places/tests/unit/test_clearHistory_shutdown.js
> @@ -14,18 +14,18 @@ const URIS = [
> , "http://b.example2.com/"
> , "http://c.example3.com/"
> ];
 
> const TOPIC_CONNECTION_CLOSED = "places-connection-closed";
 
> let EXPECTED_NOTIFICATIONS = [
>   "places-shutdown"
>+, "places-expiration-finished"
> , "places-will-close-connection"
>-, "places-expiration-finished"
> , "places-connection-closed"
> ];

> :::
> toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.
> js
> @@ +19,5 @@
> > +  // save to file
> > +  let backupFile = FileUtils.getFile("TmpD", ["bookmarks-2013-01-01.json"]);
> > +  backupFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, parseInt("0600", 8));
> > +
> > +  Task.spawn(function() {
> 
> why not using add_task?

Changed to use add_task.
Attachment #757768 - Attachment is obsolete: true
Attachment #759025 - Flags: review?(mak77)
Blocks: 859695
marco: could you review this patch when you have a chance please?
sorry, I had to delay it a bit to verify the shutdown problem
Comment on attachment 759025 [details] [diff] [review]
v9

Review of attachment 759025 [details] [diff] [review]:
-----------------------------------------------------------------

So, I'd appreciate the following manual testing, based on the patch:
1. manual testing of automatic backups
2. manual testing that creating a backup elsewhere also updates automatic backups
3. check how longer it takes to make backups (based the other bug that pointed out it takes much longer to restore them)
4. manual testing that clear history on shutdown still works as expected

::: toolkit/components/places/PlacesBackups.jsm
@@ +223,5 @@
>        }
>  
> +      let backupFile = yield this._getBackupFileForSameDate(newBackupFilename);
> +      if (backupFile) {
> +        // force backup so remove the same date backup. Otherwise, return it.

comment looks unclear, fwiw the code is quite self-documented, doesn't seem to be needed

::: toolkit/components/places/tests/bookmarks/test_818593-store-backup-metadata.js
@@ +27,5 @@
> +  do_check_true(nodeCount > 0);
> +  do_check_true(backupFile.exists());
> +  do_check_eq(backupFile.leafName, "bookmarks.json");
> +
> +  // Ensure the backup would be copied to our backups folder when the original 

trailing space

@@ +29,5 @@
> +  do_check_eq(backupFile.leafName, "bookmarks.json");
> +
> +  // Ensure the backup would be copied to our backups folder when the original 
> +  // backup is saved somewhere else.
> +  let recentBackup = PlacesBackups.getMostRecent(); 

trailing space

::: toolkit/components/places/tests/head_common.js
@@ +506,5 @@
>  
>  /**
>   * Check a JSON backup file for today exists in the profile folder.
>   *
> + * @param aIsAutomaticBackup The boolean indicates whether it's a automatic

nit: s/a automatic/an automatic/
Attachment #759025 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #25)
> Comment on attachment 759025 [details] [diff] [review]
> v9
> 
> Review of attachment 759025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, I'd appreciate the following manual testing, based on the patch:
> 1. manual testing of automatic backups

It's fine.

> 2. manual testing that creating a backup elsewhere also updates automatic
> backups

No because in Bookmarks Library > Backups... is actually using |BookmarkJSONUtils.exportToFile| instead of |PlacesBackups.saveBookmarksToJSONFile|
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#526 

Shall we change that to PlacesBackups.saveBookmarksToJSONFile?

> 3. check how longer it takes to make backups (based the other bug that
> pointed out it takes much longer to restore them)

Took around 1.5-2 seconds to backup 2000 bookmarks on Windows so I think that's fine.

> 4. manual testing that clear history on shutdown still works as expected
> 

Yes
Flags: needinfo?(mak77)
(In reply to Raymond Lee [:raymondlee] from comment #26)
> > 2. manual testing that creating a backup elsewhere also updates automatic
> > backups
> 
> No because in Bookmarks Library > Backups... is actually using
> |BookmarkJSONUtils.exportToFile| instead of
> |PlacesBackups.saveBookmarksToJSONFile|
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/places.js#526 
> 
> Shall we change that to PlacesBackups.saveBookmarksToJSONFile?

Yes, I think so.
Flags: needinfo?(mak77)
Attached patch v10 (obsolete) — Splinter Review
Replace |BookmarkJSONUtils.exportToFile| with |PlacesBackups.saveBookmarksToJSONFile| in places.js

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=59fe4829ad61
Attachment #759025 - Attachment is obsolete: true
(In reply to Raymond Lee [:raymondlee] from comment #28)
> Created attachment 771133 [details] [diff] [review]
> v10
> 
> Replace |BookmarkJSONUtils.exportToFile| with
> |PlacesBackups.saveBookmarksToJSONFile| in places.js
> 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=59fe4829ad61

There are some oranges in windows.  I've tried to investigate that and an error is thrown from test_browserGlue_shutdown.js.  In the third test, it tried to create a JSON backup directory "bookmarkbackups" (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/head_common.js#477), nsIFile.create() throws FILE_ACCESS_DENIED and I really couldn't figure out what causes
strange, well, Windows is the pickiest platform about file locks, but I never saw this kind of error.
I somehow suspect this is due to the OS.File.DirectoryIterator you added in getBackupFileForSameDate, being asynchronous may happen at any time and keep a lock on the directory
Attached patch v11 (obsolete) — Splinter Review
This should fix the issue, at least on my windows machine.

Push to try and wait for the results
https://tbpl.mozilla.org/?tree=Try&rev=2890f94c6a0d
Attachment #771133 - Attachment is obsolete: true
(In reply to Raymond Lee [:raymondlee] from comment #31)
> Created attachment 779255 [details] [diff] [review]
> v11
> 
> This should fix the issue, at least on my windows machine.
> 
> Push to try and wait for the results
> https://tbpl.mozilla.org/?tree=Try&rev=2890f94c6a0d

The issue about the file locks are gone. However, there is still an error only happens on WinXP.  I am going to investigate that.

11:02:26 ERROR - Return code: 1 
11:02:27 ERROR - Return code: 1 
11:19:34 WARNING - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\xpcshell\tests\toolkit\components\places\tests\unit\test_utils_backups_create.js | Test timed out 
11:19:34 WARNING - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\xpcshell\tests\toolkit\components\places\tests\unit\test_utils_backups_create.js | test failed (with xpcshell return code: 1), see following log: 
11:32:50 ERROR - Return code: 1
Attached patch v12 (obsolete) — Splinter Review
Updated the patch and it also passed all tests on Windows.
https://tbpl.mozilla.org/?tree=Try&rev=204a449a3684

@marco: please review it again.  Thanks
Attachment #779255 - Attachment is obsolete: true
Attachment #779825 - Flags: review?(mak77)
@marco: Please review this when you have time.  Thanks!
Flags: needinfo?(mak77)
I was doing this yesterday, but I got distracted, will surely do this today.
Flags: needinfo?(mak77)
Comment on attachment 779825 [details] [diff] [review]
v12

Review of attachment 779825 [details] [diff] [review]:
-----------------------------------------------------------------

It looks OK, I suppose the previous failures where something due to testing restrictions, not something that may happen in common product use?

::: browser/components/places/content/places.js
@@ +419,5 @@
>        return;
>  
>      // Populate menu with backups.
>      for (let i = 0; i < backupFiles.length; i++) {
> +      let [size, unit] = DownloadUtils.convertByteUnits(backupFiles[i].fileSize);

I'm a little bit worried of the main-thread IO we are generating each time the menu is opened.
We should try to address this concern in bug 859695 by reading all info through OS.File and delaying the popup opening, it won't be funny :(

::: browser/components/places/tests/unit/test_browserGlue_prefs.js
@@ +275,5 @@
>  {
>    // Create our bookmarks.html from bookmarks.glue.html.
>    create_bookmarks_html("bookmarks.glue.html");
> +  // Remove all JSON backups.
> +  remove_all_JSON_backups();

the comment is useless :)

::: browser/components/places/tests/unit/test_browserGlue_shutdown.js
@@ +123,5 @@
>      do_check_true(profileBookmarksJSONFile.exists());
>      do_check_eq(profileBookmarksJSONFile.lastModifiedTime, lastMod);
>      do_check_eq(profileBookmarksJSONFile.fileSize, fileSize);
>  
> +    finish_test();

rather, kill that finish_test function, it's useless... leave the do_test_finished() here
Attachment #779825 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #36)
> Comment on attachment 779825 [details] [diff] [review]
> v12
> 
> Review of attachment 779825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks OK, I suppose the previous failures where something due to testing
> restrictions, not something that may happen in common product use?
> 

Yes, it shouldn't happen in common product user.  It seems to only happen when trying to delete /backups directory and then create it again immediately.

> ::: browser/components/places/content/places.js
> @@ +419,5 @@
> >        return;
> >  
> >      // Populate menu with backups.
> >      for (let i = 0; i < backupFiles.length; i++) {
> > +      let [size, unit] = DownloadUtils.convertByteUnits(backupFiles[i].fileSize);
> 
> I'm a little bit worried of the main-thread IO we are generating each time
> the menu is opened.
> We should try to address this concern in bug 859695 by reading all info
> through OS.File and delaying the popup opening, it won't be funny :(
> 
> ::: browser/components/places/tests/unit/test_browserGlue_prefs.js
> @@ +275,5 @@
> >  {
> >    // Create our bookmarks.html from bookmarks.glue.html.
> >    create_bookmarks_html("bookmarks.glue.html");
> > +  // Remove all JSON backups.
> > +  remove_all_JSON_backups();
> 
> the comment is useless :)
> 

Removed.

> ::: browser/components/places/tests/unit/test_browserGlue_shutdown.js
> @@ +123,5 @@
> >      do_check_true(profileBookmarksJSONFile.exists());
> >      do_check_eq(profileBookmarksJSONFile.lastModifiedTime, lastMod);
> >      do_check_eq(profileBookmarksJSONFile.fileSize, fileSize);
> >  
> > +    finish_test();
> 
> rather, kill that finish_test function, it's useless... leave the
> do_test_finished() here

Fixed
Attachment #779825 - Attachment is obsolete: true
Pushed to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&rev=d946eb0e997e
(In reply to Raymond Lee [:raymondlee] from comment #38)
> Pushed to try and waiting for the results
> https://tbpl.mozilla.org/?tree=Try&rev=d946eb0e997e

Passed try
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1788bbaa0f6a
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1788bbaa0f6a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
You need to log in before you can comment on or make changes to this bug.