Closed
Bug 1394777
Opened 7 years ago
Closed 7 years ago
Perma-red browser_UsageTelemetry_content_aboutHome.js
Categories
(Firefox :: New Tab Page, enhancement)
Firefox
New Tab Page
Tracking
()
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: andreio, Assigned: andreio)
References
Details
User Story
Attachments
(1 file)
Test browser/modules/test/browser/browser_UsageTelemetry_content_aboutHome.js fails with browser.newtabpage.activity-stream.aboutHome.enabled;true because it waits for `AboutHomeLoadSnippetsCompleted` events. We want to change this in order to support Activity Stream on about:home.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → andrei.br92
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8902226 [details] Bug 1394777 - Add test for Activity Stream about:home search. https://reviewboard.mozilla.org/r/173748/#review179236 It looks like Dexter wrote (bug 1303333) and fixed some intermittents (bug 1313825) related to this test. I'm not sure if removing the listener is the right approach especially if it causes the test to intermittently fail, but that you should be able to try. Dexter, how should we get this test to pass with Activity Stream enabled on about:home? It does have a search box reusing the same contentSearchUI.js, and this test doesn't seem like it's actually using snippets, so the waiting for event might not be relevant?
Updated•7 years ago
|
Flags: needinfo?(alessio.placitelli)
Comment 3•7 years ago
|
||
(In reply to Ed Lee :Mardak from comment #2) > Comment on attachment 8902226 [details] > Bug 1394777 - Wait for search input to render before starting about:home > telemetry test. > > https://reviewboard.mozilla.org/r/173748/#review179236 > > Dexter, how should we get this test to pass with Activity Stream enabled on > about:home? It does have a search box reusing the same contentSearchUI.js, > and this test doesn't seem like it's actually using snippets, so the waiting > for event might not be relevant? The rationale behind waiting for "AboutHomeLoadSnippetsCompleted" was to be completely sure that the about:home page was completely loaded and ready before doing anything. Any better way to do this would make that listener useless and ok to drop. However, I'm not sure if > await ContentTask.spawn(tab.linkedBrowser, null, async function() { > await ContentTaskUtils.waitForCondition(() => content.document.querySelector("input")); > }); is the best way to do so, as I don't actively work on front-end code. :mak might give a better opinion on that part (thanks :mak :D)! Regardless of the chosen approach, I'd try to run the failing test 10-20 times on try and see that it stays green before landing this. Also, this seems to be related to bug 1347693.
Flags: needinfo?(alessio.placitelli) → needinfo?(mak77)
Comment 4•7 years ago
|
||
AboutHomeLoadSnippetsCompleted is basically only needed if the code cares to work with snippets, and as Dexter said, it could be used as a way to tell "even the slowest thing on the page has finished loading". In this case, I think we only care about search. About:home setups search like this: 1. on "pageshow" event it invokes setupSearch 2. setupSearch creates ContentSearchUIController It also sends a custom "AboutHomeLoad" event used by tab-content.js to setup various things, but those look unrelated to search and thus we may not care. Thus, just for the search functionality, I assume for both pages we could executeSoon after the "pageshow" event?
Flags: needinfo?(mak77)
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
I've changed the patch to consider Activity Stream about:home separate from the current about:home. This is not yet ready for review, it's waiting either on https://github.com/mozilla/activity-stream/pull/3325 or 1381460 because currently the source and healthreportkey of Activity Stream about:home are not correct.
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada691e0088240b52f76a1d83a364067ac1931db looks good. Mardak, can you please have another look at the patch when you get the time?
Flags: needinfo?(edilee)
Comment 9•7 years ago
|
||
I retriggered the bc runs that included browser_UsageTelemetry_content_aboutHome. Although the try run only had linux64 runs. I triggered a try run from mozreview: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac7846a8bf3
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8902226 [details] Bug 1394777 - Add test for Activity Stream about:home search. https://reviewboard.mozilla.org/r/173750/#review180854 Looks good locally and on try
Attachment #8902226 -
Flags: review?(edilee) → review+
Comment 11•7 years ago
|
||
Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a1c4ec3f36f6 Add test for Activity Stream about:home search. r=Mardak
Updated•7 years ago
|
Flags: needinfo?(edilee)
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1c4ec3f36f6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
•