Closed Bug 1162903 Opened 9 years ago Closed 9 years ago

Switch to using favicons as the context URL preview image

Categories

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

defect
Points:
3

Tracking

(firefox40 verified, firefox41 verified)

VERIFIED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [context][UX bug])

Attachments

(2 files, 2 obsolete files)

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.
Rank: 9
Whiteboard: [context][UX bug]
Iteration: 40.3 - 11 May → 41.1 - May 25
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)
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.
Attachment #8604704 - Attachment is obsolete: true
Attachment #8605256 - Flags: review?(standard8)
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
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
Attachment #8605257 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.