1591059 - setDefaultPrivate (and variations) should ensure separatePrivateDefault preference is set to true.

Status: RESOLVED FIXED

(Firefox :: Search, task, P2)

Description

[Mark Banner (:standard8)]

setDefaultPrivate was originally designed alongside getDefaultPrivate and for both of those we would fallback to getting the normal search engine if having a separate private default was turned off.

I did this because I was thinking about other apps potentially calling code which then would call the private variations.

However, given the testing received in bug 1587804, and the subsequent discussion, I think it would be fine for setDefaultPrivate to ensure the browser.search.separatePrivateDefault is set to true.

Other apps are unlikely to call code to set the default unless its in their preferences - if so they'll want it on as well. Additionally the code where we call it is all browser specific, so it only affects us.

Comment 1

[Richard G (:cardo)]

Is it simply removing this.defaultPrivateEngine and returning (this._separatePrivateDefault = true) in setDefaultPrivate?

Comment 2

[Mark Banner (:standard8)]

(In reply to Richard G (:cardo) from comment #1)

Is it simply removing this.defaultPrivateEngine and returning (this._separatePrivateDefault = true) in setDefaultPrivate?

Not quite. We still want to set the private default engine.

When I wrote this, I'd forgotten about the defaultPrivateEngine setter. So what we actually want to do is to set this._separatePrivateDefault = true in the defaultPrivateEngine setter, just before doing the call to this._setEngineDefault(). We'll need to create a _separatePrivateDefault setter that sets the value of the preference via Services.prefs.setBoolPref(SearchUtils.BROWSER_SEARCH_PREF + "separatePrivateDefault", value).

The test in toolkit/components/search/tests/xpcshell/test_defaultPrivateEngine.js will also need updating. You can run it via ./mach xpcshell-test path/to/test.

Comment 3

[Richard G (:cardo)]

Please can I get pointers on what I have to update in the test file to make it pass?

Comment 4

[Mark Banner (:standard8)]

(In reply to Richard G (:cardo) from comment #3)

Please can I get pointers on what I have to update in the test file to make it pass?

Sorry for the delay, feel free to request information from me in future as I'll likely get back to it quicker.

The setup and test_defaultPrivateEngine parts of the test shouldn't need a change.

test_defaultPrivateEngine_turned_off needs to be changed so that when defaultPrivateEngine is assigned, the tests take account of the change of functionality, roughly:

• no need to check for the "normal" notification.
• The defaultEngine (the normal one) should remain the same as before.
• We'll need to add a check that browser.search.separatePrivateDefault is set to true (use getBoolPref in a similar way to setBoolPref is being used).

I think the test_defaultPrivateEngine_ui_turned_off section of the test should also not need to change.

Comment 5

[Richard G (:cardo)]

Attachment: Bug 1591059 - Make setDefaultPrivate (and variations) ensure separatePrivateDefault preference is set to true. r?Standard8

Comment 6

[Richard G (:cardo)]

Hi Mark! I think I may need more concrete instructions to solve this. Pls take a look at my initial work and you can leave notes.

Comment 7

[Richard G (:cardo)]

Hi Mark. Happy New Year. :)
I left a comment on the change you proposed. I'm not sure if you saw it.

The latest change fails the test (line 186) that checks if separate private default pref is set to true.

Comment 8

[Mark Banner (:standard8)]

(In reply to Richard G (:cardo) from comment #7)

Hi Mark. Happy New Year. :)

Hi Richard, Happy New Year.

I left a comment on the change you proposed. I'm not sure if you saw it.

The latest change fails the test (line 186) that checks if separate private default pref is set to true.

I didn't see it, and I can't find it at the moment - it might be that in phabricator once you've added one or more comments, you need to hit submit down the bottom as well.

Anyway, I've just realised the issue, the code snippet I gave you for the defaultPrivateEngine setter was wrong. I missed the ! in the if statement, the if statement should be:

if (!this._separatePrivateDefaultPrefValue) {

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43ead86af68b
Make setDefaultPrivate (and variations) ensure separatePrivateDefault preference is set to true. r=Standard8

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bb0f3d9eb9d
Backed out changeset 43ead86af68b as requested by Standard8 for xpcshell failures. CLOSED TREE

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25b6ff764c82
Make setDefaultPrivate (and variations) ensure separatePrivateDefault preference is set to true. r=Standard8

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/25b6ff764c82