855638 - Remove previous calls to PlacesUtils.backups and move them to PlacesBackups

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

Once PlacesBackups.jsm is landed, we should remove previous calls to PlacesUtils.backups and move them to PlacesBackups.

Comment 1

[Raymond Lee [:raymondlee]]

This bug is to move the calls, not to remove PlacesUtils.backups.  Bug 857429 would handle remove and deprecate PlacesUtils.backups.

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - Update browser/

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: Part 2 - Update toolkit/places

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 732716 [details] [diff] [review]
Part 1 - Update browser/

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

::: toolkit/components/places/Makefile.in
@@ +67,5 @@
>  
>  EXTRA_PP_JS_MODULES = \
>    BookmarkHTMLUtils.jsm \
>    PlacesUtils.jsm \
> +  PlacesBackups.jsm \

should be done in the PlacesBackups patch (just added a note)

bot not in the _PP_ section since it doesn't appear to have any preprocessor directives, rather in an EXTRA_JS_MODULE

::: toolkit/components/places/PlacesBackups.jsm
@@ +38,5 @@
>      delete this.folder;
>      return this.folder = bookmarksBackupDir;
>    },
>  
> +  get profileRelativeFolderPath() "bookmarkbackups",

please file a bug to check if we can remove profileRelativeFolderPath from PlacesUtils, or otherwise mark it as deprecated if add-ons rely on it.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 732734 [details] [diff] [review]
Part 2 - Update toolkit/places

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

::: toolkit/components/places/tests/head_common.js
@@ +36,5 @@
>  
>  // This imports various other objects in addition to PlacesUtils.
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");
>  Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");
> +Cu.import("resource://gre/modules/PlacesBackups.jsm");

the last 2 should rather be defineLazyModuleGetter

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - Update browser/ v2

> please file a bug to check if we can remove profileRelativeFolderPath from
> PlacesUtils, or otherwise mark it as deprecated if add-ons rely on it.

Filed bug 859151

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Part 2 - toolkit/places v2

(In reply to Marco Bonardo [:mak] from comment #5)
> Comment on attachment 732734 [details] [diff] [review]
> Part 2 - Update toolkit/places
> 
> Review of attachment 732734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/tests/head_common.js
> @@ +36,5 @@
> >  
> >  // This imports various other objects in addition to PlacesUtils.
> >  Cu.import("resource://gre/modules/PlacesUtils.jsm");
> >  Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");
> > +Cu.import("resource://gre/modules/PlacesBackups.jsm");
> 
> the last 2 should rather be defineLazyModuleGetter

Fixed

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - Update browser/ v3

Added back the profileRelativeFolderPath() into PlacesBackups

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - Update browser/ v4

Resolve conflict with latest code

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Part 1 - patch for checkin

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Part 2 - patch for checkin

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

Please ensure that bug 852034 is checked in first.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7426b6e43f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7949ba1f68

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa7426b6e43f
https://hg.mozilla.org/mozilla-central/rev/0e7949ba1f68

Comment 14

[Richard Newman [:rnewman]]

You missed /services/!