Parallelize and wait for endpoint requests before rendering Discovery Stream

VERIFIED FIXED in Firefox 66

Status

()

defect
P1
normal
VERIFIED FIXED
4 months ago
4 months ago

People

(Reporter: thecount, Assigned: thecount)

Tracking

({github-merged})

unspecified
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66+ verified, firefox67 verified)

Details

Attachments

(2 attachments)

Assignee

Description

4 months ago

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

Assignee

Updated

4 months ago
Assignee: nobody → sdowne
Assignee

Comment 2

4 months ago

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.

Assignee

Updated

4 months ago
Iteration: --- → 67.1 - Jan 28 - Feb 10

Updated

4 months ago
Priority: -- → P1
Keywords: github-merged
Blocks: 1526066

Comment 3

4 months ago
Status: NEW → RESOLVED
Closed: 4 months 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)
Assignee

Comment 7

4 months ago

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)

Updated

4 months ago
Blocks: 1527816

Updated

4 months ago
Blocks: 1527819

Comment 9

4 months ago

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)

Updated

4 months ago
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+
Assignee

Comment 12

4 months ago

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.

Updated

4 months ago
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.

You need to log in before you can comment on or make changes to this bug.