Closed Bug 1525413 Opened 1 year ago Closed 1 year ago

Parallelize and wait for endpoint requests before rendering Discovery Stream

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 67
Iteration:
67.1 - Jan 28 - Feb 10
Tracking Status
firefox66 + verified
firefox67 --- verified

People

(Reporter: thecount, Assigned: thecount)

References

Details

(Keywords: github-merged)

Attachments

(2 files)

To deal with loading jank, for now we're going to wait until everything is ready until we render the layout.

Assignee: nobody → sdowne

For QA I would say just make sure dev-test-all or basic load and nothing looks out of place.

This bug is likely to break in very noticeable and dramatic ways, so if it's displaying content still I think we're good.

Iteration: --- → 67.1 - Jan 28 - Feb 10
Priority: -- → P1
Blocks: 1526066
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Summary: Don't render any layout stuff until it is ready → Parallelize endpoint requests and wait for them to be done to render Discovery Stream
Summary: Parallelize endpoint requests and wait for them to be done to render Discovery Stream → Parallelize and wait for endpoint requests before rendering Discovery Stream

Looks like this depends on Bug 1520258 to be uplifted

Can you write the uplift request for this?

Flags: needinfo?(sdowne)

Comment on attachment 9043424 [details]
Bug 1525413 - Parallelize and wait for endpoint requests before rendering Discovery Stream

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Experiment data less reliable. User performance issues.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

  1. Set the pref browser.newtabpage.activity-stream.discoverystream.config to {"enabled":true,"show_spocs":true,"layout_endpoint":"https://getpocket.com/v3/newtab/layout?version=1&consumer_key=40249-e88c401e1b1f2242d9e441c4&layout_variant=dev-test-all"}
  2. Load up a new tab.
  3. Watch for when new tab loads, it loads all in one shot, and not concurrently, ie elements shouldn't load until everything is ready, and load and render once, and not load elements as they come in, causing the page to jump around.

I'm not able to throttle this using responsive design mode tools as responsive design mode does wonky things in about:home, but with the above steps, I have been able to do it fast enough to see that everything gets loaded at once. If you need to try it again, you can change the pref from the above one in step 1 to {"enabled":true,"show_spocs":true,"layout_endpoint":"https://getpocket.com/v3/newtab/layout?version=1&consumer_key=40249-e88c401e1b1f2242d9e441c4&layout_variant=dev-test-all" }, notice the extra space after dev-test-all" which triggers a fresh load/cache clear.

List of other uplifts needed

Bug 1520258

Risk to taking this patch

Medium

Why is the change risky/not risky? (and alternatives if risky)

It's preffed off and has tests, but really hard to verify because it relies on network requests..

String changes made/needed

None

Flags: needinfo?(sdowne)
Attachment #9043424 - Flags: approval-mozilla-beta?

Can you verify this one?

Flags: needinfo?(bnagabandi)
Blocks: 1527816
Blocks: 1527819

There are 2 different issues found on Mac OS and Windows OS respectively.

Windows :

"dev-test-all" end-point doesn't work on Windows 10 Pro.

Tracking here : bug 1527816

Mac OS :

Layout has some words cutoff specifically for the letter g on some cards (maybe css issue?)

Tracking here : bug 1527819

No longer blocks: 1527816, 1527819
Flags: needinfo?(bnagabandi) → needinfo?(sdowne)
Blocks: 1527819

Brahmini, it sounds like those issues are unrelated to this patch and don't need to block the uplift.

  1. For Bug 1527816, we're unable to reproduce this/don't have any reports of nightly not working on Windows 10 – are you sure you have your configuration set up correctly?
  2. For Bug 1527819, this reproduce-able on nightly but unrelated and non-blocking for this uplift.

Comment on attachment 9043424 [details]
Bug 1525413 - Parallelize and wait for endpoint requests before rendering Discovery Stream

Let's go ahead with this uplift for beta 8 since the issues found by QA aren't blocking.

Attachment #9043424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Clearing need info, Kate said it well.

Flags: needinfo?(sdowne)

I had no clue about the pref browser.newtabpage.activity-stream.discoverystream.optOut.0 (which was true), so having incorrect value on Windows caused the issue.

After pref reset, it works as expected.

Thanks for the clarification.

Tested bug 1527816.

Closing as verified.

Status: RESOLVED → VERIFIED

I have verified that the issue is no longer reproducible on the latest Beta 66.0b8 (Build ID 20190214195736) on Windows 10 x64, Mac 10.14, and Arch Linux 4.14.3.

Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.