Closed Bug 749305 Opened 10 years ago Closed 10 years ago

Favicons aren't shown when navigating back/forward

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

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

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: bnicholson, Assigned: bnicholson)

Details

(Whiteboard: [testday-20120518])

Attachments

(1 file, 2 obsolete files)

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.
Summary: Favicons aren't shown when navigation back/forward → Favicons aren't shown when navigating back/forward
It looks like this is ok if you go forward, why is that?
(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.
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: nobody → bnicholson
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patch v2 (obsolete) — Splinter Review
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-
Attached patch patch v3Splinter Review
(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+
http://hg.mozilla.org/mozilla-central/rev/c798e1ea15b4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
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+
I verify that this bug is fixed on beta version (v14).
Whiteboard: [testday-20120518]
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: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.