If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use asynchronous getCharsetForURI in getShortcutOrURI for metro

RESOLVED FIXED in Firefox 22

Status

Firefox for Metro
Sync
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 22
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.js#377
(Assignee)

Updated

5 years ago
Summary: Bug 846635 - Use asynchronous getCharsetForURI in getShortcutOrURI for metro → Use asynchronous getCharsetForURI in getShortcutOrURI for metro
(Assignee)

Comment 1

5 years ago
Created attachment 721122 [details] [diff] [review]
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

5 years ago
general->sync 10 bugs
Component: General → Sync

Updated

5 years ago
No longer blocks: 834543
Depends on: 834543

Updated

5 years ago
Blocks: 699850
(Assignee)

Comment 3

5 years ago
Created attachment 724334 [details] [diff] [review]
WIP v2

The patch is ready but need to test it on metro.
Attachment #721122 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #724334 - Attachment is patch: true
(Assignee)

Comment 4

5 years ago
Created attachment 729579 [details] [diff] [review]
v3

Matt: could you kindly help to test this patch and review it please?
Attachment #724334 - Attachment is obsolete: true
Attachment #729579 - Flags: review?(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));
Attachment #729579 - Flags: review?(mbrubeck) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 729631 [details] [diff] [review]
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.
Attachment #729631 - Flags: review?(mbrubeck)
(Assignee)

Updated

5 years ago
Attachment #729579 - Attachment is obsolete: true
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.
Attachment #729631 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 729722 [details] [diff] [review]
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!
Attachment #729722 - Flags: checkin?(mbrubeck)
(Assignee)

Updated

5 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #729631 - Attachment is obsolete: true
Comment on attachment 729722 [details] [diff] [review]
Patch for check-in

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8509229f205
Attachment #729722 - Flags: review+
Attachment #729722 - Flags: checkin?(mbrubeck)
Attachment #729722 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c8509229f205
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.