Closed Bug 1127337 Opened 5 years ago Closed 5 years ago

Show article favicon in reader mode

Categories

(Firefox :: General, defect, P4)

defect

Tracking

()

VERIFIED FIXED
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Margaret, Assigned: abhinav.koppula, Mentored)

References

Details

Attachments

(1 file, 7 obsolete files)

Flags: firefox-backlog+
Assignee: nobody → abhinav.koppula
Mentor: jaws, margaret.leibovic
Status: NEW → ASSIGNED
P4 based on being polish, adding an icon in-content to the reader view's header.
Priority: -- → P4
(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).
NI to dolske, since I'm not sure if this changes your prioritization reasoning.
Flags: needinfo?(dolske)
Attached patch faviconReader.patch (obsolete) — Splinter Review
Used PlacesUtils to obtain favicon of the article and placed the data in the messageManager.
Attachment #8574284 - Flags: review?(jaws)
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:/, "")
>   });
> });
Attachment #8574284 - Flags: review?(jaws) → feedback+
Attached patch faviconReader.patch (obsolete) — Splinter Review
Improved code style
Attachment #8574284 - Attachment is obsolete: true
Attachment #8574614 - Flags: review?(jaws)
Attached patch faviconReader.patch (obsolete) — Splinter Review
Improved code styling and fixed indentation issue of last patch
Attachment #8574614 - Attachment is obsolete: true
Attachment #8574614 - Flags: review?(jaws)
Attachment #8574620 - Flags: review?(jaws)
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?
Attachment #8574620 - Flags: review?(margaret.leibovic)
Attachment #8574620 - Flags: review?(jaws)
Attachment #8574620 - Flags: review+
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.
Attachment #8574620 - Flags: review?(margaret.leibovic) → review+
(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. :)
Flags: needinfo?(dolske)
Attached patch faviconReader.patch (obsolete) — Splinter Review
Removed trailing whitespace and lazy loading of PlacesUtils
Attachment #8574620 - Attachment is obsolete: true
Attachment #8575404 - Flags: review?(margaret.leibovic)
Attachment #8575404 - Flags: review?(margaret.leibovic) → review+
Attached patch Patch for checkin (obsolete) — Splinter Review
Attachment #8575404 - Attachment is obsolete: true
Attachment #8575422 - Flags: review+
We can also uplift this to Fx38 with a=readinglist. Let's remember to do that after this lands on m-c.
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
Flags: needinfo?(abhinav.koppula)
Keywords: checkin-needed
Attached patch faviconReader.patch (obsolete) — Splinter Review
Report unhandled errors which prevents failing of tests.
Flags: needinfo?(abhinav.koppula)
Attachment #8576140 - Flags: review?(jaws)
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 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);
Attachment #8576140 - Flags: review?(jaws) → feedback+
Attachment #8575422 - Attachment is obsolete: true
Attached patch faviconReader.patch (obsolete) — Splinter Review
Added rejection case and catch for promise
Attachment #8576140 - Attachment is obsolete: true
Attachment #8578169 - Flags: review?(jaws)
(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 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.
Attachment #8578169 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/2166c79040a7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Iteration: --- → 39.3 - 30 Mar
Flags: qe-verify?
QA Contact: andrei.vaida
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify? → qe-verify+
Depends on: 1166729
You need to log in before you can comment on or make changes to this bug.