749305 - Favicons aren't shown when navigating back/forward

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

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.

Comment 1

[JP Rosevear [:jpr]]

It looks like this is ok if you go forward, why is that?

Comment 2

[Brian Nicholson (:bnicholson)]

(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.

Comment 3

[Brian Nicholson (:bnicholson)]

Also, bug 749321 may cause favicons to appear if you go back/forward through several pages (since the page will be reloaded, firing DOMContentLoaded).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

we could look at using "pageshow" instead of "DOMContentLoaded". It is fired all the time.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: 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).

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: 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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: 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)

Comment 9

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/mozilla-central/rev/c798e1ea15b4

Comment 10

[Brian Nicholson (:bnicholson)]

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

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 620067 [details] [diff] [review]
patch v3

[Triage Comment]
Low risk, mobile only Fennec 1.0 blocker. Approved for Aurora 14.

Comment 12

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/ec07ee60ab4c

Comment 13

[Alfredos-Panagiotis Damkalis [:fredy]]

I verify that this bug is fixed on beta version (v14).

Comment 14

[Paul Feher]

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