Create a PlacesBackups.jsm

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: andreshm, Assigned: andreshm)

Tracking

unspecified
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Move the backups PlacesUtils code to a new jsm called PlacesBackups.jsm, that handle all backups calls.
(Assignee)

Updated

6 years ago
Depends on: 822200
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 727527 [details] [diff] [review]
Patch v1
Attachment #727527 - Flags: feedback?(mak77)
When you file new bugs to switch to use methods in PlacesBackups.jsm instead of PlacesUtils backups, please also address bug 852041 comment 1
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 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.
Attachment #727527 - Flags: feedback?(mak77)
(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.
Depends on: 852041
(Assignee)

Updated

6 years ago
Depends on: 854761
Blocks: 846644
Blocks: 855638
(Assignee)

Comment 6

6 years ago
Created attachment 730866 [details] [diff] [review]
Patch v2
Attachment #727527 - Attachment is obsolete: true
Attachment #730866 - Flags: review?(mak77)
Blocks: 818593
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
Attachment #730866 - Flags: review?(mak77) → review+
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
Created attachment 734405 [details] [diff] [review]
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
Attachment #730866 - Attachment is obsolete: true
(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.
Created attachment 735647 [details] [diff] [review]
Patch for check-in
Attachment #734405 - Attachment is obsolete: true
Keywords: checkin-needed
No longer depends on: 854761
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/42a879409f18
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.

Updated

4 years ago
Depends on: 1116600
(In reply to neil@parkwaycc.co.uk 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.
You need to log in before you can comment on or make changes to this bug.