Closed Bug 1037170 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: FxA, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: spenrose, Assigned: spenrose)

References

Details

Attachments

(1 file, 1 obsolete file)

Intercept "DIALOG_CLOSED_BY_USER" and redirect to the cancel path.
Blocks: 1037171
Attached 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: 9 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.