852040 - Apply use of BookmarkJSONUtils.importFromURL

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Andres Hernandez [:andreshm]]

See bug 822200 comment 7

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

marco: do you know someone who can test this patch?  I am on mac and not able to test it due to bug 847807

Comment 2

[Marco Bonardo [:mak]]

Maybe matt may help here

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 727053 [details] [diff] [review]
v1

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

::: browser/metro/components/BrowserStartup.js
@@ +59,5 @@
>        if (metroRootItems.length > 0)
>          return; // no need to do initial import
>      }
>  
> +    Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");

You need to import Task.jsm here too.

With that change, this patch works for me.

@@ +66,5 @@
> +      try {
> +        yield BookmarkJSONUtils.importFromURL("chrome://browser/locale/bookmarks.json", false);
> +      } catch (err) {
> +        // Report the error, but ignore it.
> +        Cu.reportError("Failed to load default bookmarks from bookmarks.json: " + err);

Would it make sense to move the error reporting into BookmarkJSONUtils?  Then you could just call importFromURL here and ignore the returned promise -- the entire Task.spawn block could be replaced by a single function call.

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Comment on attachment 727053 [details] [diff] [review]
> v1
> 
> Review of attachment 727053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/components/BrowserStartup.js
> @@ +59,5 @@
> >        if (metroRootItems.length > 0)
> >          return; // no need to do initial import
> >      }
> >  
> > +    Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");
> 
> You need to import Task.jsm here too.

Thanks for checking that. ;_)

> 
> With that change, this patch works for me.
> 
> @@ +66,5 @@
> > +      try {
> > +        yield BookmarkJSONUtils.importFromURL("chrome://browser/locale/bookmarks.json", false);
> > +      } catch (err) {
> > +        // Report the error, but ignore it.
> > +        Cu.reportError("Failed to load default bookmarks from bookmarks.json: " + err);
> 
> Would it make sense to move the error reporting into BookmarkJSONUtils? 
> Then you could just call importFromURL here and ignore the returned promise
> -- the entire Task.spawn block could be replaced by a single function call.

Since this is the only caller, it makes sense to have Task.spawn block in BookmarkJSONUtils.  However, we might want to keep the same coding convention as other methods e.g. BookmarkJSONUtils.importFromFile which returns promise.  Furthermore, if the entire Task.spawn block is moved to BookmarkJSONUtils, we lose the ability to execute other code after BookmarkJSONUtils.importFromURL is invoked as it's an asynchronous method.

Marco: what's your opinion?

Comment 5

[Marco Bonardo [:mak]]

it's not common to use task spawn internally, other existing methods in BookmarkHTMLUtils just hand you a promise and you do what you think makes more sense,
in this case instead of task and try catch you may just install a failure handler in the promise then(), we should try catch internally regardless and resolve the priomise properly (if we don't it's our fault)
Basically it's up to the consumer to decide how to use the promise and whether to use a task, not the API method, imo.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

(In reply to Marco Bonardo [:mak] from comment #5)
> it's not common to use task spawn internally, other existing methods in
> BookmarkHTMLUtils just hand you a promise and you do what you think makes
> more sense,
> in this case instead of task and try catch you may just install a failure
> handler in the promise then(), we should try catch internally regardless and
> resolve the priomise properly (if we don't it's our fault)
> Basically it's up to the consumer to decide how to use the promise and
> whether to use a task, not the API method, imo.

OK, it's best to keep it as it is.  We do try catch internally and resolve / reject the promise properly.

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Raymond Lee [:raymondlee] from comment #6)
> OK, it's best to keep it as it is.  We do try catch internally and resolve /
> reject the promise properly.

What I was suggesting is that you could move the logging (Cu.reportError) into the internal "catch" block.  The code would still return a promise and would still reject it on error, but the Metro code would be free to just ignore that promise, so it would no longer need to spawn a task.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3

Do you mean something like this?

Comment 9

[Matt Brubeck (:mbrubeck)]

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

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

Yep!  Thanks. :)

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 11

[Raymond Lee [:raymondlee]]

Looks good on try:
https://tbpl.mozilla.org/?tree=Try&rev=e34c3717fe6e

Comment 12

[Matt Brubeck (:mbrubeck)]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4967844a6ae2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4967844a6ae2