1220322 - Change migration URI from mozilla.org to support.mozilla.org in firefox.js

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Tanvi Vyas[:tanvi]]

As soon as we have https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/push working with some the Push Notification text (bug 1220250) we will need to update the urls in our codebase to point here instead of https://www.mozilla.org/en-US/firefox/push/.

For the one time notification prompt on upgrade to let the user know about notification changes, we can link directly to a part of the page using something like:
https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/push#w_[What's-New]

Code to change is here:
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#998

Comment 1

[Tanvi Vyas[:tanvi]]

Hi Joni,

I'm not sure how to make the url fragment work.  I've tried it with the mixed content page:

https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox#w_The%20icon%20is%20a%20gray%20triangle
or
https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox#w_The-icon-is-a-gray-triangle

Do spaces can translated into dashes or something else?  Can you provide an example url fragment so I can use that for Push.  We have to put it in the codebase, so I want to make sure we get it right.

Thanks!

Comment 2

[Tanvi Vyas[:tanvi]]

Attachment: Bug1220322-11-01-15.patch

Changed the url for the one-time notification on upgrade to point to the sumo article instead.

We need to get this reviewed, landed, and uplifted today in order for it to be released to dev edition on Tuesday.  Otherwise, we will need to reprompt after we get the right url.  Hence I am r? to markh who may be working before other browser peers are.  This should be a pretty quick and easy review.  If Mark doesn't get to it, I will change the r? to Jared or Matt.

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6b08ff61a5f

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Bug1220322-11-01-15B.patch

Added a url fragment to the previous version so that we point directly to the "Upgraded Notifications" section.

+pref("browser.push.warning.migration.infoURL", "https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/push#w_upgraded-notifications");

On my browser that takes me to https://support.mozilla.org/en-US/kb/push-notifications-firefox?as=u&utm_source=inproduct#w_upgraded-notifications

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8681712 [details] [diff] [review]
Bug1220322-11-01-15B.patch

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

::: browser/app/profile/firefox.js
@@ +995,5 @@
>  #endif
>  
>  pref("browser.geolocation.warning.infoURL", "https://www.mozilla.org/%LOCALE%/firefox/geolocation/");
>  pref("browser.push.warning.infoURL", "https://www.mozilla.org/%LOCALE%/firefox/push/");
> +pref("browser.push.warning.migration.infoURL", "https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/push#w_upgraded-notifications");

Nit: Why not "browser.push.warning.migrationURL"?

::: testing/profiles/prefs_general.js
@@ +115,5 @@
>  // at least once will mean that codepath is still tested in automation.
>  user_pref("network.sntp.maxRetryCount", 1);
>  
>  // Make sure the notification permission migration test doesn't hit the network.
> +user_pref("browser.push.warning.migration.infoURL", "http://%(server)s/alerts-dummy/infoURL");

Please keep the other pref here too and have their paths differ for debugging.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8681712 [details] [diff] [review]
Bug1220322-11-01-15B.patch

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

The "content" panel of preferences still uses the old URL, which I assume is intended - if that's the case, it LGTM.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

I missed Matt's review - please also make the changes he suggests.

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Bug1220322-11-01-15C.patch

Updated per Matt's suggestions.  Carrying over r+.

The web page content has also been updated so you can see it here:
https://support.mozilla.org/en-US/kb/push-notifications-firefox?as=u&utm_source=inproduct#w_upgraded-notifications

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/798c82c8a96d

Comment 9

[Tanvi Vyas[:tanvi]]

Comment on attachment 8681749 [details] [diff] [review]
Bug1220322-11-01-15C.patch

Approval Request Comment
[Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1216271
[User impact if declined]: Security issue.  With the changes to notifications in Firefox 44, Firefox gives extra Push permissions to sites that have enabled local notifications without their consent.  In order to inform the users of this change, we show a one time notification.  This patch updates the link to the right page that provides the necessary information for the user to take action.  Without this patch, the user will not be directed to the right place.
[Describe test coverage new/current, TreeHerder]: browser/base/content/test/alerts/browser_notification_permission_migration.js already exists to test the general functionality.  But since mochitests don't make real network connections, there is dummy url in place.
[Risks and why]: 
[String/UUID change made/needed]: None

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #1)
> Hi Joni,
> 
> I'm not sure how to make the url fragment work.  I've tried it with the
> mixed content page:
> 
> https://support.mozilla.org/en-US/kb/mixed-content-blocking-
> firefox#w_The%20icon%20is%20a%20gray%20triangle
> or
> https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox#w_The-
> icon-is-a-gray-triangle
> 
Got this working by using a lowercase "t" in "#w_The" above.  Clearing needinfo.

https://support.mozilla.org/en-US/kb/mixed-content-blocking-firefox#w_the-icon-is-a-gray-triangle

Comment 11

[Bill Maggs (bmaggs)]

Pointing out that the content on the web page still needs to change via https://bugzilla.mozilla.org/show_bug.cgi?id=1209992 to show permissions change via Content Preferences.

Comment 12

[Tanvi Vyas[:tanvi]]

(In reply to Bill Maggs (bmaggs) from comment #11)
> Pointing out that the content on the web page still needs to change via
> https://bugzilla.mozilla.org/show_bug.cgi?id=1209992 to show permissions
> change via Content Preferences.

The "Upgraded Notifications" section is correct and that is the most important for this bug.  We will get other parts of the page fixed up tomorrow.

Comment 13

[Tanvi Vyas[:tanvi]]

Updating subject to reflect that this bug only changes the migration URI.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8681749 [details] [diff] [review]
Bug1220322-11-01-15C.patch

Taking it as it fixes an important issue.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/798c82c8a96d

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/798c82c8a96d

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b8f03c5aa29e