Switch to using favicons as the context URL preview image

VERIFIED FIXED in Firefox 40

Status

Hello (Loop)
Client
P1
normal
Rank:
9
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

unspecified
mozilla41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox40 verified, firefox41 verified)

Details

(Whiteboard: [context][UX bug])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 3
Priority: -- → P1
Flags: qe-verify+
Flags: firefox-backlog+
Blocks: 1162905
Blocks: 1162909
Depends on: 1153800
Note: I was originally intending bug 1153800 would switch to favicons. Whatever we do, we should switch to data uris at the same time.

Updated

3 years ago
Rank: 9
Whiteboard: [context][UX bug]

Updated

3 years ago
Iteration: 40.3 - 11 May → 41.1 - May 25
Created attachment 8604704 [details] [diff] [review]
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.
Attachment #8604704 - Flags: review?(standard8)
Created attachment 8605175 [details] [diff] [review]
Patch 2: add unit test coverage for favicons use
Attachment #8605175 - Flags: review?(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/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 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.
Attachment #8604704 - Flags: review?(standard8) → review-
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
Attachment #8605175 - Flags: review?(standard8) → review-
(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.
Created attachment 8605256 [details] [diff] [review]
Patch 1.1: use favicons as context thumbmnails
Attachment #8604704 - Attachment is obsolete: true
Attachment #8605256 - Flags: review?(standard8)
Created attachment 8605257 [details] [diff] [review]
Patch 2.1: add unit test coverage for favicons use
Attachment #8605175 - Attachment is obsolete: true
Attachment #8605257 - Flags: review?(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
Attachment #8605257 - Flags: review?(standard8) → review+
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
Attachment #8605256 - Flags: review?(standard8) → review+
Blocks: 1164510

Comment 12

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/e256f9750d7f
https://hg.mozilla.org/integration/fx-team/rev/62331f536d88
https://hg.mozilla.org/mozilla-central/rev/e256f9750d7f
https://hg.mozilla.org/mozilla-central/rev/62331f536d88
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
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
Attachment #8605256 - Flags: approval-mozilla-aurora?
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
Attachment #8605257 - Flags: approval-mozilla-aurora?
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.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
status-firefox40: --- → affected
Attachment #8605257 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6f750ed544b
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3e39e6d51d7
status-firefox40: affected → fixed
Flags: in-testsuite+
Attachment #8605256 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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).
status-firefox40: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.