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

Status: RESOLVED FIXED

Description

[Sam Penrose]

Intercept "DIALOG_CLOSED_BY_USER" and redirect to the cancel path.

Comment 1

[Sam Penrose]

Attachment: 1037170-signin-oncancel.patch

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.

Comment 2

[Sam Penrose]

https://tbpl.mozilla.org/?tree=Try&rev=b8fed8d5937d

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8454051 [details] [diff] [review]
1037170-signin-oncancel.patch

Whoops - forgot to change the flag

Comment 5

[Sam Penrose]

Attachment: 1037170-signin-oncancel.patch

Sloppy mistakes clean up. Thanks very much Jed.

Comment 6

[Sam Penrose]

Error cleanup filed as bug 1037268

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d86018462663

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e100e6b0efc1