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)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
relnote-firefox --- 62+
firefox62 --- verified

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)
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.
Ah, so those preview images and high res favicons are the same thing?
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.
Flags: needinfo?(edilee) → needinfo?(andrei.br92)
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)
If there is no image for a card this is what it should look like https://mozilla.invisionapp.com/share/HJGDPPMSE56
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: nobody → dao+bmo
Status: NEW → ASSIGNED
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
Attached image sshot 1
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 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 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+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbff3bada5fd
Display screenshot and favicon in the New Bookmark dialog. r=mak
https://hg.mozilla.org/mozilla-central/rev/dbff3bada5fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Keywords: feature
Depends on: 1470393
Adding this to release notes for 62 beta,  as "Display preview image and favicon in the New Bookmark dialog"
Blocks: 1472472
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]
QA Whiteboard: [bugday-20180627] → [good first verify][bugday-20180627]
What is the advantage of showing a small screenshot of the page the user is already looking at on top of that page?
(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.
Depends on: 1492235
Depends on: 1505184
Keywords: uiwanted
Thank you,  Mohammad Maruf Rahman!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: