Closed Bug 1409058 Opened 8 years ago Closed 8 years ago

Add pref to show or hide snippets in the pref panel

Categories

(Firefox :: New Tab Page, defect, P1)

Product:
Firefox
Component:
New Tab Page
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: k88hudson, Assigned: k88hudson)

Details

Attachments

(2 files)

Bug 1409058 - Add pref to show or hide snippets in the pref panel
59 bytes, text/x-review-board-request
Mardak
: review+
ritu
: approval-mozilla-beta+
Details
for beta
4.28 KB, patch
Details | Diff | Splinter Review
This pref will allow us to show or hide snippets as an option in the AS Preferences panel. If the option is not show in the panel, snippets will also not be shown on Activity Stream.
Assignee: nobody → khudson
Attachment #8918917 - Flags: review?(edilee)
[Tracking Requested - why for this release]: this is a contingency pref in case we need to pref off Snippets for any reason. I marked it P2 to indicate that this will not *break* anything if it's not uplifted - feel free to weigh it against other risks and reject
status-firefox57: --- → affected
tracking-firefox57: --- → ?
Priority: -- → P2
Once you land a patch for this please go ahead and request uplift to beta.
status-firefox58: --- → affected
tracking-firefox57: ? → +
Thanks, Liz! :mardak, let's review/land/uplift this one.
Flags: needinfo?(edilee)
Priority: P2 → P1
Comment on attachment 8918917 [details] Bug 1409058 - Add pref to show or hide snippets in the pref panel https://reviewboard.mozilla.org/r/189800/#review195670 I have more detailed review https://github.com/mozilla/activity-stream/pull/3717 that we can land separately from this patch if we want something sooner. In particular, if we land and uplift this patch, there will be some beta users who won't see the option to turn snippets back on, which might be fine as those users had already indicated they didn't want snippets. However, by uplifting this sooner, there will be fewer users who end up in that state when upgrading to Firefox 58.
Attachment #8918917 - Flags: review?(edilee) → review+
Flags: needinfo?(edilee)
Hi Kate, I am looking at the tracked 57 bugs. Is this ready to land on central? If this is a must uplift for 57, the sooner we land it the better. Thanks!
Flags: needinfo?(khudson)
Ok, I updated this to use a different pref name. I'll add the feed logic in a follow-up for 58.
Flags: needinfo?(khudson)
Comment on attachment 8918917 [details] Bug 1409058 - Add pref to show or hide snippets in the pref panel https://reviewboard.mozilla.org/r/189800/#review196476 New approach with `disableSnippets` looks good fixing the goodbye-forever-snippets issue. This has some caveats of probably falling back to onboarding and no telemetry.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 512a46325139 -d cdffffb1bfcf: rebasing 428430:512a46325139 "Bug 1409058 - Add pref to show or hide snippets in the pref panel r=Mardak" (tip bug1409058@mozilla-central) merging browser/extensions/activity-stream/data/content/activity-stream.bundle.js merging browser/extensions/activity-stream/lib/ActivityStream.jsm warning: conflicts while merging browser/extensions/activity-stream/data/content/activity-stream.bundle.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by khudson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb5138e5bd55 Add pref to show or hide snippets in the pref panel r=Mardak
https://hg.mozilla.org/mozilla-central/rev/eb5138e5bd55
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8918917 [details] Bug 1409058 - Add pref to show or hide snippets in the pref panel Approval Request Comment [Feature/Bug causing the regression]: This allows us to disable Activity Stream snippets for all users without affecting the original about:home snippets [User impact if declined]: The EoY fundraising campaign will be impacted if we need to turn off all snippets on Activity Stream without this patch landed [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: no [Why is the change risky/not risky?]: small amount of code all behind a pref [String changes made/needed]: no
Attachment #8918917 - Flags: approval-mozilla-beta?
Comment on attachment 8918917 [details] Bug 1409058 - Add pref to show or hide snippets in the pref panel Must fix, Beta57+
Attachment #8918917 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs a rebased patch for Beta.
Flags: needinfo?(khudson)
Attached patch for beta — — Splinter Review
Is this acceptable?
Flags: needinfo?(khudson) → needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: