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

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Comment 2

[Ed Lee :Mardak]

Attachment: Bug 1415812 - Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream.

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

Comment 3

[Ed Lee :Mardak]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd887aed5fe231709c41d1b015de5c4aa9b2119

Comment 4

[Ed Lee :Mardak]

Talos run looks good: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a3f183201f7f183c263d554bfb15fbf0b0ed2ea4&newProject=try&newRevision=913ab73de39ca554cbd0b451b1d514e4bc38da81&framework=1&filter=linux&showOnlyImportant=1

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8928691 [details]
Bug 1415812 - Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream.

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

Comment 6

[Kate Hudson :k88hudson]

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!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4d1db669c93
Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream. r=k88hudson

Comment 8

[Noemi Erli[:noemi_erli]]

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

Comment 9

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/41dfb7b9f077a56ab99a38a0500fc4e3fbbc668e
chore(mc): Remove patches that as part of bug 1415812

Comment 10

[Ed Lee :Mardak]

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")

Comment 11

[Gerry Chang [:gchang]]

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.

Comment 12

[Francesco Lodolo [:flod]]

(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.

Comment 13

[Paul Oiegas [:poiegas], PM for (Ecosystem QA, Services QA, WebCompat QA, Hubs QA teams)]

Hi Gerry,

Sure thing! I will assign this to Ciprian to verify, since he's doing QA on AS and we'll report back.

Comment 14

[Ed Lee :Mardak]

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.

Comment 15

[Ciprian Muresan [:cmuresan], Ecosystem QA]

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.

Comment 16

[Gerry Chang [:gchang]]

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.

Comment 17

[Ed Lee :Mardak]

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.

Comment 18

[Gerry Chang [: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.

Comment 19

[Gerry Chang [:gchang]]

Hi :Mardak,
Any updates?

Comment 20

[Ed Lee :Mardak]

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.

Comment 21

[Ed Lee :Mardak]

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.

Comment 22

[Gerry Chang [:gchang]]

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.