1127337 - Show article favicon in reader mode

Status: VERIFIED FIXED

(Firefox :: General, defect, P4)

Description

[:Margaret Leibovic]

We just need to hook up some pieces here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#52

Comment 1

[Justin Dolske [:Dolske]]

P4 based on being polish, adding an icon in-content to the reader view's header.

Comment 2

[:Margaret Leibovic]

(In reply to Justin Dolske [:Dolske] from comment #1)
> P4 based on being polish, adding an icon in-content to the reader view's
> header.

Oh, this is about showing the favicon in the toolbar, to match the URL that we're showing. We need to handle this because we're loading about:reader, which doesn't actually have a favicon, so we want to pull the favicon that's stored for the original URL.

We don't currently have any place we show an icon in the content of about:reader (we would need another bug for that).

Comment 3

[:Margaret Leibovic]

NI to dolske, since I'm not sure if this changes your prioritization reasoning.

Comment 4

[Abhinav Koppula]

Attachment: faviconReader.patch

Used PlacesUtils to obtain favicon of the article and placed the data in the messageManager.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8574284 [details] [diff] [review]
faviconReader.patch

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

Nice patch, works good :) Just some code-style things to tweak, and I want to run it by Margaret after you make your changes to make sure I didn't miss anything.

::: browser/modules/ReaderParent.jsm
@@ +55,5 @@
>          });
>          break;
>  
>        case "Reader:FaviconRequest": {
> +        // Make sure the target browser is still alive before trying to send data back.

Hm, this comment is already above. I'm not sure if it is necessary in both places, so we can remove it.

@@ +57,5 @@
>  
>        case "Reader:FaviconRequest": {
> +        // Make sure the target browser is still alive before trying to send data back.
> +        if (message.target.messageManager) {
> +          //Get faviconUrl

This comment is redundant.

@@ +59,5 @@
> +        // Make sure the target browser is still alive before trying to send data back.
> +        if (message.target.messageManager) {
> +          //Get faviconUrl
> +          let faviconUrl = PlacesUtils.promiseFaviconLinkUrl(message.data.url);
> +          Promise.resolve(faviconUrl).then(function(favicon) {

We don't need to use `Promise.resolve(promise).then()`. We can instead just use the promise.then itself.

@@ +60,5 @@
> +        if (message.target.messageManager) {
> +          //Get faviconUrl
> +          let faviconUrl = PlacesUtils.promiseFaviconLinkUrl(message.data.url);
> +          Promise.resolve(faviconUrl).then(function(favicon) {
> +            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", { url: message.data.url, faviconUrl: favicon.path.replace("favicon:","") });

Please use `favicon.path.replace(/^favicon:/, "") instead. This uses a regular-expression to only remove "favicon:" from the start of the string and in no other place.

Also, please start a new line after the opening curly bracket of the data object. The code should look something like:

> faviconUrl.then(function(favicon) {
>   message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", {
>     url: message.data.url,
>     faviconUrl: favicon.path.replace(/^favicon:/, "")
>   });
> });

Comment 6

[Abhinav Koppula]

Attachment: faviconReader.patch

Improved code style

Comment 7

[Abhinav Koppula]

Attachment: faviconReader.patch

Improved code styling and fixed indentation issue of last patch

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8574620 [details] [diff] [review]
faviconReader.patch

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

Margaret, this looks good to me. Can you take a look at this too?

Comment 9

[:Margaret Leibovic]

Comment on attachment 8574620 [details] [diff] [review]
faviconReader.patch

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

::: browser/modules/ReaderParent.jsm
@@ +11,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");

We could probably make this a lazy module getter, since we don't necessarily need this right away, but ReaderParent is loaded during startup:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#705

@@ +58,5 @@
>        case "Reader:FaviconRequest": {
> +        if (message.target.messageManager) {
> +          let faviconUrl = PlacesUtils.promiseFaviconLinkUrl(message.data.url);
> +          faviconUrl.then(function(favicon) {
> +            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", { 

Nit: Please remove trailing whitespace.

@@ +60,5 @@
> +          let faviconUrl = PlacesUtils.promiseFaviconLinkUrl(message.data.url);
> +          faviconUrl.then(function(favicon) {
> +            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", { 
> +              url: message.data.url,
> +              faviconUrl: favicon.path.replace(/^favicon:/, "") 

Same here.

Also, I trust Jared's review on whether or not this returns a favicon URL formatted the way we would want it to be.

Comment 10

[Justin Dolske [:Dolske]]

(In reply to :Margaret Leibovic from comment #2)
> (In reply to Justin Dolske [:Dolske] from comment #1)
> > P4 based on being polish, adding an icon in-content to the reader view's
> > header.
> 
> Oh, this is about showing the favicon in the toolbar, to match the URL that
> we're showing. We need to handle this because we're loading about:reader,
> which doesn't actually have a favicon, so we want to pull the favicon that's
> stored for the original URL.

Ah, we read it wrong in triage. Still a P4, since it's a polish issue that doesn't impact being able to use it, but I'm glad we're fixing it. :)

Comment 11

[Abhinav Koppula]

Attachment: faviconReader.patch

Removed trailing whitespace and lazy loading of PlacesUtils

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for checkin

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=620185398501

Comment 14

[:Margaret Leibovic]

We can also uplift this to Fx38 with a=readinglist. Let's remember to do that after this lands on m-c.

Comment 15

[Matthew N. [:MattN]]

It looks like there is are mochitest-browser-chrome test failures:

> 74 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug1124271_readerModePinnedTab.js | A promise chain failed to handle a rejection: - undefined
> 692 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_readerMode.js | A promise chain failed to handle a rejection: - undefined

https://treeherder.mozilla.org/logviewer.html#?job_id=5531144&repo=try

Comment 16

[Abhinav Koppula]

Attachment: faviconReader.patch

Report unhandled errors which prevents failing of tests.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8576140 [details] [diff] [review]
faviconReader.patch

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

::: browser/modules/ReaderParent.jsm
@@ +62,5 @@
> +            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", {
> +              url: message.data.url,
> +              faviconUrl: favicon.path.replace(/^favicon:/, "")
> +            });
> +          }).catch(Components.utils.reportError);

Nit: If this is the correct solution (jaws will review), shorten this to Cu.reportError

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8576140 [details] [diff] [review]
faviconReader.patch

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

I can't reproduce the test failures locally. Abhinav, do you know why the exception was getting thrown? The promise gets rejected if there is no favicon for the URI. It is fine here to keep the .catch(Cu.reportError) that you have added but we should also add a separate function for the rejection case.

Please also update http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1466 to provide a reason for the rejection. You can pass "favicon not found for uri" to deferred.reject().

::: browser/modules/ReaderParent.jsm
@@ +62,5 @@
> +            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", {
> +              url: message.data.url,
> +              faviconUrl: favicon.path.replace(/^favicon:/, "")
> +            });
> +          }).catch(Components.utils.reportError);

faviconUrl.then(function onResolution(favicon) {
  ...
}, function onRejection(reason) {
  Cu.reportError("Error requesting favicon URL for about:reader content: " + reason);
}).catch(Cu.reportError);

Comment 19

[Abhinav Koppula]

Attachment: faviconReader.patch

Added rejection case and catch for promise

Comment 20

[Abhinav Koppula]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Comment on attachment 8576140 [details] [diff] [review]
> faviconReader.patch
> 
> Review of attachment 8576140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't reproduce the test failures locally. Abhinav, do you know why the
> exception was getting thrown? The promise gets rejected if there is no
> favicon for the URI. It is fine here to keep the .catch(Cu.reportError) that
> you have added but we should also add a separate function for the rejection
> case.
> 
> Please also update
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> PlacesUtils.jsm#1466 to provide a reason for the rejection. You can pass
> "favicon not found for uri" to deferred.reject().
> 
> ::: browser/modules/ReaderParent.jsm
> @@ +62,5 @@
> > +            message.target.messageManager.sendAsyncMessage("Reader:FaviconReturn", {
> > +              url: message.data.url,
> > +              faviconUrl: favicon.path.replace(/^favicon:/, "")
> > +            });
> > +          }).catch(Components.utils.reportError);
> 
> faviconUrl.then(function onResolution(favicon) {
>   ...
> }, function onRejection(reason) {
>   Cu.reportError("Error requesting favicon URL for about:reader content: " +
> reason);
> }).catch(Cu.reportError);

I ran the tests in few scenarios, first I added the rejection case and then all tests passed as expected. Then I ran the tests after removing the rejection case and still the tests passed, as Jared stated. Next I ran the tests, but this time my browser was not in focus, which meant that there were timeouts that were bound to happen and hence I got above 2 mentioned failures if I didn't have the rejection case.

I saw here http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#591 and concluded that rejection of promises happen in case of timeouts and since the rejections were not handled the exceptions were raised.

Pardon me if I am completely wrong.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8578169 [details] [diff] [review]
faviconReader.patch

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

Looks good. Can you please update your patch summary to just "Bug 1127337 - Show article favicon in the browser tab in reader mode. r=jaws"? Then you can reupload and mark this for checkin-needed.

If you are using Mercurial Queues, you can run `hg qref -e` to edit the patch summary.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f5244d09ddd

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2166c79040a7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2166c79040a7

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dfd7c0ccfffd

Comment 26

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b1-build1 (20150330154247) and Aurora 39.0a2 (2015-04-02), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.