1444276 - Refactor the telemetry pref setting code

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

Refactoring this code will enable bug 1444275.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1444276 - Refactor the telemetry pref setting code.

This patch doesn't change the functionality, it just splits out the code into
separate functions that are easier to read.

Review commit: https://reviewboard.mozilla.org/r/226370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226370/

Comment 2

[Chris H-C :chutten]

Comment on attachment 8957446 [details]
Bug 1444276 - Refactor the telemetry pref setting code.

https://reviewboard.mozilla.org/r/226370/#review232384

The tortuous logic appears to have been reproduced correctly here. This should undergo manual QA with artificial channel builds, so please do request PI take a look into this.

We're actually just about to undergo a PI run for bug 1435753 that you might be able to piggyback on

::: modules/libpref/Preferences.cpp:2932
(Diff revision 1)
> +
> +static Maybe<bool>
> +TelemetryPrefValue()
> +{
> +  // Leave it unchanged if it's already set.
> +  // XXX: how could it already be set?

I presume it could be set by some platforms via preferences files? I'm not sure what order things are resolved.

::: modules/libpref/Preferences.cpp:2937
(Diff revision 1)
> +  // XXX: how could it already be set?
> +  if (Preferences::GetType(kTelemetryPref) != nsIPrefBranch::PREF_INVALID) {
> +    return Nothing();
> +  }
> +
> +    // Determine the correct default for toolkit.telemetry.enabled. If this

Is the indentation a little off here, or are my eyes wrong...

Comment 3

[Nicholas Nethercote [inactive]]

> The tortuous logic appears to have been reproduced correctly here. This
> should undergo manual QA with artificial channel builds, so please do
> request PI take a look into this.
> 
> We're actually just about to undergo a PI run for bug 1435753 that you might
> be able to piggyback on.

How do I request that? Is this something that happens before or after landing on mozilla-inbound? Should landing and/or PI of this wait until March 13 so it's in the FF61 dev cycle instead of the FF60 dev cycle?


> > +  // Leave it unchanged if it's already set.
> > +  // XXX: how could it already be set?
> 
> I presume it could be set by some platforms via preferences files? I'm not
> sure what order things are resolved.

I think they could be, though in practice they aren't, at least in a standard build configuration.

I find the logic of the non-Android case a lot easier to follow, because it unconditionally sets the default value of the pref.


> > +    // Determine the correct default for toolkit.telemetry.enabled. If this
> 
> Is the indentation a little off here, or are my eyes wrong...

You're right. I'm using clang-format consistently for libpref now, and occasionally it does weird things, especially around #ifdefs. But the benefits outweigh these kinds of niggles, IMO.

Comment 4

[Chris H-C :chutten]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> > The tortuous logic appears to have been reproduced correctly here. This
> > should undergo manual QA with artificial channel builds, so please do
> > request PI take a look into this.
> > 
> > We're actually just about to undergo a PI run for bug 1435753 that you might
> > be able to piggyback on.
> 
> How do I request that? Is this something that happens before or after
> landing on mozilla-inbound? Should landing and/or PI of this wait until
> March 13 so it's in the FF61 dev cycle instead of the FF60 dev cycle?
> 

Good question. If we decide to wait until 61 (and that might be prudent, given the convolutions of this code), then piggy-backing makes no sense. I'll look into this and get back to you.

Comment 5

[Chris H-C :chutten]

Word is back, if it's 61 it'll need a separate PI Request. Here's the mana page covering how PI Requests work: https://mana.mozilla.org/wiki/display/PI/PI+Request

The tl;dr is, land the patch, request PI assign some manual QA. When you mention the words "Telemetry" and "Preferences" you'll likely get madalin, who has done this twice before for this code (in the last six months or so) and has developed a knack for it. Timelines and schedules will be set and agreed to, you'll have input on the test plan to make sure the corners are covered, and then testing will proceed.

...or at least that's been my experience with this process. I've only done it the once :S

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8957446 [details]
Bug 1444276 - Refactor the telemetry pref setting code.

https://reviewboard.mozilla.org/r/226370/#review233030

Comment 7

[Nicholas Nethercote [inactive]]

Ok, thanks for the info, chutten! I'll go through that process once bug 1444275 lands, because that's related and is riskier than this patch.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce2e2c6b35ed4ae0052fef4b83440c0e632603a8
Bug 1444276 - Refactor the telemetry pref setting code. r=glandium,chutten

Comment 9

[Andreea Pavel [:apavel]]

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

Comment 10

[Tad]

This appears to be breaking compile.

 4:03.39 In file included from /home/$USER/gecko-dev/obj-arm-linux-androideabi/modules/libpref/Unified_cpp_modules_libpref0.cpp:2:
 4:03.39 /home/$USER/gecko-dev/modules/libpref/Preferences.cpp:2947:15: error: use of undeclared identifier 'prefValue'
 4:03.39   return Some(prefValue.EqualsLiteral("beta"));
 4:03.39               ^
 4:03.39 1 error generated.
 4:03.39 /home/$USER/gecko-dev/config/rules.mk:1045: recipe for target 'Unified_cpp_modules_libpref0.o' failed
 4:03.39 make[4]: *** [Unified_cpp_modules_libpref0.o] Error 1
 4:03.39 /home/$USER/gecko-dev/config/recurse.mk:73: recipe for target 'modules/libpref/target' failed
 4:03.40 make[3]: *** [modules/libpref/target] Error 2
 4:03.40 make[3]: *** Waiting for unfinished jobs....

should prefValue be channelPrefValue?

Apologies if this isn't the correct way to report.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Tad from comment #10)
> This appears to be breaking compile.

I ran into this also with a local Fennec build. I'll put up a patch.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Ok, thanks for the info, chutten! I'll go through that process once bug
> 1444275 lands, because that's related and is riskier than this patch.

I just sent the PI assistance request email.

Comment 13

[Nicholas Nethercote [inactive]]

PI testing has been completed, and looks like the following:

> toolkit.telemetry.enabled status:
> 
> Windows7(x64)
> 	Release - False (locked)
> 	Early Beta - True (locked)
> 	Late Beta - True (locked)
> 	Nightly - True (locked)
> 
> Ubuntu 14.0.4
> 	Release - False (locked)
> 	Early Beta - True (locked)
> 	Late Beta - True (locked)
> 	Nightly - True (locked)
> 
> MacOS 10.13.3
> 	Release - False (locked)
> 	Early Beta - True (locked)
> 	Late Beta - True (locked)
> 	Nightly - True (locked)
> 
> Android 8.0
> 	Release - False 
> 	Early Beta - True 
> 	Late Beta - True
> 	Nightly - True

This looks good to me. chutten, does it look good to you? (Note that the revisions tested included the changes from this bug *and* the changes from bug 1444275.)

Comment 14

[Chris H-C :chutten]

That looks good to me, too.