Closed Bug 852040 Opened 8 years ago Closed 8 years ago

Apply use of BookmarkJSONUtils.importFromURL

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: andreshm, Assigned: raymondlee)

References

Details

Attachments

(1 file, 3 obsolete files)

Blocks: 852030
Depends on: 822200
Assignee: nobody → raymond
Attached patch v1 (obsolete) — Splinter Review
marco: do you know someone who can test this patch?  I am on mac and not able to test it due to bug 847807
Flags: needinfo?(mak77)
Maybe matt may help here
Flags: needinfo?(mak77) → needinfo?(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.
Attachment #727053 - Flags: review+
Flags: needinfo?(mbrubeck)
(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?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
Attached patch Patch for check-in (obsolete) — Splinter Review
(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.
Attachment #727053 - Attachment is obsolete: true
Keywords: checkin-needed
(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.
Attached patch v3 (obsolete) — Splinter Review
Do you mean something like this?
Attachment #727722 - Attachment is obsolete: true
Attachment #727823 - Flags: review?(mbrubeck)
Keywords: checkin-needed
Comment on attachment 727823 [details] [diff] [review]
v3

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

Yep!  Thanks. :)
Attachment #727823 - Flags: review?(mbrubeck) → review+
Attachment #727823 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
Blocks: 854288
Blocks: 854761
Attachment #728053 - Flags: checkin?(mbrubeck)
https://hg.mozilla.org/mozilla-central/rev/4967844a6ae2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.