846636 - Use asynchronous getCharsetForURI in getShortcutOrURI for metro

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Sync, defect)

Description

[Raymond Lee [:raymondlee]]

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.js#377

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: WIP

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

Comment 2

[Jim Mathies [:jimm]]

general->sync 10 bugs

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: WIP v2

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

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v3

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

Comment 5

[Matt Brubeck (:mbrubeck)]

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));

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v4

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

[Matt Brubeck (:mbrubeck)]

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.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

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

[Matt Brubeck (:mbrubeck)]

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

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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