Closed Bug 1420714 Opened 7 years ago Closed 7 years ago

favicon.path is undefined when clicking reader-mode-button

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox58 --- wontfix
firefox59 --- verified
firefox60 --- verified

People

(Reporter: magicp.jp, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Steps to reproduce: 1. Launch Nightly 2. Go to "https://developer.mozilla.org/en-US/Firefox" 3. Click reader-mode-button Actual Results: Logged an error in Browser Console as below. favicon.path is undefined ReaderParent.jsm:47
Flags: needinfo?(gijskruitbosch+bugs)
I know I should have picked this up sooner but I've just been distracted by other stuff. Looking at this just now for unrelated reasons, I can't help but wonder... is this a regression? Is it possible for you to check this? It looks like this is using a places API that should be returning an object with a .path property but is no longer doing so, so I wonder if this is a regression caused by a places API change or something. Having a regression window would make looking for that stuff a lot easier.
Flags: needinfo?(magicp.jp)
Probably bug 1326520 that changed nsIURI.path to nsIURI.pathQueryRef
(In reply to Marco Bonardo [::mak] from comment #2) > Probably bug 1326520 that changed nsIURI.path to nsIURI.pathQueryRef Oh irony, that that updated Readability.js (which it shouldn't have done) but not ReaderParent.jsm (which it should have done).
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1326520
Status: NEW → ASSIGNED
Flags: needinfo?(magicp.jp)
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1433958
No longer blocks: 1426501
Not sure what this has to do with the setters bug - we're only touching getters here, I think? :-)
Flags: needinfo?(valentin.gosu)
My bad. I saw .replace and I thought it was actually changing it :)
No longer blocks: 1433958
Flags: needinfo?(valentin.gosu)
Comment on attachment 8946671 [details] Bug 1420714 - fix favicon fetching in reader mode, https://reviewboard.mozilla.org/r/216648/#review222366 LGTM, but I have a question. ::: toolkit/components/reader/test/browser_readerMode.js:55 (Diff revision 1) > let readerUrl = gBrowser.selectedBrowser.currentURI.spec; > ok(readerUrl.startsWith("about:reader"), "about:reader loaded after clicking reader mode button"); > is_element_visible(readerButton, "Reader mode button is present on about:reader"); > + let favicon = document.getAnonymousElementByAttribute(tab, "anonid", "tab-icon-image"); > + is_element_visible(favicon, "Favicon should be visible"); > + is(favicon.src, TEST_PATH + "favicon.ico", "Correct favicon should be loaded"); I don't know the reader code very well, should this wait on some event to avoid intermittents or use waitForCondition? what prevents it from checking the favicon before it has been fetched and stored?
Attachment #8946671 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #7) > Comment on attachment 8946671 [details] > Bug 1420714 - fix favicon fetching in reader mode, > > https://reviewboard.mozilla.org/r/216648/#review222366 > > LGTM, but I have a question. > > ::: toolkit/components/reader/test/browser_readerMode.js:55 > (Diff revision 1) > > let readerUrl = gBrowser.selectedBrowser.currentURI.spec; > > ok(readerUrl.startsWith("about:reader"), "about:reader loaded after clicking reader mode button"); > > is_element_visible(readerButton, "Reader mode button is present on about:reader"); > > + let favicon = document.getAnonymousElementByAttribute(tab, "anonid", "tab-icon-image"); > > + is_element_visible(favicon, "Favicon should be visible"); > > + is(favicon.src, TEST_PATH + "favicon.ico", "Correct favicon should be loaded"); > > I don't know the reader code very well, should this wait on some event to > avoid intermittents or use waitForCondition? what prevents it from checking > the favicon before it has been fetched and stored? conversely, I don't know places and how the favicon gets fetched and stored. :-) We load the regular page and wait for the load, then wait for the reader mode icon to be visible, then load reader view. Is that likely to be problematic? It worked in my local testing, but that's not a very good guarantee that it'll always work... Is there a good way to wait for the favicon to be present?
Flags: needinfo?(mak77)
> (In reply to Marco Bonardo [::mak] from comment #7) > We load the regular page and wait for the load, then wait for the reader > mode icon to be visible, then load reader view. Is that likely to be > problematic? It worked in my local testing, but that's not a very good > guarantee that it'll always work... I think it boils down to whether all of that work is longer than fetching the icon and storing it, that is all asynchronous and depending on I/O it could actually happen at any time... I can't predict whether in practice it will be an issue on tinderboxes or not. Additionally, it's somehow possible that the reader view itself is not waiting long enough and in some cases will just push out the default icon instead of the right one, though it's likely the user is a bit slower than some test code. > Is there a good way to wait for the favicon to be present? One way could be to add an history observer and wait for onPageChanged with ATTRIBUTE_FAVICON (in the future we'll have a better single-target notification) for the page url. Another way could be to just poll with waitForCondition in the test. A third possibility could be, in the test, to previously store the icon in the favicons service for that page rather than through the <link>, so it's readily available. For example https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/browser/components/places/tests/browser/browser_toolbar_overflow.js#83
Flags: needinfo?(mak77)
As discussed on IRC, worked around the 'waiting' issue by explicitly loading the favicon into places instead of doing it via a pageload. As a bonus, now I don't need external files so the patch is much simpler! :-)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is this worth uplifting to 59?
Comment on attachment 8946671 [details] Bug 1420714 - fix favicon fetching in reader mode, (In reply to Julien Cristau [:jcristau] from comment #14) > Is this worth uplifting to 59? Yes, good point. Approval Request Comment [Feature/Bug causing the regression]: bug 1326520 [User impact if declined]: no favicons for reader mode, errors in the browser console [Is this code covered by automated tests?]: it is now! I added some in the patch. [Has the fix been verified in Nightly?]: I verified manually while writing the patch, and there's automated tests, but it hasn't been independently verified, if that matters... [Needs manual test from QE? If yes, steps to reproduce]: tbh, given manual verification + automated tests, not sure it's helpful, but if necessary steps are in comment #0. [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: no [Why is the change risky/not risky?]: 1-line code change, has automated test, has baked for a bit now [String changes made/needed]: nope
Attachment #8946671 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: in-testsuite+
Comment on attachment 8946671 [details] Bug 1420714 - fix favicon fetching in reader mode, Simple fix with new tests added. Let's take this for 59b8.
Attachment #8946671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have managed to reproduce the issue described in comment 0 using Firefox 59.0a1 (BuildId:20171126220311). This issue is verified fixed using Firefox 60.0a1 (BuildId:20180208103049) and Firefox 59.0b8 (BuildId:20180208125751 -Tinderbox build) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 14.04 64bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1453467
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: