853138 - Provide channel-specific provider categories

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P2)

Description

[Richard Newman [:rnewman]]

We'd like to support different sets of providers for different channels (e.g., Aurora vs Release). We already support multiple category entries, so this should be as simple as:

* Splitting our providers registrations into multiple category files
* Setting different prefs for those for each channel
* Optionally, splitting our providers into different files, so that they don't need to be loaded if they're not in use.

I'm calling this a P2 for now, but it might warrant a P1.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Not sure how we could test this.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 727459 [details] [diff] [review]
Proposed patch. v1

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

I don't like having per-channel manifest files present on every build. I think it would raise eyebrows if someone saw a -aurora filename in a beta build or a nightly manifest in a release build!

What's wrong with defining every category in one manifest and then conditionally only using the required set of categories?

Ideally, we wouldn't have any references to categories not belonging to us. But my understanding is preprocessed manifest files didn't go over well? Why not do the MOZ_UPDATE_CHANNEL in the Makefile.in to select which manifest files get installed?

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

One manifest is a much better idea.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 727853 [details] [diff] [review]
Proposed patch. v2

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

r+ without changes to Makefile.in.

I trust you to make a call on the preprocessor foo in the prefs file. Maybe get f+ from mconnor?

::: services/healthreport/Makefile.in
@@ +55,5 @@
>    $(NULL)
>  
> +EXTRA_PP_COMPONENTS := \
> +  healthreport-prefs.js \
> +  $(NULL)

You don't need this.

healthreport-prefs.js is concatenated with all.js as part of /modules/libpref/src/Makefile.in. Yes, it's obvious that FHR stuff is in /modules/libpref, I know.

::: services/healthreport/healthreport-prefs.js
@@ +26,5 @@
> +    "healthreport-js-provider-default,healthreport-js-provider-nightly"
> +#elif MOZ_UPDATE_CHANNEL == aurora
> +    "healthreport-js-provider-default,healthreport-js-provider-aurora"
> +#elif MOZ_UPDATE_CHANNEL == beta
> +    "healthreport-js-provider-default,healthreport-js-provider-beta"

I hope our code properly handles a non-defined category! Would you mind testing it before 21 hits beta. I'd hate to see us waste a week of beta data due to a silly bug.

@@ +28,5 @@
> +    "healthreport-js-provider-default,healthreport-js-provider-aurora"
> +#elif MOZ_UPDATE_CHANNEL == beta
> +    "healthreport-js-provider-default,healthreport-js-provider-beta"
> +#else
> +    "healthreport-js-provider-default"

This will also be called for non-official and local builds. Essentially release + default. Perhaps we should special case "release" and use Nightly settings for the "default"/unknown channel? This is a hard call to make!

Comment 5

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Returns searches to the default set. Also sorts the list, and improves docs for the category loader.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #4)

> You don't need this.
> 
> healthreport-prefs.js is concatenated with all.js as part of
> /modules/libpref/src/Makefile.in. Yes, it's obvious that FHR stuff is in
> /modules/libpref, I know.

Oh, yay. Non-obvious, but neat.


> I hope our code properly handles a non-defined category! Would you mind
> testing it before 21 hits beta. I'd hate to see us waste a week of beta data
> due to a silly bug.

Yeah, see v3 :)

I tested this, just didn't document it in v2. nsICategoryManager just returns an empty enumeration if the category doesn't exist.


> > +    "healthreport-js-provider-default"
> 
> This will also be called for non-official and local builds. Essentially
> release + default. Perhaps we should special case "release" and use Nightly
> settings for the "default"/unknown channel? This is a hard call to make!

Yeah, I erred on the side of "custom developer builds shouldn't leak more info than release", which is why this isn't a much shorter conditional! mconnor, do you disagree?

Comment 7

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v4

With Makefile.in change.

Comment 8

[Mike Connor [:mconnor]]

I don't think we should special-case anything except beta/release builds.

Comment 9

[Gregory Szorc [:gps]]

We earlier decided that we wanted FHR enabled in local builds to maximize the similarity between what you build locally and what is shipped to users. The theory being that this will maximize the possibility of finding bugs and other weird interactions. Custom developer builds are easily distinguishable by values in the payload, so it's trivial for Metrics to ignore them.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 727991 [details] [diff] [review]
Proposed patch. v4

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

::: services/healthreport/healthreport-prefs.js
@@ +29,5 @@
> +#elif MOZ_UPDATE_CHANNEL == beta
> +    "healthreport-js-provider-default,healthreport-js-provider-beta"
> +#else
> +    "healthreport-js-provider-default"
> +#endif

Per mconnor and my opinion, we should distinguish between release and other, treating other like Nightly.

Comment 11

[Richard Newman [:rnewman]]

Landed -- with makefile change, because without it healthreport-prefs.js does not appear in the objdir.

https://hg.mozilla.org/services/services-central/rev/477539720e76

Comment 12

[Richard Newman [:rnewman]]

Verified that prefs make it into greprefs.js, removed Makefile addition.

https://hg.mozilla.org/services/services-central/rev/907c765ec68f

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/477539720e76
https://hg.mozilla.org/mozilla-central/rev/907c765ec68f

Comment 14

[Chris Coulson]

Don't browser_aboutHome.js, browser_contextSearchTabPosition.js and browser_urlbar_search_healthreport.js in browser/base/content/test also need updating? They seem to still check for an entry using the old category name, eg:

http://hg.mozilla.org/mozilla-central/file/a7ec1554d481/browser/base/content/test/browser_urlbar_search_healthreport.js#l10

... and now skip some tests as a result:

15:09:43     INFO -  TEST-START | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbar_search_healthreport.js
15:09:43     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbar_search_healthreport.js | Firefox Health Report is not enabled.
15:09:43     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbar_search_healthreport.js | finished in 19ms

Comment 15

[Richard Newman [:rnewman]]

*sigh*

Always trips me up having services/ tests living in browser/. Will fix. Thanks for the catch!

Comment 16

[Richard Newman [:rnewman]]

Attachment: Follow-up. v1

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 734964 [details] [diff] [review]
Follow-up. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  This bug.

User impact if declined: 
  None; test-only. Tests won't actually test the searches provider.

Testing completed (on m-c, etc.): 
  Run locally; now on s-c and will merge to m-c this week.

Risk to taking this patch (and alternatives if risky): 
  None; test-only.

String or IDL/UUID changes made by this patch:
  None.

gps r+'ed via Twitter.

https://hg.mozilla.org/services/services-central/rev/f536d418c3d2

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 734964 [details] [diff] [review]
Follow-up. v1

test-only fix, a+

Comment 19

[Richard Newman [:rnewman]]

Greg, could you please uplift this for me? I'm AFK foh eva.

Comment 20

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/935ac957cfb9

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/623646b5080b for breaking browser_aboutHome.js, browser_contextSearchTabPosition.js, browser_urlbar_search_healthreport.js and browser_healthreport.js.

Comment 22

[Gregory Szorc [:gps]]

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

Comment 23

[Richard Newman [:rnewman]]

Relanded after relanding three parts of Bug 841554, which got eaten by a merge.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6fcbbe48902c