Closed Bug 697180 Opened 8 years ago Closed 8 years ago

favicon replaced by default one

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: fabrice, Assigned: sriram)

Details

Attachments

(2 files, 1 obsolete file)

STR:

- load about:addons

ER:
The favicon is the green puzzle piece

AR:
The green puzzle piece is displayed, but is then removed and replaced by the default favicon.
Attached patch WIP (obsolete) — Splinter Review
This patch fixes the issue.
The oldUri is used so that "#" changes doesn't reset the favicon.
Assignee: nobody → sriram
Attachment #569451 - Flags: review?(mark.finkle)
Comment on attachment 569451 [details] [diff] [review]
WIP

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java

>     void handleLocationChange(final int tabId, final String uri) {
>         Tab tab = Tabs.getInstance().getTab(tabId);
>-        if (tab != null)
>-            tab.updateURL(uri);
>+        final String oldUri;
>+        if (tab == null)
>+            return;
>+
>+        oldUri = tab.getURL();

Don't declare oldURI until it's needed. And use "oldURI"

>         if (!Tabs.getInstance().isSelectedTab(tab))
>             return;
> 
>         mMainHandler.post(new Runnable() { 
>             public void run() {
>                 mBrowserToolbar.setTitle(uri);
>-                mBrowserToolbar.setFavicon(null);
>+                if (!uri.equals(oldUri))
>+                    mBrowserToolbar.setFavicon(null);

Makes me wonder if we need to reset the title if the URI's are the same. We could avoid posting the runnable altogether.

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

>   onLocationChange: function(aWebProgress, aRequest, aLocationURI) {
>     let browser = BrowserApp.getBrowserForWindow(aWebProgress.DOMWindow);
>     let uri = browser.currentURI.spec;
>+    let contentWin = aWebProgress.DOMWindow;
>+    if (contentWin != contentWin.top)
>+        return;

Put the contentWin check at the beginning of the function. Use "contentWin" in the call to getBrowserForWindow

r+ but put up a new patch with nits fixed
Attachment #569451 - Flags: review?(mark.finkle) → review+
Attached patch PatchSplinter Review
This patch compares the base uri with old base uri and makes change to title bar only when needed. The title and favicon are unaltered for location changes with "#ref".
Attachment #569451 - Attachment is obsolete: true
Attachment #569516 - Flags: review?(mark.finkle)
Attachment #569516 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/projects/birch/rev/9124828a4f2c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attached patch Patch 2Splinter Review
The place for caching of oldURL was wrong.
There were few defensive checks missing on using substring with indexOf()
They have been properly updated on this patch.
Sorry for missing out on them.
Attachment #569573 - Flags: review?(mark.finkle)
Comment on attachment 569573 [details] [diff] [review]
Patch 2

Oh how I wish other languages work more like JS.
Attachment #569573 - Flags: review?(mark.finkle) → review+
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111026 Firefox/10.0a1 Fennec/10.0a1
Status: RESOLVED → VERIFIED
OS: Linux → Android
Hardware: x86_64 → ARM
You need to log in before you can comment on or make changes to this bug.