See bug 822200 comment 7

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

Maybe matt may help here

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.

(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?

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.

(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.

(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.

Do you mean something like this?

Comment on attachment 727823 [details] [diff] [review] v3 Review of attachment 727823 [details] [diff] [review]: ----------------------------------------------------------------- Yep! Thanks. :)

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

Comment on attachment 728053 [details] [diff] [review] Patch for check-in https://hg.mozilla.org/integration/mozilla-inbound/rev/4967844a6ae2