Closed Bug 1204714 Opened 4 years ago Closed 4 years ago

Have the "activate your FxAccount" tab switch to about:preferences#sync if opened in the browser configured with the account

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox43 --- affected
firefox46 --- fixed

People

(Reporter: edwong, Assigned: stomlinson)

References

()

Details

(Whiteboard: fxsync)

Attachments

(1 file, 3 obsolete files)

6b. If opened in same browser, new tab becomes Sync prefs, Confirm Your Account becomes Account Verified
related to bug 1199303. Needs a content server bug to send webChan message to enable 6b.
Iteration: --- → 43.3 - Sep 21
Whiteboard: fxsync
ni? :markh to link me to the content-server if we have one, or provide a description of what's needed if we don't; I'm not confident I remember all the details from when we discussed this in IRC.
Flags: needinfo?(markh)
(In reply to Ryan Kelly [:rfkelly] from comment #2)
> ni? :markh to link me to the content-server if we have one, or provide a
> description of what's needed if we don't; I'm not confident I remember all
> the details from when we discussed this in IRC.

We don't have a content-server issue yet. In a nutshell, we want the landing page for "please verify your mail" to send us a message as it transitions to the "much success" page. The browser would be listening for this message and switch the tab to about:preferences. Note also that this message probably shouldn't be a generic "user is/became verified" message as the semantics of that might be unclear - I guess it is closer to "user just became verified via this exact page".
Flags: needinfo?(markh)
Flags: firefox-backlog+
This isn't happening in 43.3. This is perhaps a candidate for 44.1.
Iteration: 43.3 - Sep 21 → ---
Priority: -- → P1
Summary: smarter logic about verification link handling → Have the "activate your FxAccount" tab switch to about:preferences#sync if opened in the browser configured with the account
Assignee: nobody → stomlinson
Comment on attachment 8700596 [details] [diff] [review]
bug-1204714-open-sync-preferences.patch

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

I believe this will open preferences in every open window. I think we should get the "browser" which sent the message and have only that window open prefs. I think this means an ugly reach into the "sendingContext", but we can probably get away with that. Testing will be a bit of a challenge, but it might make sense to create a new browser test purely for webchannels (ie, along the lines of browser_aboutaccounts, but focusing on browser-side channel handling)
Attachment #8700596 - Flags: feedback?(markh) → feedback-
Attachment #8700596 - Attachment is obsolete: true
Thanks for the feedback Mark. I have been digging in today and found another approach that may be preferable, and at first blush appears to "work" though I don't have enough system context to know for sure.

It starts with the assumption that the currently displayed tab sent the WebChannel message. This assumption may be ill founded, but it vastly simplifies things. In utilityOverlay.js, there is a function called "openUILinkIn" [1] that takes a "where" configuration option that allows a URI to be opened in one of several locations, "current" means "replace the current tab". If openPreferences [2] is updated so its extraArgs parameter also accepts a "where" option, if "where" exists, I can call openUILinkIn with "where", if "where" does not exist, use the existing behavior. Workable? I've updated the patch with this approach.

[1] - https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/base/content/utilityOverlay.js#177
[2] - https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/browser/base/content/utilityOverlay.js#526
Flags: needinfo?(markh)
Comment on attachment 8704630 [details] [diff] [review]
bug-1204714-open-sync-preferences.patch

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

ISTM that this will still make every open window switch to preferences - browser-syncui is loaded into each window and the notification will be seen by each of them.  I expect that something like:

sendingContext.browser.loadURI(...) is all you need.

::: browser/base/content/utilityOverlay.js
@@ +736,5 @@
>    openUILinkIn(url, where);
>  }
>  
>  function openPrefsHelp() {
> +  // non-instant apply prefwindows are usually modal, so we can't open in the topmost window,

let's not touch unrelated lines.
Flags: needinfo?(markh)
Mark, is this more along the lines of what you were thinking? The only thing that worries me about this approach is if preferences are loaded via a different mechanism in the future. The openPreferences function in utilityOverlay.js provides an abstraction to open preferences that should guard against future changes, it just doesn't provide the functionality (replace the current tab) we need.

The other thing this patch does not yet do is provide a mechanism for notifying the content server this feature is enabled. We could either bump the context from fx_desktop_v2 to fx_desktop_v3, or we could add a query parameter. I really feel like we need the capabilities mechanism we keep bringing up!

Treeherder run https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a6057568cb7
Attachment #8704630 - Attachment is obsolete: true
Attachment #8705624 - Flags: review?(markh)
Comment on attachment 8705624 [details] [diff] [review]
Open sync preferences in the current tab of the WebChannel message's sendingContext

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

I think this is fine - about:preferences can be found here via dxr if a future means we need to revisit this.

Re letting the server know - I'm fine with either a _v3 or a query param - but it's not clear to me why the server cares - at the moment that tab just stays on the verification page, and this will continue if the client doesn't understand the message - but I'm probably missing something obvious as I haven't thought much about it.
Attachment #8705624 - Flags: review?(markh) → review+
(In reply to Mark Hammond [:markh] from comment #13)
> Comment on attachment 8705624 [details] [diff] [review]
> Open sync preferences in the current tab of the WebChannel message's
> sendingContext
> 
> Review of attachment 8705624 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Re letting the server know - I'm fine with either a _v3 or a query param -
> but it's not clear to me why the server cares - at the moment that tab just
> stays on the verification page, and this will continue if the client doesn't
> understand the message - but I'm probably missing something obvious as I
> haven't thought much about it.


Ah, the big thing is we (plan to) show the user the "Your account has been verified!" screen with a big button that says "Sync preferences". We don't want to show the big blue button unless the browser can handle the message, correctly handling legacy browsers. Once the user clicks the button, preferences are loaded. This is the same scheme currently used by Fennec; the user is informed their account has been verified, plus they have an opportunity to continue to preferences if they wish.
This most recent patch changes the context from fx_desktop_v2 to fx_desktop_v3. This patch should land after the content server patch makes its way to prod or else Nightly users will be unable to sign in/up to Sync.

[1] https://github.com/mozilla/fxa-content-server/pull/3341
Attachment #8705624 - Attachment is obsolete: true
Attachment #8706583 - Flags: review?(markh)
Attachment #8706583 - Flags: review?(markh) → review+
Attachment #8706583 - Flags: checkin?
Attachment #8706583 - Flags: checkin?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f1573412a4ad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful.

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
When already logged into Firefox account, new tab open with about:preferences#sync account becomes
logged into account.

Actual Results: 
As expected
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.