Fire oncancel() when user cancels Sign Up/Sign In

RESOLVED FIXED in Firefox 32

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: spenrose, Assigned: spenrose)

Tracking

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Intercept "DIALOG_CLOSED_BY_USER" and redirect to the cancel path.
Blocks: 1037171
Posted patch 1037170-signin-oncancel.patch (obsolete) — Splinter Review
Hi Jed -- another small one. My bad for not verifying all the flows and noting that the error object format changes depending on the dialog cancelled.
Attachment #8454051 - Flags: review?(jed+bmo)
https://tbpl.mozilla.org/?tree=Try&rev=b8fed8d5937d
blocking-b2g: --- → 2.0?
Comment on attachment 8454051 [details] [diff] [review]
1037170-signin-oncancel.patch

Review of attachment 8454051 [details] [diff] [review]:
-----------------------------------------------------------------

Grody that our flows send error or error.error.  That should be fixed, or maybe filed as a Good First Bug for someone to enjoy.

From a quick scan through fxaccounts and toolkit/identity, this seems like the only place where we look into the error object, so this is fine as a no-risk, short-term fix with me.

Also ok if you want to change Credentials.jsm logger, as mentioned below.

r=me

Thanks, Sam!

::: toolkit/identity/FirefoxAccounts.jsm
@@ +12,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/identity/LogUtils.jsm");
>  
> +

Unimportant, but newline accidentally introduced.

@@ +31,5 @@
>  } catch (e) {
>    this.LOG_LEVEL = Log.Level.Error;
>  }
>  
> +let log = Log.repository.getLogger("FirefoxAccounts");

If you want to change this from Identity.FxAccounts to FirefoxAccounts, you should also change the logger in Credentials.jsm.  If you want to add the change to Credentials.jsm to this patch, I'm fine with that.

@@ +190,4 @@
>          // Cancellation is passed through an error channel; here we reroute.
> +        if ((error.error && (error.error.details == "DIALOG_CLOSED_BY_USER"))
> +            || (error.details == "DIALOG_CLOSED_BY_USER")
> +        ) {

Microscopic nit: The placement of the braces and the || here is not quite in tune with Moz style.  Style guide likes line breaks after || or &&, and the close paren and curly brace should be on the same line (above).
Comment on attachment 8454051 [details] [diff] [review]
1037170-signin-oncancel.patch

Whoops - forgot to change the flag
Attachment #8454051 - Flags: review?(jed+bmo) → review+
Sloppy mistakes clean up. Thanks very much Jed.
Attachment #8454051 - Attachment is obsolete: true
Keywords: checkin-needed
Error cleanup filed as bug 1037268
https://hg.mozilla.org/mozilla-central/rev/d86018462663
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
blocking-b2g: 2.0? → 2.0+
You need to log in before you can comment on or make changes to this bug.