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

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

(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.

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

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

(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

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 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.

(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.

(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"?

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 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]) ?

(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.

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

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?

(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.

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