852032 - Create a PlacesBackups.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Andres Hernandez [:andreshm]]

Move the backups PlacesUtils code to a new jsm called PlacesBackups.jsm, that handle all backups calls.

Comment 1

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 2

[Raymond Lee [:raymondlee]]

When you file new bugs to switch to use methods in PlacesBackups.jsm instead of PlacesUtils backups, please also address bug 852041 comment 1

Comment 3

[Marco Bonardo [:mak]]

we need bugs to converts callers and then remove the backups object from PlacesUtils

I'd also ask to please coordinate between you, if you are both working on the same piece of code it's really hard for me to make reviews since stuff keeps bitrotting across bugs and order of the patches is not manageable.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 727527 [details] [diff] [review]
Patch v1

more specifically, either we first fix backups to be smarter and them move to the module, or we do the opposite, we can't risk to fix PlacesUtils.backups and then having to port again the fixes to a separate module...
I'm fine both ways, but I need to know in which order you plan to work on this.

IMO we should first of all move PlacesUtils.backups to PlacesBackups, fix all of the callers, remove PlacesUtils.backups, then make backups smarter.
And we should also remove all duplicate code from PlacesUtils regarding bug 822200 before doing anything with backups since that copied some of the json code.

Next week I will put up and etherpad with an ordered list of tasks to complete, otherwise we are just risking to overwrite each other work.

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #4)
> Comment on attachment 727527 [details] [diff] [review]
> Patch v1
> 
> more specifically, either we first fix backups to be smarter and them move
> to the module, or we do the opposite, we can't risk to fix
> PlacesUtils.backups and then having to port again the fixes to a separate
> module...
> I'm fine both ways, but I need to know in which order you plan to work on
> this.
> 
> IMO we should first of all move PlacesUtils.backups to PlacesBackups, fix
> all of the callers, remove PlacesUtils.backups, then make backups smarter.
> And we should also remove all duplicate code from PlacesUtils regarding bug
> 822200 before doing anything with backups since that copied some of the json
> code.
>

Yes, lets finish the PlacesBackups piece first.
 
> Next week I will put up and etherpad with an ordered list of tasks to
> complete, otherwise we are just risking to overwrite each other work.

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 730866 [details] [diff] [review]
Patch v2

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

::: toolkit/components/places/PlacesBackups.jsm
@@ +124,5 @@
> +   *
> +   * @param aFile
> +   *        nsIFile where to save JSON backup.
> +   */
> +  saveBookmarksToJSONFile: function PB_saveBookmarksToFile(aFile) {

please fix the label and name to be coherent

@@ +158,5 @@
> +          }
> +          aFile.copyTo(this.folder, name);
> +        }
> +      }
> +    }.bind(this));

I think you can use the new arrow functions to avoid the bind as:
return Task.spawn(() => {
  ...
});

@@ +215,5 @@
> +      if (newBackupFile.exists())
> +        throw new Task.Result();
> +
> +      yield this.saveBookmarksToJSONFile(newBackupFile);
> +    }.bind(this));

ditto

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 730866 [details] [diff] [review]
Patch v2

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

ehr, you should add the new module to Makefile.in EXTRA_JS_MODULES

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v3

Line: 139(In reply to Marco Bonardo [:mak] from comment #7)
> Comment on attachment 730866 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 730866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/PlacesBackups.jsm
> @@ +124,5 @@
> > +   *
> > +   * @param aFile
> > +   *        nsIFile where to save JSON backup.
> > +   */
> > +  saveBookmarksToJSONFile: function PB_saveBookmarksToFile(aFile) {
> 
> please fix the label and name to be coherent

Done

> 
> @@ +158,5 @@
> > +          }
> > +          aFile.copyTo(this.folder, name);
> > +        }
> > +      }
> > +    }.bind(this));
> 
> I think you can use the new arrow functions to avoid the bind as:
> return Task.spawn(() => {
>   ...
> });
> 
> @@ +215,5 @@
> > +      if (newBackupFile.exists())
> > +        throw new Task.Result();
> > +
> > +      yield this.saveBookmarksToJSONFile(newBackupFile);
> > +    }.bind(this));
> 
> ditto

I got the following error so I didn't make the change
Error: SyntaxError: arrow function may not contain yield

> ehr, you should add the new module to Makefile.in EXTRA_JS_MODULES

Fixed

Comment 10

[Marco Bonardo [:mak]]

(In reply to Raymond Lee [:raymondlee] from comment #9)
> I got the following error so I didn't make the change
> Error: SyntaxError: arrow function may not contain yield

Ah, good to know, we don't have full documentation of arrow functions yet, so this is an interesting information.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 12

[Raymond Lee [:raymondlee]]

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=4ae38e73c05f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9b27b090db

Comment 14

[Raymond Lee [:raymondlee]]

This patch has been backed out with patch for bug 854288.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae57a618325

However, I don't see any issue with this patch.  Please help to check this patch in again.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42a879409f18

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42a879409f18

Comment 17

[[e-mail]]

Comment on attachment 735647 [details] [diff] [review]
Patch for check-in

>+      new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-([0-9-]+)\.(json|html)");
...
>+      let rx = new RegExp("\." + fileExt + "$");

Unfortunately "\." is just the same as "." and so new RegExp("\.") matches any character. To match a "." you need to use new RegExp("\\.") instead.

Comment 18

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #17)
> Unfortunately "\." is just the same as "." and so new RegExp("\.") matches
> any character. To match a "." you need to use new RegExp("\\.") instead.

filed bug 1116600.