Update newtab endpoints to new v2/links

RESOLVED FIXED in Firefox 33

Status

()

Firefox
New Tab Page
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Mardak, Assigned: Mardak)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 34
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox33+ fixed, firefox34+ fixed)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Depends on: 1043687
(Assignee)

Comment 1

3 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

3 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

3 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

3 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

3 years ago
How could I forget.  Sounds fine to me.
Depends on: 1050328
(Assignee)

Comment 6

3 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

3 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

3 years ago
I note comment 7 as currently v1 returns 400 bad request for those without directoryCount.
(Assignee)

Comment 9

3 years ago
Created attachment 8472752 [details] [diff] [review]
wip

wip pending link confirmation from oyiptong
Assignee: nobody → edilee
Status: NEW → ASSIGNED
(Assignee)

Comment 10

3 years ago
Notes: could also remove toolkit/content/directoryLinks.json
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

3 years ago
Depends on: 1054341
(Assignee)

Updated

3 years ago
Summary: Update newtab endpoints to new v1/links → Update newtab endpoints to new v2/links
(Assignee)

Comment 12

3 years ago
Created attachment 8473730 [details] [diff] [review]
v1
Attachment #8472752 - Attachment is obsolete: true
Attachment #8473730 - Flags: review?(adw)

Comment 13

3 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

3 years ago
Oh duh. I even left comment 10 to remind myself.
(Assignee)

Comment 15

3 years ago
Bug 1042214 has changes to turn off remote connections for tests.
URL: 1042214
(Assignee)

Comment 16

3 years ago
Created attachment 8474024 [details] [diff] [review]
v2

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

3 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)

Updated

3 years ago
Blocks: 1057602
(Assignee)

Comment 18

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/d76f7a57af61
(Assignee)

Comment 19

3 years ago
Created attachment 8477777 [details] [diff] [review]
fix reftest burning ?

testing/profiles/prefs_general.js wasn't enough
(Assignee)

Comment 20

3 years ago
Created attachment 8477781 [details] [diff] [review]
reftest fix instead?
(Assignee)

Comment 21

3 years ago
Created attachment 8477782 [details] [diff] [review]
reftest fix instead?
Attachment #8477777 - Attachment is obsolete: true
Attachment #8477781 - Attachment is obsolete: true
Attachment #8477782 - Flags: review?(edilee)
status-firefox33: --- → affected
status-firefox34: --- → affected
Tracking because enhanced tile is a new feature.
tracking-firefox33: --- → +
tracking-firefox34: --- → +
(Assignee)

Comment 23

3 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+
https://hg.mozilla.org/mozilla-central/rev/d76f7a57af61
https://hg.mozilla.org/mozilla-central/rev/d5cb280f36bf
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

3 years ago
Flags: qe-verify?
(Assignee)

Updated

3 years ago
Depends on: 1058091
(Assignee)

Updated

3 years ago
status-firefox33: affected → fixed
(Assignee)

Comment 25

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe5ffd28b29a

Updated

3 years ago
Iteration: --- → 34.3
Flags: qe-verify? → qe-verify-

Updated

3 years ago
Depends on: 1060464
No longer depends on: 1060464

Updated

3 years ago
Depends on: 1060464

Updated

3 years ago
No longer depends on: 1060464

Updated

3 years ago
Depends on: 1069776
(Assignee)

Updated

3 years ago
Blocks: 1059591
You need to log in before you can comment on or make changes to this bug.