Closed Bug 1175606 Opened 5 years ago Closed 4 years ago

Private Browsing Mode should enable Tracking Protection by default

Categories

(Firefox :: General, defect, P1)

defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox41 --- affected
firefox42 --- verified

People

(Reporter: past, Assigned: past)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(1 file, 4 obsolete files)

When the user opens a new window in PBM, it should have TP enabled by default.
Flags: firefox-backlog?
Rank: 6
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P1
I think this is all there is to it.
Assignee: nobody → past
Status: NEW → ASSIGNED
Points: --- → 1
Flags: qe-verify+
Iteration: --- → 42.1 - Jul 13
Assignee: past → nobody
Status: ASSIGNED → NEW
Iteration: 42.1 - Jul 13 → ---
QA Contact: mwobensmith
When we flip the default value of the preference and enable the feature for all users of Nightly and other channels, we'll have to show the new design of the "about:privatebrowsing" page implemented in bug 1177152.

The new version is temporarily controlled by the "privacy.trackingprotection.ui.enabled" preference, so that we can test it manually but is disabled by default in Nightly. We'll just have to remove the dependence on the preference (and keep the "ui.enabled" preference to its value of false).
Points: 1 → 2
Whiteboard: [fxprivacy] → [fxprivacy] [blocked on feature readiness]
Whiteboard: [fxprivacy] [blocked on feature readiness] → [fxprivacy] [campaign] [blocked on feature readiness]
Javaun confirmed this is ready for selection in IT 42.3
Whiteboard: [fxprivacy] [campaign] [blocked on feature readiness] → [fxprivacy] [campaign]
Assignee: nobody → past
Status: NEW → ASSIGNED
Iteration: --- → 42.3 - Aug 10
Attached patch Patch v2 (obsolete) — Splinter Review
Updated the patch according to comment 2. The trackingprotection mochitests pass locally.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4058d217004a
Attachment #8624199 - Attachment is obsolete: true
Attachment #8640112 - Flags: review?(ttaubert)
Attachment #8640112 - Flags: review?(ttaubert) → review+
I suggest we wait a few days before landing this in Nightly, in order to test and implement the revised about:privatebrowsing design first, which is significantly different from the current one.

Bug 1188455 has references to the updated mockups.
Comment on attachment 8640112 [details] [diff] [review]
Patch v2

Flagging the patch as a reminder.
Attachment #8640112 - Flags: checkin-
Comment on attachment 8640112 [details] [diff] [review]
Patch v2

I don't get the reasoning why we wouldn't land it in Nightly? It's a technical audience that could help us get some more test coverage. This shouldn't block on any about:privatebrowsing design changes, we have no A/B tests or telemetry on Nightly to figure out if that makes a difference anyway.
Attachment #8640112 - Flags: checkin-
The reasoning is that we would show a version of about:privatebrowsing we're not going to use, for just three days or so. I can see how our technical audience may be surprised. On the other hand, if we land this sooner we gain three more days of testing on the actual Tracking Protection feature, so there is an advantage as well.
Would love to get some input from Aislinn but she's OOO until the end of the week. Maybe we can just discuss this in the standup in a few minutes. The new design is indeed very different but it does just as well promote TP on about:privatebrowsing. I would expect at least 90% of the Nightly audience to understand what TP is immediately... But let's ask other people too.
Comment on attachment 8640112 [details] [diff] [review]
Patch v2

Talked it through, let's hold off until bug 1188455 and bug 1184312 are fixed.
Attachment #8640112 - Flags: checkin-
Attached patch Patch v3 (obsolete) — Splinter Review
Fixes a test that I missed before locally, but was caught on try. Still not planning to land yet, per the discussion above.
Attachment #8640112 - Attachment is obsolete: true
Attachment #8641093 - Flags: review?(ttaubert) → review+
This is ready to land, but the tree is closed so let's see if the sheriffs can land it when they reopen it.
Keywords: checkin-needed
Depends on: 1192339
This should actually be rebased on top of bug 1190427.
Keywords: checkin-needed
Depends on: 1190427
Attached patch Patch v4 (obsolete) — Splinter Review
Rebased.
Attachment #8641093 - Attachment is obsolete: true
Maybe we could override privacy.trackingprotection.pbmode.enabled in the talos prefs
I'll put together a patch for talos
Reminder to include again the removal of the code that hides the Tracking Protection section from about:privatebrowsing in the next version of the patch.
Depends on: 1192416
Attached patch Patch v5Splinter Review
Attachment #8645150 - Attachment is obsolete: true
Flags: needinfo?(past)
Backed out in https://hg.mozilla.org/integration/fx-team/rev/c56e7866445c since the talos bump had to be backed out.
Depends on: 1192534
Not sure merge into m-c removed this patch.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1192611 for breaking win7 Start Menu

I may have messed up regression window testing but testing points to:

20150807135006 799742085c18 as Good
20150807143017 152eafb9dc89 as Bad
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #27)
> Not sure merge into m-c removed this patch.
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1192611 for breaking win7
> Start Menu
> 
> I may have messed up regression window testing but testing points to:
> 
> 20150807135006 799742085c18 as Good
> 20150807143017 152eafb9dc89 as Bad

Commented in 1192611 for further discussion, but both of the previous landings have been backed out before hitting m-c.  And in fact 152eafb9dc89 was the backout revision
(In reply to Pulsebot from comment #26)
> https://hg.mozilla.org/integration/fx-team/rev/c0a2c18218d1

Note, this follow up commit was to fix an issue with broken crashtests in linux 32 debug [0].  I needed to disable TP separately for reftests to fix that issue (which, sidenote, apparently have *many* places that default prefs can be set [1]).  Picked layout/tools/reftest/runreftest.py because that's where safebrowsing was being disabled as well.  It seems to have worked since all the retriggers after this look good [2].

[0]: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=dc44b9a3b542
[1]: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Need_to_set_preferences_for_test-suites
[2]: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=c0a2c18218d1
No longer depends on: 1192611
I don't know what happened or how it cleared. I'm totally confused at this point.

See my comment here: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1192611#c5

Sorry for noise.
https://hg.mozilla.org/mozilla-central/rev/dc44b9a3b542
https://hg.mozilla.org/mozilla-central/rev/c0a2c18218d1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Just tested the m-c build with the patch landing in cset:

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

The missing Nightly Icon in the Win7 Start Menu 'Most Frequent used list' is again missing as described in bug 1192611

Please see my comments in that bug about this bug needing a 'clobber' to work correctly.
https://bugzilla.mozilla.org/show_bug.cgi?id=1192611#c6

I guess tomorrow's Nightly will maybe work because Nightly's are 'clobber builds'.
Testing in today's Nightly build the Nightly Icon appears now in the Most Frequent list from the Win7 Start Orb. 

Appears to me that whenever this patch is backed out or landed that it needs a clobber to fully activate/deactivate the changes in this patch.  

As to why its affecting the Start Menu Frequently used list is beyond my understanding.
Sanity test on Mac/Win/Linux to confirm enabled by default in private browsing looks good. That's all I will verify in this bug.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.