This is a work in progress patch because I can't test it until I get my metro build working bug 847807

general->sync 10 bugs

The patch is ready but need to test it on metro.

Matt: could you kindly help to test this patch and review it please?

Comment on attachment 729579 [details] [diff] [review] v3 Review of attachment 729579 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! r=mbrubeck with some trivial changes (below). Setting f+ for now because I'd like to review/test the final version before it's checked in. ::: browser/metro/base/content/browser.js @@ +10,5 @@ > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > + "resource://gre/modules/Task.jsm"); You should add this to browser-scripts.js instead. Note that bug 808770 is also adding this there; if it lands first then you can just remove this. @@ +155,5 @@ > + let activationURI; > + Task.spawn(function() { > + activationURI = yield Browser.getShortcutOrURI(MetroUtils.activationURI); > + }).then(function() { > + // Should we restore the previous session (crash or some other event) Just curious -- why do you use a then() callback here instead of just continuing within the Task.spawn calback? If possible, could you move this all into the Task.spawn block? Then you can also move "function loadStartupURI" and "let activationURI" inside that block. @@ +405,5 @@ > + // Try to get the saved character-set. > + try { > + // makeURI throws if URI is invalid. > + // Will return an empty string if character-set is not found. > + charset = PlacesUtils.history.getCharsetForURI(Util.makeURI(shortcutURL)); I think you mean: charset = yield PlacesUtils.getCharsetForURI(Util.makeURI(shortcutURL));

(In reply to Matt Brubeck (:mbrubeck) from comment #5) > Comment on attachment 729579 [details] [diff] [review] > v3 > > Review of attachment 729579 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the patch! r=mbrubeck with some trivial changes (below). > > Setting f+ for now because I'd like to review/test the final version before > it's checked in. > > ::: browser/metro/base/content/browser.js > @@ +10,5 @@ > > > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > > + > > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > > + "resource://gre/modules/Task.jsm"); > > You should add this to browser-scripts.js instead. Note that bug 808770 is > also adding this there; if it lands first then you can just remove this. > OK, moved to browser-scripts.js > @@ +155,5 @@ > > + let activationURI; > > + Task.spawn(function() { > > + activationURI = yield Browser.getShortcutOrURI(MetroUtils.activationURI); > > + }).then(function() { > > + // Should we restore the previous session (crash or some other event) > > Just curious -- why do you use a then() callback here instead of just > continuing within the Task.spawn calback? > > If possible, could you move this all into the Task.spawn block? Then you > can also move "function loadStartupURI" and "let activationURI" inside that > block. Done. > > @@ +405,5 @@ > > + // Try to get the saved character-set. > > + try { > > + // makeURI throws if URI is invalid. > > + // Will return an empty string if character-set is not found. > > + charset = PlacesUtils.history.getCharsetForURI(Util.makeURI(shortcutURL)); > > I think you mean: > > charset = yield PlacesUtils.getCharsetForURI(Util.makeURI(shortcutURL)); Thanks for spotting that.

Comment on attachment 729631 [details] [diff] [review] v4 Review of attachment 729631 [details] [diff] [review]: ----------------------------------------------------------------- Great! ::: browser/metro/base/content/browser.js @@ +196,4 @@ > > + // Let everyone know what kind of mouse input we are > + // starting with: > + InputSourceHelper.fireUpdate(); Since the messageManager and InputSourcHelper lines don't depend on the async stuff in this task, I'd like to move them up to just above the Task.spawn call. If you'd like, I can land this patch for you with that minor change.

(In reply to Matt Brubeck (:mbrubeck) from comment #7) > Comment on attachment 729631 [details] [diff] [review] > v4 > > Review of attachment 729631 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great! > > ::: browser/metro/base/content/browser.js > @@ +196,4 @@ > > > > + // Let everyone know what kind of mouse input we are > > + // starting with: > > + InputSourceHelper.fireUpdate(); > > Since the messageManager and InputSourcHelper lines don't depend on the > async stuff in this task, I'd like to move them up to just above the > Task.spawn call. > Moved > If you'd like, I can land this patch for you with that minor change. It would be great!

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