Closed Bug 1268369 Opened 8 years ago Closed 8 years ago

Expose distributionId as part of the appinfo configuration

Categories

(Firefox :: Tours, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: hectorz, Assigned: hectorz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In bug 1264843, I want certain changes made to mozilla.org (use FxA + Sync servers hosted in China etc.) when visiting with China repack of Firefox for desktop.

I'd like to expose a "distributionId" as used in app update ping to mozilla.org pages through the UITour API getConfiguration("appinfo"), for mozilla.org to detect the usage of Fx China repack.
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49417/diff/1-2/
Attachment #8746411 - Attachment description: MozReview Request: Bug 1268369 - Expose distributionId as part of the appinfo configuration. r?Gjis → MozReview Request: Bug 1268369 - Expose distributionId as part of the appinfo configuration. r?Gijs
Attachment #8746411 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

https://reviewboard.mozilla.org/r/49417/#review46257

So... I think this is not quite right. I think we should add a different message to UITour. `appinfo` relies on Services.appinfo, which is really nsIXULAppInfo. Which, confusingly, does have a distributionId property, but it seems that that is set at build-time, and so presumably it is the wrong thing for the China repack? This isn't really clear. Either way, we shouldn't make the `appinfo` message include non-appinfo information, especially not information that's named as if it does come from appinfo, when it really doesn't. So either we should reuse `Services.appinfo.distributionID`**** or we should add a separate message. But not this mixture of the two. :-)

What I also don't understand is why mozilla.org's UITour needs to know this particularly. Doesn't the repack control all the URLs we use, and so it could just produce alternative URLs instead of providing this information via UITour ?

**** Nit: Note also capital 'ID' :-)

::: browser/components/uitour/test/browser_UITour.js:299
(Diff revision 2)
> +      ok(typeof(result[property]) !== "undefined", "Check " + property + " isn't undefined.");
> +      is(result[property], "default", "Should be \"default\" without preference set.");

It's clear that this is copied from the earlier test, but please just hardcode the value of `property` throughout this function. Right now, reading the code you'd think it changes, but it doesn't.
Attachment #8746411 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #3)
Thanks for the quick feedback!

> 
> So... I think this is not quite right. I think we should add a different
> message to UITour. `appinfo` relies on Services.appinfo, which is really
> nsIXULAppInfo.

I thought it's okay since default browser related entries are added to `appinfo` later.

> Which, confusingly, does have a distributionId property, but
> it seems that that is set at build-time, and so presumably it is the wrong
> thing for the China repack?

It's the wrong thing, what I want is value of preference `distribution.id`, set from distribution.ini on desktop. This value is also used in these pings[1] to identify the partner repack.

In fact, it looks like `MOZ_DISTRIBUTION_ID` is only referenced in nsSearchService.js and not really being used since [2].

> This isn't really clear. Either way, we
> shouldn't make the `appinfo` message include non-appinfo information,
> especially not information that's named as if it does come from appinfo,
> when it really doesn't. So either we should reuse
> `Services.appinfo.distributionID`**** or we should add a separate message.
> But not this mixture of the two. :-)

I presumed `defaultBrowser` was added into `appinfo` to reduce the number of API calls and didn't think much about this potential confusion.

Do you have suggestions here? Maybe I can name it `distribution`, like in Fx update ping?

> 
> What I also don't understand is why mozilla.org's UITour needs to know this
> particularly. Doesn't the repack control all the URLs we use, and so it
> could just produce alternative URLs instead of providing this information
> via UITour ?

There're concerns about people copy-n-pasting these alternative URLs and share it[3].

Also I hope an user could navigate from one in-product page to other pages on mozilla.org and still sees the proper content. Without UITour's help, such alterations must be passed around in some way.

[1]: about:config?filter=DISTRIBUTION
[2]: https://hg.mozilla.org/mozilla-central/diff/99a61ec93c5a/browser/locales/en-US/searchplugins/google.xml
[3]: https://github.com/mozilla/bedrock/pull/4066#issuecomment-214429789
(In reply to Hector Zhao [:hectorz] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> Thanks for the quick feedback!
> 
> > 
> > So... I think this is not quite right. I think we should add a different
> > message to UITour. `appinfo` relies on Services.appinfo, which is really
> > nsIXULAppInfo.
> 
> I thought it's okay since default browser related entries are added to
> `appinfo` later.

This is fair. Sorry for not noticing this earlier.

> > This isn't really clear. Either way, we
> > shouldn't make the `appinfo` message include non-appinfo information,
> > especially not information that's named as if it does come from appinfo,
> > when it really doesn't. So either we should reuse
> > `Services.appinfo.distributionID`**** or we should add a separate message.
> > But not this mixture of the two. :-)
> 
> I presumed `defaultBrowser` was added into `appinfo` to reduce the number of
> API calls and didn't think much about this potential confusion.

