975210 - Augment Site._render logic to allow for Sponsored Tiles images & text

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

New Tab's current thumbnail service fetches tiles only from background tab thumbnailing.  Fetching should be augmented to handle three different sources:

1. Background tab screenshots (current state)
2. Partner/Sponsored tile images and text (possibly background color also)
3. Mozilla internal tile images and text

Comment 1

[Dão Gottwald [:dao]]

Why should the thumbnail service know about this? Seems like it shouldn't be involved for preset entries.

Comment 2

[Ed Lee :Mardak]

(In reply to Dão Gottwald [:dao] from comment #1)
> Why should the thumbnail service know about this?
Nod. The thinking is either we pass in enough metadata from the data sources to trigger a different codepath for read the image from the metadata instead of calling PageThumbs.getThumbnailURL for Directory Tiles or change things around for regular page thumbnails as well so the metadata includes an already-resolved PageThumbs.getThumbnailURL and the Site._render logic is just to take this.link.imageUrl.

Comment 3

[John Hesling [:John99] (NeedInfo me)]

Sorry if this is slightly off topic. Could you point me at the discussion/mailing list about this and related matters.

I take it from the blog comment
> More Details on Directory Tiles [ https://blog.mozilla.org/advancingcontent/ ] 
> Our frecency algorithm takes about 30 days of normal browsing behavior to update Tiles.
>At that point the user will start seeing content that reflects the sites they’ve recently and frequently visited.

That the frecency behaviour is being modified to give precedence to sponsored sites over current frecency behaviour. Current behaviour

Comment 4

[John Hesling [:John99] (NeedInfo me)]

Continued from comment 3 

Current behaviour is able to very quickly populate the full set of nine tiles. New blank tiles can be populated in seconds plus the restart time. Not the  30 days indicated in the Blog. 
(There are suggestions the 30 days is just a wildly conservative figure.)  
Support contributors are discussing this in a thread https://support.mozilla.org/en-US/forums/contributors/709999.

(Sorry about the double post. Some keyboard combination obviously submits comments. Not sure if that is a change in bugzilla behaviour, or what key combination I need to avoid? )

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to John Hesling [:John99] from comment #4)
> Not the  30 days indicated in the Blog. 
> (There are suggestions the 30 days is just a wildly conservative figure.)  

Sorry, I know that's what the blog post mentioned and I'm not entirely sure where the 30 day number came from but that's not the plan going forward. See bug 975228 comment 4

Comment 6

[Ed Lee :Mardak]

Attachment: wip v1

The tiles with imageUri should probably have styling something like:

background-position: top center
background-size: auto

Note, currently .newtab-thumbnail is behind the title text (slightly transparent) while the new design from bug 978338 has the title below.

Comment 7

[Ed Lee :Mardak]

Attachment: v1

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8391050 [details] [diff] [review]
v1

Review of attachment 8391050 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/newtab/sites.js
@@ +142,5 @@
>     * Captures the site's thumbnail in the background, but only if there's no
>     * existing thumbnail and the page allows background captures.
>     */
>    captureIfMissing: function Site_captureIfMissing() {
> +    if (gPage.allowBackgroundCaptures && !this.link.imageURI) {

Please call it imageURL instead of imageURI.  Usually we use URI to mean an nsIURI object.  If these things really are general URIs and not URLs in specific, then please call it imageURISpec instead.  I don't see imageURI actually defined in any of these patches, so does it come from the JSON?

@@ +155,3 @@
>      let thumbnail = this._querySelector(".newtab-thumbnail");
> +    thumbnail.style.backgroundColor = this.link.bgColor;
> +    let uri = this.link.imageURI || PageThumbs.getThumbnailURL(this.url);

Here too.

Comment 9

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #8)
> does it come from the JSON?
Yes, see attachment 8393905 [details] [diff] [review].

"imageURI": "data:image/jpg;base64,/9j/4AAQ...

> Please call it imageURL instead of imageURI... please call it imageURISpec instead
We're using data URI specs, so it seems that we should call it imageURISpec.

Comment 10

[Ed Lee :Mardak]

(In reply to Drew Willcoxon :adw from comment #8)
> > +    let uri = this.link.imageURI || PageThumbs.getThumbnailURL(this.url);
> Here too.
Now that we're using this.link.imageURISpec, do you want the variable to be "spec" instead of "uri"?

Comment 11

[Ed Lee :Mardak]

Attachment: for checkin

Comment 12

[Drew Willcoxon :adw]

No, that's fine for a local variable.  I just wanted to keep to the convention for an object member that's part of an interface.

Comment 13

[Ed Lee :Mardak]

Comment on attachment 8395251 [details] [diff] [review]
for checkin

>+.newtab-site[type=sponsored] .newtab-thumbnail {
>+  background-position: top center;
>+  background-size: auto;
adw, I just realized we'll need to update these when the .json actually has type=affiliate or type=organic. We can update these now to have those 2 additional css selectors, or we can check .newtab-site:not([type=history]) ?

Comment 14

[Drew Willcoxon :adw]

(In reply to Ed Lee :Mardak from comment #13)
> We can update these now to have those 2 additional css selectors

I'd prefer this so that if we add new kinds of tiles in the future, or if some add-on does, the selector won't apply to them.

Comment 15

[Ed Lee :Mardak]

Attachment: for checkin

Comment 16

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/208ba43a7098

Comment 17

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/4caa44af6d1b, see bug 975228 comment 30.

Comment 18

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/6010065255d6

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6010065255d6

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

If the user already had a partner website in his browsing history with a history thumbnail, the sponsored icon isn't shown. If using a new profile it will be shown for the predefined Sponsored Tiles.

Is this behaviour intended?

Comment 21

[Ed Lee :Mardak]

(In reply to Cornel Ionce [QA] from comment #20)
> If the user already had a partner website in his browsing history with a
> history thumbnail, the sponsored icon isn't shown.
That's the intended behavior. If it's a "history tile" which shows a thumbnail, it's not the sponsored image/logo being display, so it shouldn't have the sponsored icon.

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

Thanks, no other issues found while testing this. Marking verified.