Closed Bug 1415812 Opened 2 years ago Closed 2 years ago

Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream

Categories

(Firefox :: New Tab Page, defect)

57 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- verified

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Whiteboard: [export])

User Story

https://github.com/mozilla/activity-stream/compare/firefox-58b2...39442ee83e058b1255dead870c3dda44cfdf1a11

Attachments

(1 file)

No description provided.
Depends on: 1407228
See Also: → 1415892
Depends on: 1412265
User Story: (updated)
Summary: Add ... and bug fixes to Activity Stream → Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream
Depends on: 1415665
Depends on: 1411867
Depends on: 1411301
Duplicate of this bug: 1415892
User Story: (updated)
User Story: (updated)
Comment on attachment 8928691 [details]
Bug 1415812 - Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream.

https://reviewboard.mozilla.org/r/199938/#review205582

Looks good, thanks!
Attachment #8928691 - Flags: review?(khudson) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b4d1db669c93
Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream. r=k88hudson
https://hg.mozilla.org/mozilla-central/rev/b4d1db669c93
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Blocks: 1418130
Comment on attachment 8928691 [details]
Bug 1415812 - Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream.

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream features added in 58 (Pocket personalization, collapsible sections)
[User impact if declined]: Users would see an un-dismissable disclaimer message for the Pocket section that now shows more personalized stories. Also users would more often see low resolution favicons and thumbnails instead of rich icons from the top site.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes 20171119220126
[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?]: Some
[Why is the change risky/not risky?]: There isn't actually that much new behavior change, but it does include usage of a pref-controlled rich icon service, which is only used as a fallback when a top site lacks one. The exported patch is large due to how Activity Stream pre-renders and packages 1) per-locale html/strings for better startup performance introduced in middle of Nightly 58 as well as a 2) per-platform css (win/mac/linux) for better platform integration and startup performance introduced towards the end of Nightly 58.
[String changes made/needed]: Changed yes, needed no. flod has reviewed the en-US changes, and this also includes the translated strings from Pontoon (including a newly added "gl")
Attachment #8928691 - Flags: approval-mozilla-beta?
See Also: → 1408121
Blocks: 1419601
Hi :Mardak,
You are not supposed to mark the bug verified by yourself. In this case, I would like to have a QE to verify this. Change it back to fixed.

Hi Paul,
Can you help verify if this was fixed and make sure there is no regression?

Hi :flod,
Might need your help to review the strins.
Flags: needinfo?(paul.oiegas)
Flags: needinfo?(francesco.lodolo)
(In reply to Gerry Chang [:gchang] from comment #11)
> Hi :flod,
> Might need your help to review the strins.

No review needed on my side for the strings, it's only a dump of strings from an external repository dedicated to localization.
Flags: needinfo?(francesco.lodolo)
Hi Gerry,

Sure thing! I will assign this to Ciprian to verify, since he's doing QA on AS and we'll report back.
Flags: needinfo?(paul.oiegas) → needinfo?(ciprian.muresan)
I set the verified flag as Ciprian has verified bugs that were fixed with this export. Similar to how I set the fixed flag for those bugs when this bug makes it to mozilla-central.
I have verified all the dependencies that this bug had (at least the ones that could be manually tested), verified all the changes introduced with this new export on Windows 10 x64, Mac 10.12.6 and Arch Linux x64 and I can confirm that the export works as expected.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ciprian.muresan)
Hi :Mardak,
Can you split the patches to different bugs, like icon + disclaimer + "bug fixes"? Merging this huge patch makes us hard to manage the release and difficult to find regression.
Flags: needinfo?(edilee)
gchang, I could try to split them up, but splitting them up and trying to land them individually could increase risk as these are now untested patches we're trying to uplift directly to beta as they won't apply to mozilla-central. In particular, splitting up the patches by grouping together related functionality means some changes might appear earlier/later than when the commit was made leading to more risk of untested behaviors as there is now more possible intermediate states of the code.

So we want to split it up into the parts as you suggested? rich icons, pocket stuff, other fixes? And we'll need to do this before we can uplift bug 1419601. The number of "id" users has dropped to ~30% of its usual beta levels, so we probably need to split up the patches and get them tested quickly before they reach the 3% levels we're seeing in Nightly.
Flags: needinfo?(gchang)
Is there any way to improve the whole development process? Uplifting all patches into beta at once and not baked enough in nightly is really not a good idea. Beta is supposed to be a stabilized phase given we don't have aurora phase anymore. This huge patches are supposed to stay in nightly to bake long enough. 
Is it possible to let this ride the train? If not, I prefer to split them up and we can pick whatever the highest value/priority bugs for users in beta.
Flags: needinfo?(gchang)
Hi :Mardak,
Any updates?
There's some pushes to try from people not on US Thanksgiving holiday:
https://public.etherpad-mozilla.org/p/58-uplift-patches

At a quick glance, it looks like the patch that landed here in m-c was split into 5 patches for uplift, and all of the try pushes when split up instead of keeping the same patch as the one from m-c have some tests failing and one of them has a lot more test failures.
Depends on: 1414979
Depends on: 1421302
User Story: (updated)
No longer depends on: 1413550
Flags: needinfo?(edilee)
Depends on: 1421306
Depends on: 1421312
The export has been manually split up into 10 patches with 8 of them looking for uplift as part of the various bugs: 1407228, 1411867, 1412265, 1414979, 1415665, 1419601, 1421302, 1421306, 1421312

The patch for bug 1411301 won't need to be fixed for 58. And various other 12 commits as the "10th patch" won't need to be fixed for 58 either.

I guess somebody will be attaching the direct-to-mozilla-beta patches in each of the above 8 bugs to individually request uplift with links to individual try runs. Although the commits on try cannot land as-is to mozilla-beta as each of the 8 of them conflict with each other, so there will need to be extra care in approving and landing them in the appropriate order.
Assignee: nobody → edilee
Depends on: 1407153
No longer blocks: 1418130
Blocks: 1421917
Comment on attachment 8928691 [details]
Bug 1415812 - Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream.

Since this bug is split into different bugs. Mark this one won't fix for 58.
Attachment #8928691 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 1430640
Depends on: 1433637
Whiteboard: [export]
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.