OK. defaultBrowser is indeed also added here - but defaultBrowser has no confusion associated with it as it does not exist on nsIXULAppInfo.

> Do you have suggestions here? Maybe I can name it `distribution`, like in Fx
> update ping?

You could do this, I guess... it would also be a good idea to have a comment in the code noting that it *isn't* the same as the nsIXULAppInfo's distributionID property, and this relates strictly to values set using distribution.ini / repacks.

If nobody uses MOZ_DISTRIBUTION_ID and the nsIXULAppInfo value for it anymore, maybe we should just remove it. I'll ping folks about this.

> > What I also don't understand is why mozilla.org's UITour needs to know this
> > particularly. Doesn't the repack control all the URLs we use, and so it
> > could just produce alternative URLs instead of providing this information
> > via UITour ?
> 
> There're concerns about people copy-n-pasting these alternative URLs and
> share it[3].

Hm, OK.

> Also I hope an user could navigate from one in-product page to other pages
> on mozilla.org and still sees the proper content. Without UITour's help,
> such alterations must be passed around in some way.

Cookies? Anyway, that doesn't help with the sharing of the alternative URLs, I guess, so let's go ahead and update the patch based on feedback so far, and I'll review again then.
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49417/diff/2-3/
Attachment #8746411 - Attachment description: MozReview Request: Bug 1268369 - Expose distributionId as part of the appinfo configuration. r?Gijs → MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r?Gijs
Attachment #8746411 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

https://reviewboard.mozilla.org/r/49417/#review46967

::: browser/components/uitour/test/browser_UITour.js:297
(Diff revision 3)
> +      ok(typeof(result["distribution"]) !== "undefined", "Check distribution isn't undefined.");
> +      is(result["distribution"], "default", "Should be \"default\" without preference set.");

Nit: please use result.distribution instead of the square bracket notation.

::: browser/components/uitour/test/browser_UITour.js:304
(Diff revision 3)
> +        ok(typeof(result["distribution"]) !== "undefined", "Check distribution isn't undefined.");
> +        is(result["distribution"], testDistributionID, "Should have the distribution as set in preference.");

Same here.
Attachment #8746411 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49417/diff/3-4/
Attachment #8746411 - Attachment description: MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r?Gijs → MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/d1bb2c95ee05
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

Approval Request Comment
[Feature/regressing bug #]: Not a regression, more like a feature request from bug 1264843.
[User impact if declined]: Serving outdated, undermaintained fork of in-product pages to Fx China repack users for more release cycles.
[Describe test coverage new/current, TreeHerder]: Landed in central for two weeks. also I confirmed the exposed distribution information is correct during both firstrun and normal browsing.
[Risks and why]: Low risk, new information exposed to mozilla.org through UITour.
[String/UUID change made/needed]: None.
Attachment #8746411 - Flags: approval-mozilla-beta?
Attachment #8746411 - Flags: approval-mozilla-aurora?
Comment on attachment 8746411 [details]
MozReview Request: Bug 1268369 - Expose distribution.id as part of the appinfo configuration. r=Gijs

Let's uplift this to Aurora48. We do not take new feature requests, even mini ones in Beta cycle. 

At this point, I would like to only accept uplifts for critical recent regressions, severe stability, security, perf, mlk issues only. This is to minimize code churn and to ensure we ship a high-quality Fx47 in 2 weeks.
Attachment #8746411 - Flags: approval-mozilla-beta?
Attachment #8746411 - Flags: approval-mozilla-beta-
Attachment #8746411 - Flags: approval-mozilla-aurora?
Attachment #8746411 - Flags: approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #13)
> 
> Let's uplift this to Aurora48. We do not take new feature requests, even
> mini ones in Beta cycle.

Thank you. I'll keep this in mind in the future.
Will this bug satisfy a very similar bug 1246791 I filed 4 months ago?
(In reply to Chris More [:cmore] from comment #17)
> Will this bug satisfy a very similar bug 1246791 I filed 4 months ago?

I mean, it's been implemented, so try it? :-)

I don't think it gives you update channel... but worth a check, if not needinfo me on the other bug and I can look next week, if there's a specific reason you need it.
(In reply to :Gijs Kruitbosch (no reviews until June 6) from comment #18)
> I don't think it gives you update channel... but worth a check, if not
> needinfo me on the other bug and I can look next week, if there's a specific
> reason you need it.

defaultUpdateChannel was already there. This what I get in a Mozilla Nightly:

Object { defaultUpdateChannel: "nightly", version: "49.0a1", distribution: "default", defaultBrowser: true, canSetDefaultBrowserInBackground: false }
Blocks: 1246791
You need to log in before you can comment on or make changes to this bug.