1162903 - Switch to using favicons as the context URL preview image

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

Right now we fetch a preview image from the website's content (metadata), but that doesn't always get us a pretty result.
To improve the layout and provide a consistent UX for all pages that are attached as conversation context, we should use the favicon as the preview.

Comment 1

[Mark Banner (:standard8)]

Note: I was originally intending bug 1153800 would switch to favicons. Whatever we do, we should switch to data uris at the same time.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: use favicons as context thumbmnails

Ok, I had to jump through a few hoops here, but this is essentially what needs to be done to get serialized icons to be displayed and stored.
Next up is unit tests updates.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: add unit test coverage for favicons use

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8605175 [details] [diff] [review]
Patch 2: add unit test coverage for favicons use

Review of attachment 8605175 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/test/mochitest/browser_mozLoop_context.js
@@ +16,5 @@
> +add_task(function* test_mozLoop_getSelectedTabMetadata() {
> +  Assert.ok(gMozLoopAPI, "mozLoop should exist");
> +
> +  let metadata = yield promiseGetMetadata();
> +  dump("META:: " + JSON.stringify(metadata, null, 2) + "\n");

left-over, removed locally.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8604704 [details] [diff] [review]
Patch 1: use favicons as context thumbmnails

Review of attachment 8604704 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is almost there, kinda a soft r- due to the fallback issue.

::: browser/components/loop/content/js/panel.jsx
@@ +705,5 @@
>      },
>  
>      onDocumentVisible: function() {
>        this.props.mozLoop.getSelectedTabMetadata(function callback(metadata) {
> +        var previewImage = metadata.favicon || (metadata.previews.length ? metadata.previews[0] : "");

I don't think we should keep the fallback to the previews. This is probably going to cause issues, with what gets displayed (e.g. try it on bugs ahoy). This will need changing in roomViews as well.

If we need a fallback default, then we should ask Sevaan for one. I'm quite happy for that to be a separate bug though.

::: browser/components/loop/modules/MozLoopAPI.jsm
@@ +905,5 @@
>            win.messageManager.removeMessageListener("PageMetadata:PageDataResult", onPageDataResult);
>            let pageData = msg.json;
> +          win.LoopUI.getFavicon(function(err, favicon) {
> +            if (err) {
> +              MozLoopService.log.error("Error occurred whilst fetching favicon", err);

Please can you add a comment after this line, that this is an intentional fall-through so that the callback happens.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8605175 [details] [diff] [review]
Patch 2: add unit test coverage for favicons use

Review of attachment 8605175 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +888,5 @@
>        var contextHostname = view.getDOMNode().querySelector(".context-url");
>        expect(contextHostname.textContent).eql("www.example.com");
>      });
> +
> +    it("should show the favicon when available", function() {

We need a similar test/change for the roomViews change.

::: browser/components/loop/test/mochitest/browser_mozLoop_context.js
@@ +18,5 @@
> +
> +  let metadata = yield promiseGetMetadata();
> +  dump("META:: " + JSON.stringify(metadata, null, 2) + "\n");
> +  Assert.strictEqual(metadata.url, null, "URL should be empty for about:blank");
> +  Assert.strictEqual(metadata.favicon, null, "Favicon should be empty for about:blank");

I'm seeing this fail when I'm running it locally with:

1212 INFO TEST-UNEXPECTED-FAIL | browser/components/loop/test/mochitest/browser_mozLoop_context.js | Favicon should be empty for about:blank - "undefined" === null - JS frame :: chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_mozLoop_context.js :: test_mozLoop_getSelectedTabMetadata :: line 22

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #6)
> I'm seeing this fail when I'm running it locally with:
> 
> 1212 INFO TEST-UNEXPECTED-FAIL |
> browser/components/loop/test/mochitest/browser_mozLoop_context.js | Favicon
> should be empty for about:blank - "undefined" === null - JS frame ::
> chrome://mochitests/content/browser/browser/components/loop/test/mochitest/
> browser_mozLoop_context.js :: test_mozLoop_getSelectedTabMetadata :: line 22

Yeah, this needs a fix in patch 1 that I have locally.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: use favicons as context thumbmnails

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: add unit test coverage for favicons use

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8605257 [details] [diff] [review]
Patch 2.1: add unit test coverage for favicons use

Review of attachment 8605257 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=Standard8

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8605256 [details] [diff] [review]
Patch 1.1: use favicons as context thumbmnails

Review of attachment 8605256 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=Standard8

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e256f9750d7f
https://hg.mozilla.org/integration/fx-team/rev/62331f536d88

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e256f9750d7f
https://hg.mozilla.org/mozilla-central/rev/62331f536d88

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8605256 [details] [diff] [review]
Patch 1.1: use favicons as context thumbmnails

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Hello
[User impact if declined]: We'll be relying on pointing to images on the website which can bad layout and confusing indicators.
[Describe test coverage new/current, TreeHerder]: Has unit tests in the part 2 patch.
[Risks and why]: Minor, only affects loop specific code
[String/UUID change made/needed]: None

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8605257 [details] [diff] [review]
Patch 2.1: add unit test coverage for favicons use

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Hello
[User impact if declined]: We'll be relying on pointing to images on the website which can bad layout and confusing indicators.
[Describe test coverage new/current, TreeHerder]: Has unit tests in the part 2 patch.
[Risks and why]: Minor, only affects loop specific code
[String/UUID change made/needed]: None

Comment 16

[Bogdan Maris, Desktop Test Engineering]

UX looks fine, as shown in mockup, using latest Nightly, across platforms (Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5). Will mark this bug as verified fixed since it reached milestone but keeping qe-verify+ to retest in Aurora.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6f750ed544b
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3e39e6d51d7

Comment 18

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #16)
> UX looks fine, as shown in mockup, using latest Nightly, across platforms
> (Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5). Will mark this
> bug as verified fixed since it reached milestone but keeping qe-verify+ to
> retest in Aurora.

Also verified using latest Aurora across platforms (Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5).