852034 - Replace restoreBookmarksFromJSONFile with importFromFile

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Andres Hernandez [:andreshm]]

Replace PlacesUtils.restoreBookmarksFromJSONFile calls with BookmarkJSONUtils.importFromFile async call.

Comment 1

[Raymond Lee [:raymondlee]]

Should land the patch in Bug 852041 because this patch would touch some common files

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

Updated this patch.  This patch should be applied after the patch in bug 852041

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 729427 [details] [diff] [review]
v2

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

Due to lack of time I just skimmed through the patch quickly, a more complete review is still needed, so forwarding that to Mano

::: browser/components/places/content/places.js
@@ +442,5 @@
> +      }
> +      catch(ex) {
> +        PlacesOrganizer._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
> +      }
> +    });

could just use the failure then() callback, instead of a task

::: browser/components/places/tests/unit/test_browserGlue_prefs.js
@@ +56,5 @@
>          }
>          catch(ex) {}
>  
> +        // Give time for BrowserGlue._initPlaces to complete before next test.
> +        do_execute_soon(run_next_test);

looks fragile, maybe we want a notification like places-browser-init-complete (see this patch I made some time ago for other reasons: https://bug839996.bugzilla.mozilla.org/attachment.cgi?id=715540) ?

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to Marco Bonardo [:mak] (Away Mar 27 - Apr 2) from comment #4)
> Comment on attachment 729427 [details] [diff] [review]
> v2
> 
> Review of attachment 729427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Due to lack of time I just skimmed through the patch quickly, a more
> complete review is still needed, so forwarding that to Mano
> 
> ::: browser/components/places/content/places.js
> @@ +442,5 @@
> > +      }
> > +      catch(ex) {
> > +        PlacesOrganizer._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
> > +      }
> > +    });
> 
> could just use the failure then() callback, instead of a task

Done

> 
> ::: browser/components/places/tests/unit/test_browserGlue_prefs.js
> @@ +56,5 @@
> >          }
> >          catch(ex) {}
> >  
> > +        // Give time for BrowserGlue._initPlaces to complete before next test.
> > +        do_execute_soon(run_next_test);
> 
> looks fragile, maybe we want a notification like
> places-browser-init-complete (see this patch I made some time ago for other
> reasons: https://bug839996.bugzilla.mozilla.org/attachment.cgi?id=715540) ?

Updated

Comment 6

[Asaf Romano (gone)]

Comment on attachment 730112 [details] [diff] [review]
v3

A couple of things:
1) In places.js, please adopt Task.spawn rather than postponing promise.resolve by main-thread self-dispatch. Please do so in both instances there. It'll result in a much more readable code.
2) Please also attach a -w diff.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to Mano from comment #6)
> Comment on attachment 730112 [details] [diff] [review]
> v3
> 
> A couple of things:
> 1) In places.js, please adopt Task.spawn rather than postponing
> promise.resolve by main-thread self-dispatch. Please do so in both instances
> there. It'll result in a much more readable code.

Done.

> 2) Please also attach a -w diff.

This s a -w diff

Comment 8

[Asaf Romano (gone)]

Comment on attachment 735629 [details] [diff] [review]
v4


>-    try {
>-      PlacesUtils.restoreBookmarksFromJSONFile(aFile);
>-    }
>-    catch(ex) {
>-      this._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
>-    }
>+    Task.spawn(function() {
>+      yield BookmarkJSONUtils.importFromFile(aFile, true);
>+    }).then(null, function() {
>+      PlacesOrganizer._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
>+    });
>   },

Hrm, why not:

    Task.spawn(function() {
      try {
        yield BookmarkJSONUtils.importFromFile(aFile, true);
      }
      catch(ex) {
        PlacesOrganizer._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
      }
    });

Comment 9

[Asaf Romano (gone)]

Also, it seems you're using |yield| which are probably not yet using the Task system (unless that's the default now).

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v5

(In reply to Mano from comment #8)
> Comment on attachment 735629 [details] [diff] [review]
> v4
> 
> 
> >-    try {
> >-      PlacesUtils.restoreBookmarksFromJSONFile(aFile);
> >-    }
> >-    catch(ex) {
> >-      this._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
> >-    }
> >+    Task.spawn(function() {
> >+      yield BookmarkJSONUtils.importFromFile(aFile, true);
> >+    }).then(null, function() {
> >+      PlacesOrganizer._showErrorAlert(PlacesUIUtils.getString("bookmarksRestoreParseError"));
> >+    });
> >   },
> 
> Hrm, why not:
> 
>     Task.spawn(function() {
>       try {
>         yield BookmarkJSONUtils.importFromFile(aFile, true);
>       }
>       catch(ex) {
>        
> PlacesOrganizer._showErrorAlert(PlacesUIUtils.
> getString("bookmarksRestoreParseError"));
>       }
>     });

