Replace default tiles for the funnelcake in feb 2017

VERIFIED FIXED in Firefox 51

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox51 verified, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: pref: browser.newtabpage.directory.source = "chrome://browser/content/newtab/alternativeDefaultSites.json")

Attachments

(1 attachment)

Assignee

Comment 1

3 years ago
Verdi, can you sanity-check the list in comment #0?


There's two basic ways we can try to get this done:

1) hardcode a list somewhere and override the results from the directorylinksprovider (which comes from the tiles server) based on a pref.
2) send funnelcake info to the directorylinksprovider and teach the directorylinksprovider service to be able to have a separate set of results for the funnelcake. Right now it uses update channels (client-side, we use UpdateUtils.UpdateChannel), listed here: https://github.com/mozilla/onyx/blob/master/onyx/api/v3.py#L17 .


I'm leaning towards (1) in order to minimize the number of dependencies and therefore risk, given that we want to have this done by the first week of January. On the flip side, (2) would give us more flexibility in the future. On the flip side of that, AFAICT the tiles service is not very useful anymore these days (we still fetch tiles once a day but we never send 'enhanced' tiles and so for mature profiles we're basically making 1 useless https request a day to fetch this data we never use) and so maybe we should think about removing that code and shutting it down, which makes (2) a lot less attractive. Dolske, thoughts?
Flags: needinfo?(mverdi)
Flags: needinfo?(dolske)
(In reply to :Gijs Kruitbosch from comment #1)
> Verdi, can you sanity-check the list in comment #0?

That's the list. Looks good.
Flags: needinfo?(mverdi)
(In reply to :Gijs Kruitbosch from comment #1)

> There's two basic ways we can try to get this done:
> 
> 1) hardcode a list somewhere and override the results from the
> directorylinksprovider (which comes from the tiles server) based on a pref.
> 2) send funnelcake info to the directorylinksprovider and teach the
> directorylinksprovider service to be able to have a separate set of results
> for the funnelcake. Right now it uses update channels (client-side, we use
> UpdateUtils.UpdateChannel), listed here:
> https://github.com/mozilla/onyx/blob/master/onyx/api/v3.py#L17 .
> 
> 
> I'm leaning towards (1) in order to minimize the number of dependencies and
> therefore risk

Simplicity seems best to me.
Flags: needinfo?(dolske)
Assignee

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 5

3 years ago
I've added a screenshot on mozreview, Michael, to make it easy to review.

To run with these changes, we'd need the following pref change:

 // directory tiles download URL
-pref("browser.newtabpage.directory.source", "https://tiles.services.mozilla.com/v3/links/fetch/%LOCALE%/%CHANNEL%");
+pref("browser.newtabpage.directory.source", "chrome://browser/content/newtab/alternativeDefaultSites.json");

Open question: the directoryId values. I don't know if I'm allowed to just make those up, or how else I would make that work. Marina or Tim, do you know?
Flags: needinfo?(tspurway)
Flags: needinfo?(msamuel)
Assignee

Comment 6

3 years ago
(oh, and just so we're clear, this is without any further styling changes to about:newtab, this is just the different about:newtab URLs, but I added metadata rather than having about:newtab start requesting all these sites on startup to get thumbnails...)

Comment 7

3 years ago
mozreview-review
Comment on attachment 8821502 [details]
Bug 1322731 - allow using different default tiles,

https://reviewboard.mozilla.org/r/100792/#review101332

Comment 8

3 years ago
mozreview-review
Comment on attachment 8821502 [details]
Bug 1322731 - allow using different default tiles,

https://reviewboard.mozilla.org/r/100794/#review101334
Attachment #8821502 - Flags: review?(mverdi) → review+

Comment 9

3 years ago
mozreview-review
Comment on attachment 8821502 [details]
Bug 1322731 - allow using different default tiles,

https://reviewboard.mozilla.org/r/100794/#review101484

Will we need to add extra instrumentation to evaluate the usage of these tiles or will this happen automatically through existing mechanisms?
Attachment #8821502 - Flags: review?(dao+bmo) → review+
Assignee

Comment 10

3 years ago
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 8821502 [details]
> Bug 1322731 - allow using different default tiles,
> 
> https://reviewboard.mozilla.org/r/100794/#review101484
> 
> Will we need to add extra instrumentation to evaluate the usage of these
> tiles or will this happen automatically through existing mechanisms?

The directoryIds will be used for reporting, AIUI, but as noted in comment #5 I don't know if the ones I specified work or if we need server changes to make them work. I'm kind of assuming that nothing disastrous will happen if we submit stuff to the tiles server with 'invalid' IDs because for obvious security reasons it should already be resilient to 'bogus' stuff being submitted... Depending on what Marina and Tim tell us about this we may want to add telemetry or other mechanisms of our own, though I would prefer to use the existing one if possible.
Blocks: 1322737
Whiteboard: pref: browser.newtabpage.directory.source = "chrome://browser/content/newtab/alternativeDefaultSites.json"
Assignee

Comment 11

3 years ago
Talked to Marina and Nan Jiang on IRC, and this should be good to go.

In principle there should be data available through the "Tiles" dataset on sql.tmo.
Flags: needinfo?(tspurway)
Flags: needinfo?(msamuel)

Comment 12

3 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cdcf8d6a9572
allow using different default tiles, r=dao,verdi

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cdcf8d6a9572
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee

Comment 14

3 years ago
Comment on attachment 8821502 [details]
Bug 1322731 - allow using different default tiles,

Approval Request Comment
[Feature/Bug causing the regression]: funnelcake for bug 1322718
[User impact if declined]: impact to the funnelcake
[Is this code covered by automated tests?]: the new tab page is covered by automated tests, but this change is behind a pref.
[Has the fix been verified in Nightly?]: no, because it's behind a pref
[Needs manual test from QE? If yes, steps to reproduce]: yes, but we'll need a custom build. Verification is easy: on the custom build, on a clean profile with no imports from another browser (e.g. because Firefox was the default) we'll show different default tiles (see comment #0).
[List of other uplifts needed for the feature/fix]: the other deps for bug 1322718 are related, but not strictly necessary.
[Is the change risky?]: No.
[Why is the change risky/not risky?]:  it's completely behind a pref so will only affect the funnelcake. We'll ship an extra file, which is JSON and should be highly compressible, so the only minor impact to release is potentially a few byte increase in filesize
[String changes made/needed]: No.
Attachment #8821502 - Flags: approval-mozilla-beta?
Attachment #8821502 - Flags: approval-mozilla-aurora?
Comment on attachment 8821502 [details]
Bug 1322731 - allow using different default tiles,

OK to uplift, this might not make it to beta 12 - if not, beta 13 next mon/tues.
Attachment #8821502 - Flags: approval-mozilla-beta?
Attachment #8821502 - Flags: approval-mozilla-beta+
Attachment #8821502 - Flags: approval-mozilla-aurora?
Attachment #8821502 - Flags: approval-mozilla-aurora+
No longer blocks: 1322737
Setting the flag for verification, description available in Comment 0 and Comment 5.
Flags: qe-verify+
Flags: needinfo?(gwimberly)

Updated

3 years ago
Flags: needinfo?(gwimberly)
Depends on: 1330611

Updated

2 years ago
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.