1438433 - Two network pref tweaks

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P1)

Description

[Nicholas Nethercote [inactive]]

I have a couple of network pref tweaks.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1438433 - Remove the network.predictor.redirect-likely-confidence pref. .

It's unused.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1438433 - Don't set Predictor::mCleanedUp directly. .

mCleanedUp is a VarCache variable, which mirrors the canonical value of the
network.predictor.cleaned-up pref. When the canonical pref value is modified,
e.g. by SetBool(), then mCleanedUp is also updated.

But the updating relationship is one-way -- if mCleanedUp is modified, the
canonical value of the pref is not updated. Such an inconsistency is bad! For
example, Predictor.cpp will use mCleanedUp's value, but about:config will show
the canonical value.

(For this reason, VarCache prefs are meant to be read-only outside of libpref.
Bug 1436655 will enforce this.)

This patch replaces one direct modification of mCleanedUp with a SetBool() call
on the canonical value. This makes it match
PredictorThreadShutdownRunner::Run(), which also uses SetBool().

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

Comment 3

[u408661]

Comment on attachment 8951153 [details]
Bug 1438433 - Remove the network.predictor.redirect-likely-confidence pref. .

https://reviewboard.mozilla.org/r/220416/#review226850

Comment 4

[u408661]

Comment on attachment 8951154 [details]
Bug 1438433 - Don't set Predictor::mCleanedUp directly. .

https://reviewboard.mozilla.org/r/220418/#review226856

We can't set the value in prefs until PredictorThreadShutdownRunner::Run happens, as the cleanup could fail for a transient reason, and we would want to try again at the next startup in that case.

We can probably just make mCleanedUp a copy (not cache) of the value in prefs at startup, and leave this direct modification as-is.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8951153 [details]
Bug 1438433 - Remove the network.predictor.redirect-likely-confidence pref. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220416/diff/1-2/

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8951154 [details]
Bug 1438433 - Don't set Predictor::mCleanedUp directly. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220418/diff/1-2/

Comment 7

[u408661]

Comment on attachment 8951154 [details]
Bug 1438433 - Don't set Predictor::mCleanedUp directly. .

https://reviewboard.mozilla.org/r/220418/#review233014

LGTM, but the first line of the commit message needs a bit of rewording. Sorry for the lag, got lost in my mess of open tabs!

::: commit-message-e8606:1
(Diff revision 2)
> +Bug 1438433 - Don't set Predictor::mCleanedUp directly. r=hurley.

This no longer matches the change being made, which makes the commit message confusing.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95dbe9cb93d791ceca6738218824b38a5094c1c5
Bug 1438433 - Remove the network.predictor.redirect-likely-confidence pref. r=hurley.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb82cefbd7aa68e080560b519feea5a86feefe1
Bug 1438433 - Make network.predictor.cleaned-up a non-VarCache pref. r=hurley.

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/95dbe9cb93d7
https://hg.mozilla.org/mozilla-central/rev/8eb82cefbd7a