Closed
Bug 1460248
Opened 6 years ago
Closed 6 years ago
Display preview image and favicon in the New Bookmark dialog
Categories
(Firefox :: Bookmarks & History, enhancement, P3)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 62
People
(Reporter: dao, Assigned: dao)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: feature)
Attachments
(3 files)
We want to display a content-provided preview image (if available) and favicon in the New Bookmark dialog. Mardak, is the preview image something we could get from or share with Activity Stream? Not quite sure what we'd display when no preview image is available.
Flags: needinfo?(edilee)
Comment 1•6 years ago
|
||
We have high res favicons (mostly apple-touch-image) in the favicon service, that is what AS uses when there's no preview image. You can point the img src to a page-icon:url and specify a size through PlacesUtils.urlWithSizeRef(). The preview image url is also available through await PlacesUtils.history.fetch(url, {"includeMeta": true}); Surely being able to reuse AS code would help, those are the things used behind the scenes though.
Assignee | ||
Comment 2•6 years ago
|
||
Ah, so those preview images and high res favicons are the same thing?
Comment 3•6 years ago
|
||
We have 2 datasets: - the preview image is an url stored in places metadata - the high res favicons are stored in the favicons service and are favicons (IIRC) larger than 96px, including apple-touch icons IIRC, AS uses the high res favicon, and if that's not available it fallbacks to the preview url, and if that's not available fallbacks to the page initials.
Updated•6 years ago
|
Flags: needinfo?(edilee) → needinfo?(andrei.br92)
Comment 4•6 years ago
|
||
If the page has been bookmarked it is very likely that we took a screenshot of the page. The screenshot can be the og:image declaration in the meta or just a screenshot of the page which you can retrieve using the PageThumb api [0]. There is also an api that you can use for the high res favicons [1]. The way AS works is: it tries to fetch a high res icon [1] and if that doesn't work it tries the preview image [0] and if that fails we just make a screenshot request for that url and cache it. Sometimes the favicon we get is not high quality enough (<96px) so we don't use it as a preview and still go on with the second step. The fallback to page initials is just a temporary step while the screenshot request is happening (its unlikely that a website you visited is no longer accessible for the screenshot request). [0] https://github.com/mozilla/activity-stream/blob/90ce2ea229d1dc661d2476c4910078d5bd530e64/system-addon/lib/Screenshots.jsm#L46-L53 [1] https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/toolkit/modules/NewTabUtils.jsm#857-867
Flags: needinfo?(andrei.br92)
Comment 5•6 years ago
|
||
If there is no image for a card this is what it should look like https://mozilla.invisionapp.com/share/HJGDPPMSE56
Comment 6•6 years ago
|
||
Currently, thumbnails of preview_image_url are expired pretty frequently unless activity stream indicates not to expire those for pages in Top Sites and Highlights. Also, activity stream only decides to get those thumbnails if Highlights decides to show them, so if the user turns off Highlights or a bookmark is no longer recent, it probably won't have a thumbnail available. Also, looking at the use case, I would think we wouldn't have a thumbnail readily available as the user is in the process of bookmarking. If the page has finished loading, most likely ContentMetaHandler would have caused the information to be saved in places via PlacesUtils.history.update: https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/browser/modules/ContentMetaHandler.jsm#22-29 https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/browser/base/content/tabbrowser.js#805-808 At that point, the new bookmarking dialog could request the thumbnail just like how activity stream does it, which is basically BackgroundPageThumbs.captureIfMissing(preview_image_url || url, {backgroundColor: GREY_10}) A general note about the meta tag is that the url can be anything, so it's not necessarily something already loaded in the page.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8985724 [details] Bug 1460248 - Display screenshot and favicon in the New Bookmark dialog. https://reviewboard.mozilla.org/r/251268/#review257698 ::: browser/base/content/browser-places.js:327 (Diff revision 1) > + faviconImage.setAttribute("iconloadingprincipal", tab.getAttribute("iconloadingprincipal")); > + faviconImage.setAttribute("src", tab.getAttribute("image")); > + } > + > + let canvas = PageThumbs.createCanvas(window); > + PageThumbs.captureToCanvas(gBrowser.selectedBrowser, canvas); I think this code provides cached screenshots for activity stream: https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/HighlightsFeed.jsm#221-227
Comment 9•6 years ago
|
||
Looks like the thumbnail is stretching the dialog far more than expected, see this screenshot. We should probably restrict its width (ideally a panel with or without sshot should have the same width).
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8985724 [details] Bug 1460248 - Display screenshot and favicon in the New Bookmark dialog. https://reviewboard.mozilla.org/r/251268/#review257902 There's some sizing problem, looks like. ::: browser/base/content/browser-places.js:327 (Diff revision 1) > + faviconImage.setAttribute("iconloadingprincipal", tab.getAttribute("iconloadingprincipal")); > + faviconImage.setAttribute("src", tab.getAttribute("image")); > + } > + > + let canvas = PageThumbs.createCanvas(window); > + PageThumbs.captureToCanvas(gBrowser.selectedBrowser, canvas); I'm not an expert of our thumbs code, thus I'm not sure what the Screenshots module would provide us on top of PageThumbs. Off-hand it looks like it allows to fetch anonymous screenshots of pages not currently open, and cache them. We likely don't need that here. I understand why they do that, but it doesn't seem critical for this dialog, since it's not open as often as the new tab page, and always with the page at hand. We can always improve it in the future if we see a necessity.
Attachment #8985724 -
Flags: review?(mak77) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8985724 [details] Bug 1460248 - Display screenshot and favicon in the New Bookmark dialog. https://reviewboard.mozilla.org/r/251268/#review258130
Attachment #8985724 -
Flags: review?(mak77) → review+
Comment 14•6 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dbff3bada5fd Display screenshot and favicon in the New Bookmark dialog. r=mak
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dbff3bada5fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment 16•6 years ago
|
||
Adding this to release notes for 62 beta, as "Display preview image and favicon in the New Bookmark dialog"
relnote-firefox:
--- → 62+
Comment 17•6 years ago
|
||
I have reproduced this bug with Nightly 62.0a1 (2018-05-10) on Windows 10, 64 Bit! This bug's fix is verified with latest Beta! Build ID 20180702164905 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
QA Whiteboard: [bugday-20180627]
Updated•6 years ago
|
QA Whiteboard: [bugday-20180627] → [good first verify][bugday-20180627]
Comment 18•6 years ago
|
||
What is the advantage of showing a small screenshot of the page the user is already looking at on top of that page?
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Lukas Waymann from comment #18) > What is the advantage of showing a small screenshot of the page the user is > already looking at on top of that page? A resolved bug isn't the best place to discuss these kind of things. Please consider posting to firefox-dev (https://mail.mozilla.org/listinfo/firefox-dev) or asking in the #ux irc channel.
Comment 21•5 years ago
|
||
See https://bugzilla.mozilla.org/show_bug.cgi?id=1506462
You need to log in
before you can comment on or make changes to this bug.
Description
•