Add configuration to UITour.jsm for determining the user's Firefox build information

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: adw, Assigned: alexbardas)

Tracking

Trunk
Firefox 35
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxgrowth])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
Broken down from bug 1062345.

The mozilla.org page that offers to reset Firefox needs to be able to determine the user's Firefox version.  We should add an action to UITour that allows that.
Flags: firefox-backlog+
Flags: qe-verify?
Reporter

Updated

5 years ago
Flags: qe-verify? → qe-verify-
Assignee

Updated

5 years ago
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Reporter

Comment 1

5 years ago
Oh, I think instead of adding a new UITour action, Gavin was saying in bug 1062345 comment 7 (re: the "is sync enabled?" thing) that we should add a configuration recognized by getConfiguration, similar to the "sync" one there that has sends a boolean "setup": http://mxr.mozilla.org/mozilla-central/source/browser/modules/UITour.jsm?rev=08ba579779b1#1127
(In reply to Drew Willcoxon :adw from comment #1)
> Oh, I think instead of adding a new UITour action, Gavin was saying in bug
> 1062345 comment 7 (re: the "is sync enabled?" thing) that we should add a
> configuration recognized by getConfiguration, similar to the "sync" one
> there that has sends a boolean "setup":

I agree with this FWIW.
Reporter

Updated

5 years ago
Summary: Add action to UITour.jsm for determining the user's Firefox version → Add configuration to UITour.jsm for determining the user's Firefox version
Assignee

Comment 3

5 years ago
Attachment #8491158 - Flags: review?(MattN+bmo)
Comment on attachment 8491158 [details] [diff] [review]
Determine user's Firefox version from UITour

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

Looking good but I'll take another look.

::: browser/modules/UITour.jsm
@@ +24,5 @@
>    "resource:///modules/BrowserUITelemetry.jsm");
> +XPCOMUtils.defineLazyGetter(this, "appInfo", () => {
> +  return Cc["@mozilla.org/xre/app-info;1"]
> +           .getService(Ci.nsIXULAppInfo)
> +           .QueryInterface(Ci.nsIXULRuntime);

I think you can just use Services.appinfo or is this doing something smarter?

@@ +1139,5 @@
>            setup: Services.prefs.prefHasUserValue("services.sync.username"),
>          });
>          break;
> +      case "version":
> +        this.sendPageCallback(aContentDocument, aCallbackID, {

Before seeing this patch I was writing:

Perhaps "appinfo" could be the configuration and it would include various bits of info from Services.appinfo as needed.

@@ +1140,5 @@
>          });
>          break;
> +      case "version":
> +        this.sendPageCallback(aContentDocument, aCallbackID, {
> +          version: appInfo.version

I think it would be useful to include some other properties such as ID (to be sure it's Firefox), defaultUpdateChannel, distributionID, isOfficialBranding, isReleaseBuild in case we want to prevent offering a reset for unofficial, non-release, and/or partner builds.

e.g. Suppose I'm running a partner build with a default search engine other than Google. If I go to mozilla.org to download vanilla Firefox, I should be able to easily download it instead of being redirect to do a reset which may leave me with partner customizations that I don't want.

::: browser/modules/test/browser_UITour.js
@@ +9,5 @@
>  
>  Components.utils.import("resource:///modules/UITour.jsm");
> +let appInfo = Components.classes["@mozilla.org/xre/app-info;1"]
> +                        .getService(Ci.nsIXULAppInfo)
> +                        .QueryInterface(Ci.nsIXULRuntime);

Use Services.appinfo inline
Attachment #8491158 - Flags: review?(MattN+bmo) → feedback+
Might as well include vendor and name as well to make sure we can distinguish third-party builds like Frontmotion Firefox which may use the same ID for add-on compat (I'm not sure).
Assignee

Comment 6

5 years ago
Attachment #8491158 - Attachment is obsolete: true
Attachment #8491208 - Flags: review?(MattN+bmo)
Iteration: --- → 35.2
Comment on attachment 8491208 [details] [diff] [review]
Determine user's Firefox version from UITour

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

Thanks!

::: browser/modules/test/browser_UITour.js
@@ +269,5 @@
> +    function callback(result) {
> +      let props = ["defaultUpdateChannel", "distributionID", "isOfficialBranding",
> +                   "isReleaseBuild", "name", "vendor", "version"];
> +      for (let property of props) {
> +        is(result[property], Services.appinfo[property], "Should have the same " + property + " property.");

It would probably be a good idea to also make sure these aren't undefined to make sure we're not comparing undefined to undefined:

ok(typeof(result[property]) !== undefined, "Check " + property + " isn't undefined.");

^ untested code
Attachment #8491208 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 8

5 years ago
Good idea to check for that undefined :-)
Attachment #8491208 - Attachment is obsolete: true
Attachment #8491269 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/60881e5c48b3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
>diff --git a/browser/modules/UITour.jsm b/browser/modules/UITour.jsm

>+      case "appinfo":
>+        let props = ["defaultUpdateChannel", "distributionID", "isOfficialBranding",
>+                     "isReleaseBuild", "name", "vendor", "version"];

This is much broader than the "version" that's in this bug's summary. Do we really need to read all of this for this use case?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #12)
> This is much broader than the "version" that's in this bug's summary. Do we
> really need to read all of this for this use case?

See comment 4 and comment 5. The bug summary and commit message should have been changed. I just did a quick pass through Services.appinfo to see which properties would be relevant for the usage in bug 1027318.

Looks like Alex forgot to include "ID".
Summary: Add configuration to UITour.jsm for determining the user's Firefox version → Add configuration to UITour.jsm for determining the user's Firefox build information
(In reply to Matthew N. [:MattN] from comment #4)
> I think it would be useful to include some other properties such as ID (to
> be sure it's Firefox), defaultUpdateChannel, distributionID,
> isOfficialBranding, isReleaseBuild in case we want to prevent offering a
> reset for unofficial, non-release, and/or partner builds.

The UITour mechanism only works with Firefox, so the ID seems unnecessary. Similarly, I don't see any need to filter by any of the other properties, so I don't think we should send them. If that changes later, we can revisit.

> e.g. Suppose I'm running a partner build with a default search engine other
> than Google. If I go to mozilla.org to download vanilla Firefox, I should be
> able to easily download it instead of being redirect to do a reset which may
> leave me with partner customizations that I don't want.

There should be a way to "download anyways" regardless, for users who want that. I don't think we need complicated filtering here.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #14)
> (In reply to Matthew N. [:MattN] from comment #4)
> > I think it would be useful to include some other properties such as ID (to
> > be sure it's Firefox), defaultUpdateChannel, distributionID,
> > isOfficialBranding, isReleaseBuild in case we want to prevent offering a
> > reset for unofficial, non-release, and/or partner builds.
> 
> The UITour mechanism only works with Firefox, so the ID seems unnecessary.
> Similarly, I don't see any need to filter by any of the other properties, so
> I don't think we should send them. If that changes later, we can revisit.

It can work in any build of the browser app which isn't necessarily Firefox. We're trying to move faster and put more trust in mozilla.org recently so adding this to prevent bottlenecks later seemed worthwhile (c.f. your recent questions about UITour whitelisting and bug 1068401). I don't see how this hurts and why we would spend time removing the additional values.

> > e.g. Suppose I'm running a partner build with a default search engine other
> > than Google. If I go to mozilla.org to download vanilla Firefox, I should be
> > able to easily download it instead of being redirect to do a reset which may
> > leave me with partner customizations that I don't want.
> 
> There should be a way to "download anyways" regardless, for users who want
> that. I don't think we need complicated filtering here.

At one point there was talk about not providing a download option. I'll reply to the rest in bug 1070138.
Whiteboard: [fxgrowth]
Duplicate of this bug: 1096514
Hi, is it possible to uplift this (and Bug 1070138) to Firefox 34 Beta assuming the risk is low? As you might know, www.mozilla.org and SUMO want to utilize the new API for version check. The current implementation on www.mozilla.org has confused visitors so I'd like to fix it soon.
Flags: needinfo?(gavin.sharp)
Um sorry, it might be too late. The calendar says Beta->Release migration happens tomorrow.
Flags: needinfo?(gavin.sharp)
Luckily there will be 2 additional betas this week, so I'd like to nominate this for 34.

[Tracking Requested - why for this release]: The current version checking on www.mozilla.org is inaccurate, confusing visitors with a wrong message, "You’re using the latest version of Firefox." We'd like to offer a better user experience on the site using this new API that provides proper channel/version info. No need to wait for 35. The risk of the patch should be low. Note that a follow-up patch in Bug 1070138 has to be landed at the same time.
This bug doesn't warrant tracking. If you want to uplift, you'll need Beta approval requests on both bugs. The changes in these bugs are really small but I'll note that the extra betas were added to the 34 cycle to stabilize and not to take additional feature work. Although we want this change earlier, I do think that we can wait for this until Firefox 35.
Is there a list somewhere of all possible values for defaultUpdateChannel? If not, could one be provided here to add to the docs (http://bedrock.readthedocs.org/en/latest/uitour.html#getconfiguration-type-callback)?
Flags: needinfo?(MattN+bmo)
(In reply to Jon Petto [:jpetto] from comment #22)
> Is there a list somewhere of all possible values for defaultUpdateChannel?
> If not, could one be provided here to add to the docs
> (http://bedrock.readthedocs.org/en/latest/uitour.html#getconfiguration-type-
> callback)?

There are hundreds of values but the important ones in decreasing order of population are:
release, beta, aurora, nightly, default

The above should be straightforward except for "default" which refers to self-built or automated testing builds. The are hundreds of variations starting with "release-" for partner builds e.g. the Yahoo! browser bundle.

For bug 1027318 I think an exact match for "release" is what you want to test against.
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.