Provide channel-specific provider categories

RESOLVED FIXED in Firefox 22

Status

Firefox Health Report
Client: Desktop
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 22
Points:
---

Firefox Tracking Flags

(firefox22 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 727459 [details] [diff] [review]
Proposed patch. v1

Not sure how we could test this.
Attachment #727459 - Flags: review?(gps)

Comment 2

5 years ago
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?
(Assignee)

Comment 3

5 years ago
Created attachment 727853 [details] [diff] [review]
Proposed patch. v2

One manifest is a much better idea.
Attachment #727459 - Attachment is obsolete: true
Attachment #727459 - Flags: review?(gps)
Attachment #727853 - Flags: review?(gps)

Comment 4

5 years ago
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!
Attachment #727853 - Flags: review?(gps) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 727988 [details] [diff] [review]
Proposed patch. v3

Returns searches to the default set. Also sorts the list, and improves docs for the category loader.
Attachment #727853 - Attachment is obsolete: true
Attachment #727988 - Flags: review?(gps)
(Assignee)

Comment 6

5 years ago
(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?
Flags: needinfo?(mconnor)
(Assignee)

Comment 7

5 years ago
Created attachment 727991 [details] [diff] [review]
Proposed patch. v4

With Makefile.in change.
Attachment #727988 - Attachment is obsolete: true
Attachment #727988 - Flags: review?(gps)
Attachment #727991 - Flags: review+
I don't think we should special-case anything except beta/release builds.
Flags: needinfo?(mconnor)

Comment 9

5 years ago
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 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.
(Assignee)

Comment 11

5 years ago
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
Whiteboard: [fixed in services]
(Assignee)

Comment 12

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

https://hg.mozilla.org/services/services-central/rev/907c765ec68f
https://hg.mozilla.org/mozilla-central/rev/477539720e76
https://hg.mozilla.org/mozilla-central/rev/907c765ec68f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla22

Updated

5 years ago
Blocks: 855371

Updated

5 years ago
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22

Comment 14

5 years ago
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
(Assignee)

Comment 15

5 years ago
*sigh*

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

Comment 16

5 years ago
Created attachment 734964 [details] [diff] [review]
Follow-up. v1
Attachment #734964 - Flags: review?(gps)
(Assignee)

Comment 17

5 years ago
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
Attachment #734964 - Flags: review?(gps)
Attachment #734964 - Flags: review+
Attachment #734964 - Flags: approval-mozilla-aurora?
Comment on attachment 734964 [details] [diff] [review]
Follow-up. v1

test-only fix, a+
Attachment #734964 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 19

5 years ago
Greg, could you please uplift this for me? I'm AFK foh eva.
Flags: needinfo?(gps)
https://hg.mozilla.org/releases/mozilla-aurora/rev/935ac957cfb9
status-firefox22: --- → fixed
Flags: needinfo?(gps)
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.
status-firefox22: fixed → affected
(Assignee)

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6fcbbe48902c
status-firefox22: affected → fixed
You need to log in before you can comment on or make changes to this bug.