Closed
Bug 975210
Opened 11 years ago
Closed 11 years ago
Augment Site._render logic to allow for Sponsored Tiles images & text
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
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)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
Comment 1•11 years ago
|
||
Why should the thumbnail service know about this? Seems like it shouldn't be involved for preset entries.
Comment 2•11 years ago
|
||
(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
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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? )
Updated•11 years ago
|
Whiteboard: [tiles] → [tiles] p=0
Comment 5•11 years ago
|
||
(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
Updated•11 years ago
|
Assignee: edilee → mzhilyaev
Comment 6•11 years ago
|
||
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.
Updated•11 years ago
|
Updated•11 years ago
|
Comment 8•11 years ago
|
||
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+
Comment 9•11 years ago
|
||
(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•11 years ago
|
||
(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 12•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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.
Updated•11 years ago
|
Blocks: fxdesktoptriage
Comment 17•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/fx-team/rev/4caa44af6d1b, see bug 975228 comment 30.
Updated•11 years ago
|
No longer blocks: fxdesktoptriage
https://hg.mozilla.org/mozilla-central/rev/6010065255d6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•11 years ago
|
Whiteboard: [tiles] p=0 → [tiles] p=0 s=it-31c-30a-29b.2
Updated•11 years ago
|
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+]
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Comment 20•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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!]
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•