Closed
Bug 1042876
Opened 10 years ago
Closed 10 years ago
Update newtab endpoints to new v2/links
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
People
(Reporter: Mardak, Assigned: Mardak)
References
()
Details
Attachments
(2 files, 4 obsolete files)
204.63 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
Mardak
:
review+
|
Details | Diff | Splinter Review |
We'll want to update both click and links:
browser.newtabpage.directory.source
https://tiles.up.mozillalabs.com/v1/links/fetch
browser.newtabpage.directory.reportClickEndPoint
https://tiles.up.mozillalabs.com/v1/links/click
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
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 | automation.py | application crashed [@ nsSocketTransport::InitiateSocket()]
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
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?
[1] http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js
Assignee | ||
Comment 4•10 years ago
|
||
(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 ?
Comment 5•10 years ago
|
||
How could I forget. Sounds fine to me.
Assignee | ||
Comment 6•10 years ago
|
||
oyiptong, the current urls I have in patches:
https://tiles.up.mozillalabs.com/v1/links/fetch
https://tiles.up.mozillalabs.com/v1/links/view
https://tiles.up.mozillalabs.com/v1/links/click
You mentioned bumping them to v2 elsewhere. Should I do that and keep the other parts of the urls the same?
Flags: needinfo?(oyiptong)
Assignee | ||
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
I note comment 7 as currently v1 returns 400 bad request for those without directoryCount.
Assignee | ||
Comment 9•10 years ago
|
||
wip pending link confirmation from oyiptong
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•10 years ago
|
||
Notes: could also remove toolkit/content/directoryLinks.json
Comment 11•10 years ago
|
||
The new urls will be the same, save for the version denominator.
They will be:
https://tiles.up.mozillalabs.com/v2/links/fetch
https://tiles.up.mozillalabs.com/v2/links/view
https://tiles.up.mozillalabs.com/v2/links/click
Flags: needinfo?(oyiptong)
Assignee | ||
Updated•10 years ago
|
Summary: Update newtab endpoints to new v1/links → Update newtab endpoints to new v2/links
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8472752 -
Attachment is obsolete: true
Attachment #8473730 -
Flags: review?(adw)
Comment 13•10 years ago
|
||
Comment on attachment 8473730 [details] [diff] [review]
v1
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.
Assignee | ||
Comment 14•10 years ago
|
||
Oh duh. I even left comment 10 to remind myself.
Assignee | ||
Comment 15•10 years ago
|
||
Bug 1042214 has changes to turn off remote connections for tests.
URL: 1042214
Assignee | ||
Comment 16•10 years ago
|
||
test_DirectoryLinksProvider ran fine without the file.
Attachment #8473730 -
Attachment is obsolete: true
Attachment #8473730 -
Flags: review?(adw)
Attachment #8474024 -
Flags: review?(adw)
Comment 17•10 years ago
|
||
Comment on attachment 8474024 [details] [diff] [review]
v2
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+
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
testing/profiles/prefs_general.js wasn't enough
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8477777 -
Attachment is obsolete: true
Attachment #8477781 -
Attachment is obsolete: true
Attachment #8477782 -
Flags: review?(edilee)
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox34:
--- → affected
Comment 22•10 years ago
|
||
Tracking because enhanced tile is a new feature.
tracking-firefox33:
--- → +
tracking-firefox34:
--- → +
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8477782 [details] [diff] [review]
reftest fix instead?
r=froydnj
https://hg.mozilla.org/integration/fx-team/rev/d5cb280f36bf
Attachment #8477782 -
Flags: review?(edilee) → review+
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d76f7a57af61
https://hg.mozilla.org/mozilla-central/rev/d5cb280f36bf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Flags: qe-verify?
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 25•10 years ago
|
||
Updated•10 years ago
|
Iteration: --- → 34.3
Updated•10 years ago
|
Flags: qe-verify? → qe-verify-
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•