Closed
Bug 1415812
Opened 7 years ago
Closed 7 years ago
Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
VERIFIED
FIXED
Firefox 59
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
(Whiteboard: [export])
User Story
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
k88hudson
:
review+
gchang
:
approval-mozilla-beta-
|
Details |
No description provided.
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Summary: Add ... and bug fixes to Activity Stream → Fix broken Pocket disclaimer, missing rich icons and bug fixes to Activity Stream
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Comment 6•7 years ago
|
||
mozreview-review |
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
Comment 8•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment 9•7 years ago
|
||
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
Assignee | ||
Comment 10•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Comment 11•7 years ago
|
||
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•7 years ago
|
||
(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)
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
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)
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
Hi :Mardak,
Any updates?
Assignee | ||
Comment 20•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 21•7 years ago
|
||
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.
Updated•7 years ago
|
Assignee: nobody → edilee
Comment 22•7 years ago
|
||
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-
Updated•7 years ago
|
Assignee | ||
Updated•6 years ago
|
Whiteboard: [export]
Updated•5 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•