Either way works so I have updated the patch to what you suggested :-)

(In reply to Mano from comment #9)
> Also, it seems you're using |yield| which are probably not yet using the
> Task system (unless that's the default now).

As far as I know, |yield| works with Task system.

@marco: can you comment on that please?

Comment 11

[Asaf Romano (gone)]

Sorry, I meant that I'm not sure these tests are using the Task system already.

Comment 12

[Asaf Romano (gone)]

importFromFile: function(aFile, aReplace) still uses main-thread dispatching rather than Task.spawn.

Comment 13

[Marco Bonardo [:mak]]

not sure which was the exact question, though xpcshell-tests use task (and can use yield) if they use add_task to add sub tests.
In any other case (included mochitests) there is not yet any centralized helper.

Comment 14

[Asaf Romano (gone)]

Right, and as far as I can tell, most (if not all) of the tests touched here didn't adopt add_task yet.

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #14)
> Right, and as far as I can tell, most (if not all) of the tests touched here
> didn't adopt add_task yet.

You have to apply patches for bug 852041 first.  Those tests touched by this patch would already using yield with either add_task or do_test_pending() and do_test_finished().

Comment 16

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #12)
> importFromFile: function(aFile, aReplace) still uses main-thread dispatching
> rather than Task.spawn.

I think we can even don't need to use main-thread dispatching and Task.spawn. Let me create another patch.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v6

Comment 18

[Raymond Lee [:raymondlee]]

This bug is blocking other bugs we are trying to land.

mano: could you review the latest patch if you have a chance please? thanks

Comment 19

[Asaf Romano (gone)]

Comment on attachment 735760 [details] [diff] [review]
v6

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

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +84,5 @@
> +   */
> +  importFromFile: function(aFile, aReplace) {
> +    if (aFile.exists()) {
> +      return this.importFromURL(NetUtil.newURI(aFile).spec, aReplace);
> +    } else {

no |else| after return.

I still think that using Task.spawn is the most safe & readable approach, but it's your call.

@@ +91,5 @@
> +
> +      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
> +      deferred.reject(new Error("File does not exist."));
> +
> +      return deferred.promise;

Hrm, does this actually work? I mean, isn't the promise rejected before it's returned?

This stuff is new for me too.

Comment 20

[Raymond Lee [:raymondlee]]

(In reply to Mano from comment #19)
> Comment on attachment 735760 [details] [diff] [review]
> v6
> 
> Review of attachment 735760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/BookmarkJSONUtils.jsm
> @@ +84,5 @@
> > +   */
> > +  importFromFile: function(aFile, aReplace) {
> > +    if (aFile.exists()) {
> > +      return this.importFromURL(NetUtil.newURI(aFile).spec, aReplace);
> > +    } else {
> 
> no |else| after return.
> 
> I still think that using Task.spawn is the most safe & readable approach,
> but it's your call.
> 
> @@ +91,5 @@
> > +
> > +      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
> > +      deferred.reject(new Error("File does not exist."));
> > +
> > +      return deferred.promise;
> 
> Hrm, does this actually work? I mean, isn't the promise rejected before it's
> returned?
> 
> This stuff is new for me too.

Lets ask marco's input.

marco: what do you think about the above code? Shall we wrap the deferred.rejected(new Error("File does not exist.")) with Task.spawn?

Comment 21

[Asaf Romano (gone)]

If you use Task.spawn you don't need to create the promise yourself.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: v7

(In reply to Raymond Lee [:raymondlee] from comment #20)
> > 
> > @@ +91,5 @@
> > > +
> > > +      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
> > > +      deferred.reject(new Error("File does not exist."));
> > > +
> > > +      return deferred.promise;
> > 
> > Hrm, does this actually work? I mean, isn't the promise rejected before it's
> > returned?
> > 
> > This stuff is new for me too.
> 
> Lets ask marco's input.
> 
> marco: what do you think about the above code? Shall we wrap the
> deferred.rejected(new Error("File does not exist.")) with Task.spawn?

I have updated it as below.

+    if (aFile.exists()) {
+      return this.importFromURL(NetUtil.newURI(aFile).spec, aReplace);
+    }
+
+    notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN);
+
+    return Task.spawn(function() {
+      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_FAILED);
+      throw new Error("File does not exist.");
+    });


Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=3629b2329d39

Comment 23

[Raymond Lee [:raymondlee]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a7052013f5be