Closed Bug 975210 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: jboriss, Assigned: mzhilyaev)

References

()

Details

(Whiteboard: [tiles] p=3 s=it-31c-30a-29b.2 [qa!])

Attachments

(2 files, 2 obsolete files)

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
Blocks: 975228, 975236, 975211
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.
Summary: Augment thumbnail-fetch service to allow for Sponsored Tiles images & text → Augment Site._render logic to allow for Sponsored Tiles images & text
No longer blocks: 975211
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? )
No longer blocks: 975228
Blocks: tiles-dev
Depends on: 975228
Whiteboard: [tiles] → [tiles] p=0
(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
Assignee: edilee → mzhilyaev
Attached image 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.
Depends on: 978338
Attached patch v1 (obsolete) — Splinter Review
Attachment #8391050 - Flags: review?(adw)
Depends on: 974736
No longer depends on: 975228, 978338
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.
Attachment #8391050 - Flags: review?(adw) → review+
(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"?
Attached patch for checkin (obsolete) — Splinter Review
Attachment #8391050 - Attachment is obsolete: true
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.
Attached patch for checkinSplinter Review
Attachment #8395251 - Attachment is obsolete: true
No longer blocks: fxdesktoptriage
https://hg.mozilla.org/mozilla-central/rev/6010065255d6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Whiteboard: [tiles] p=0 → [tiles] p=0 s=it-31c-30a-29b.2
Whiteboard: [tiles] p=0 s=it-31c-30a-29b.2 → [tiles] p=3 s=it-31c-30a-29b.2
Flags: in-testsuite?
QA Contact: cornel.ionce
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.2 → [tiles] p=3 s=it-31c-30a-29b.2 [qa+]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
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.
Status: RESOLVED → VERIFIED
Whiteboard: [tiles] p=3 s=it-31c-30a-29b.2 [qa+] → [tiles] p=3 s=it-31c-30a-29b.2 [qa!]
You need to log in before you can comment on or make changes to this bug.