Closed Bug 1042876 Opened 7 years ago Closed 7 years ago

Update newtab endpoints to new v2/links


(Firefox :: New Tab Page, defect)

Not set



Firefox 34
Tracking Status
firefox33 + fixed
firefox34 + fixed


(Reporter: Mardak, Assigned: Mardak)





(2 files, 4 obsolete files)

We'll want to update both click and links:
Flags: firefox-backlog+
Depends on: 1043687
adw, do you know what's a good way to turn off this pref for tests? There's hooks that prevent external connections:

PROCESS-CRASH | | application crashed [@ nsSocketTransport::InitiateSocket()]
I see there's bug 1022785 but then also bug 1036028. It seems the pattern is to have the test harness set the appropriate prefs to prevent remote connections.
The only tests that trigger code paths that use those prefs are tiles-related, right?  If so, you can just set the prefs in the relevant tests or head files by using SpecialPowers.pushPrefEnv [1] in mochitest variants or the usual pref services APIs in both mochitests and xpcshell tests.

The mochitest framework sets a bunch of prefs when it starts if I recall correctly, so we could set the prefs there as a more heavyweight solution, if unrelated tests end up triggering these paths.  Like how bug 892875 turned off background thumbnailing for all mochitests.

For xpcshell tests, I don't think there's any batch of prefs that the framework sets when it starts, but xpcshell tests are generally more isolated than mochitests, so it seems like only tiles-related tests should be triggering these paths, so you can simply use the usual pref service APIs when the relevant tests start.

Does that help?

(In reply to Drew Willcoxon :adw from comment #3)
> The only tests that trigger code paths that use those prefs are
> tiles-related, right?
Not quite. Because the new tab gets preloaded, the directory links endpoint will be fetched even if the test doesn't require it directly. So it seems that we'll need to update the various test harnesses to change the default prefs ?
How could I forget.  Sounds fine to me.
oyiptong, the current urls I have in patches:

You mentioned bumping them to v2 elsewhere. Should I do that and keep the other parts of the urls the same?
Flags: needinfo?(oyiptong)
oyiptong, also in bug 1042214, I remove the directoryCount object from the fetch, so the fetch's payload is just {"locale":"en-US"} which arguably looks like the view/click payload with "tiles" being optional.
I note comment 7 as currently v1 returns 400 bad request for those without directoryCount.
Attached patch wip (obsolete) — Splinter Review
wip pending link confirmation from oyiptong
Assignee: nobody → edilee
Notes: could also remove toolkit/content/directoryLinks.json
The new urls will be the same, save for the version denominator.

They will be:
Flags: needinfo?(oyiptong)
Depends on: 1054341
Summary: Update newtab endpoints to new v1/links → Update newtab endpoints to new v2/links
Attached patch v1 (obsolete) — Splinter Review
Attachment #8472752 - Attachment is obsolete: true
Attachment #8473730 - Flags: review?(adw)
Comment on attachment 8473730 [details] [diff] [review]

Review of attachment 8473730 [details] [diff] [review]:

Yay remote links!  It looks like the only thing that still uses directoryLinks.json is test_DirectoryLinksProvider.js, so shouldn't that file be removed along with this patch?  It can be moved to the test directory if the test still really needs it.
Oh duh. I even left comment 10 to remind myself.
Bug 1042214 has changes to turn off remote connections for tests.
URL: 1042214
Attached patch v2Splinter Review
test_DirectoryLinksProvider ran fine without the file.
Attachment #8473730 - Attachment is obsolete: true
Attachment #8473730 - Flags: review?(adw)
Attachment #8474024 - Flags: review?(adw)
Comment on attachment 8474024 [details] [diff] [review]

Review of attachment 8474024 [details] [diff] [review]:

Oh I see, directoryLinks.json is also the name of the file in the profile directory where we cache remote links, not only the directory.source file packaged at that chrome URI, and that's what the test is concerned with.
Attachment #8474024 - Flags: review?(adw) → review+
Blocks: 1057602
Attached patch fix reftest burning ? (obsolete) — Splinter Review
testing/profiles/prefs_general.js wasn't enough
Attached patch reftest fix instead? (obsolete) — Splinter Review
Attachment #8477777 - Attachment is obsolete: true
Attachment #8477781 - Attachment is obsolete: true
Attachment #8477782 - Flags: review?(edilee)
Tracking because enhanced tile is a new feature.
Comment on attachment 8477782 [details] [diff] [review]
reftest fix instead?

Attachment #8477782 - Flags: review?(edilee) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify?
Depends on: 1058091
Iteration: --- → 34.3
Flags: qe-verify? → qe-verify-
No longer depends on: 1060464
No longer depends on: 1060464
Depends on: 1069776
Blocks: 1059591
No longer depends on: 1069776
Regressions: 1069776
You need to log in before you can comment on or make changes to this bug.