Favicons aren't shown when navigating back/forward

VERIFIED FIXED in Firefox 14

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 15
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, blocking-fennec1.0 +)

Details

(Whiteboard: [testday-20120518])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
STR:

1) Go to http://www.google.com
2) Go to about:home
3) Click back

Expected result:
Google favicon is shown

Actual result:
Empty favicon is shown

This happens because we update the favicon on DOMContentLoaded, but we don't receive a DOMContentLoaded when navigating back/forward.
(Assignee)

Updated

6 years ago
Summary: Favicons aren't shown when navigation back/forward → Favicons aren't shown when navigating back/forward

Comment 1

6 years ago
It looks like this is ok if you go forward, why is that?
(Assignee)

Comment 2

6 years ago
(In reply to JP Rosevear [:jpr] from comment #1)
> It looks like this is ok if you go forward, why is that?

Are you talking about when going forward from Google->about:home? If so, about:home is a special case because it doesn't rely on DOMContentLoaded for its favicon to be shown. Try replacing about:home from step 2 above with http://www.yahoo.com.
(Assignee)

Comment 3

6 years ago
Also, bug 749321 may cause favicons to appear if you go back/forward through several pages (since the page will be reloaded, firing DOMContentLoaded).
we could look at using "pageshow" instead of "DOMContentLoaded". It is fired all the time.
blocking-fennec1.0: ? → +
(Assignee)

Updated

6 years ago
Assignee: nobody → bnicholson
(Assignee)

Comment 5

6 years ago
Created attachment 619764 [details] [diff] [review]
patch

This patch moves setting the favicon to pageshow rather than DOMContentLoaded. I also threw in a few other changes:

* Removed the Runnable around loadFavicon() calls that executes it on the UI thread. loadFavicon() just creates/executes an async task, so there's no need to run it on the UI thread.
* Removed setting the title in DOMContentLoaded. DOMTitleChanged is called for every page that has a title, so setting it again in DOMContentLoaded is unnecessary. Now, DOMContentLoaded only notifies listeners of the Tabs.TabEvents.LOADED event. No one actually listens for this event, so we can remove our DOMContentLoaded listener entirely if we wanted (I kept it in case we want it for the future).
Attachment #619764 - Flags: review?(mark.finkle)
(Assignee)

Comment 6

6 years ago
Created attachment 619779 [details] [diff] [review]
patch v2

Actually, it looks like AsyncTasks need to be run on looper threads, so we may need to post to the main thread after all.
Attachment #619764 - Attachment is obsolete: true
Attachment #619764 - Flags: review?(mark.finkle)
Attachment #619779 - Flags: review?(mark.finkle)
Comment on attachment 619779 [details] [diff] [review]
patch v2


>     void handleContentLoaded(int tabId, String uri, String title) {

If we aren't using uri and title maybe we should stop sending them from JS and passing them into this method. Thoughts?

>-        tab.updateTitle(title);

Does DOMTitleChanged get called when we go back and forward in session history?

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>       // Reset invalid submit state on each pageshow
>       case "pageshow":
>         let target = aEvent.originalTarget;
>         let selectedDocument = BrowserApp.selectedBrowser.contentDocument;
>         if (target == selectedDocument || target.ownerDocument == selectedDocument)
>           this._invalidSubmit = false;
>+
>+        // only send pageshow for the top-level document
>+        if (target == target.defaultView.top.document) {
>+          let tab = BrowserApp.getTabForBrowser(BrowserApp.getBrowserForDocument(target));
>+          sendMessageToJava({
>+            gecko: {
>+              type: "Content:PageShow",
>+              tabID: tab.id
>+            }
>+          });
>+        }

You might think I am crazy, but let's add a separate "pageshow" handler in Tab. Let's keep the tab stuff in Tab. Also you can use:
  if (target.ownerDocument.defaultView != this.browser.contentWindow)

to avoid frames in a Tab handler

r- for the "pageshow" addition to Tab
Attachment #619779 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 8

6 years ago
Created attachment 620067 [details] [diff] [review]
patch v3

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 619779 [details] [diff] [review]
> patch v2
> 
> 
> >     void handleContentLoaded(int tabId, String uri, String title) {
> 
> If we aren't using uri and title maybe we should stop sending them from JS
> and passing them into this method. Thoughts?

Works for me, as long as we don't care about logging the uri/title when we get the message in Java.

> >-        tab.updateTitle(title);
> 
> Does DOMTitleChanged get called when we go back and forward in session
> history?

Yes

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >       // Reset invalid submit state on each pageshow
> >       case "pageshow":
> >         let target = aEvent.originalTarget;
> >         let selectedDocument = BrowserApp.selectedBrowser.contentDocument;
> >         if (target == selectedDocument || target.ownerDocument == selectedDocument)
> >           this._invalidSubmit = false;
> >+
> >+        // only send pageshow for the top-level document
> >+        if (target == target.defaultView.top.document) {
> >+          let tab = BrowserApp.getTabForBrowser(BrowserApp.getBrowserForDocument(target));
> >+          sendMessageToJava({
> >+            gecko: {
> >+              type: "Content:PageShow",
> >+              tabID: tab.id
> >+            }
> >+          });
> >+        }
> 
> You might think I am crazy, but let's add a separate "pageshow" handler in
> Tab. Let's keep the tab stuff in Tab. Also you can use:
>   if (target.ownerDocument.defaultView != this.browser.contentWindow)
> 
> to avoid frames in a Tab handler

We can't use target.ownerDocument since target is itself a document (and the ownerDocument is null), but I used something similar:
   if (aEvent.originalTarget.defaultView != this.browser.contentWindow)
Attachment #619779 - Attachment is obsolete: true
Attachment #620067 - Flags: review?(mark.finkle)
Attachment #620067 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/mozilla-central/rev/c798e1ea15b4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → Firefox 15
(Assignee)

Comment 10

6 years ago
Comment on attachment 620067 [details] [diff] [review]
patch v3

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: favicons won't appear when going back/forward
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: none
Attachment #620067 - Flags: approval-mozilla-aurora?
Comment on attachment 620067 [details] [diff] [review]
patch v3

[Triage Comment]
Low risk, mobile only Fennec 1.0 blocker. Approved for Aurora 14.
Attachment #620067 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

6 years ago
status-firefox14: --- → fixed
status-firefox15: --- → fixed
I verify that this bug is fixed on beta version (v14).
Whiteboard: [testday-20120518]

Comment 14

6 years ago
The issue is not reproducible anymore on:
Nightly Fennec 15.0a1 (2012-05-22)
Aurora Fennec 14.0a2 (2012-05-22)
Device: HTC Desire Z
OS: Android 2.3.3

Using the STR from comment 0
status-firefox14: fixed → verified
status-firefox15: fixed → verified

Updated

6 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.