Use asynchronous getCharsetForURI in getShortcutOrURI for metro

RESOLVED FIXED in Firefox 22

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

unspecified
Firefox 22
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Updated

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

Comment 1

6 years ago
Posted patch WIP (obsolete) — Splinter Review
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
Component: General → Sync
No longer blocks: 834543
Depends on: 834543
Assignee

Comment 3

6 years ago
Posted patch WIP v2 (obsolete) — Splinter Review
The patch is ready but need to test it on metro.
Attachment #721122 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #724334 - Attachment is patch: true
Assignee

Comment 4

6 years ago
Posted patch v3 (obsolete) — Splinter Review
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

6 years ago
Posted patch v4 (obsolete) — Splinter Review
(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

6 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

6 years ago
(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

6 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Assignee

Updated

6 years ago
Attachment #729631 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c8509229f205
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
You need to log in before you can comment on or make changes to this bug.