448080 - switching between tabs fails to update parts of UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

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

1. The titlebar and URL bar location are not updated.

We could hard code the call to update the UI in BrowserUI.selectTab, but listening for an event from deckbrowser would be preferable.

2. The content scroll position is not updated correctly when switching tabs.

Comment 1

[Neil Deakin]

Attachment: update state when tab switching

Fixes this bug as well as bugs 448740 and 448078. Haven't tested it extensively, but it seems to work ok.

Comment 2

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

Comment on attachment 332933 [details] [diff] [review]
update state when tab switching

>   selectTab : function(aTab) {
>     Browser.content.selectTab(aTab);
>+    var browser = getBrowser();
>+    if (this._caption)
>+      this._caption.value = browser.contentDocument.title;
>+    if (this._favicon)
>+      this._favicon.setAttribute("src", browser.mIconURL ? browser.mIconURL.spec :
>+                                                           kDefaultFavIconURL);

You might need to set the this._edit.value as well. Worth noting that this code won't work if an extension calls deckbrowser.selectTab instead of this method. Maybe we should create a listener for "TabSelect" and move this code there.

> 
>-    SpatialNavigation.init(this.content);
>+    try {
>+      SpatialNavigation.init(this.content);
>+    } catch (ex) { }

Did you want this in the patch? SpatialNavigation should always be present.

Otherwise looks good to me.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 332933 [details] [diff] [review]
update state when tab switching

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

>+    if (faviconURI.schemeIs("javascript") ||
>+        fis.isFailedFavicon(faviconURI))
>+      faviconURI = ios.newURI(kDefaultFavIconURL, null, null);
>+
>+    var browser = getBrowser();
>+    browser.mIconURL = faviconURI;

mIconURL in tabbrowser is a string, having it be an nsIURI in Fennec will just be confusing.

>   selectTab : function(aTab) {
>     Browser.content.selectTab(aTab);
>+    var browser = getBrowser();
>+    if (this._caption)
>+      this._caption.value = browser.contentDocument.title;
>+    if (this._favicon)

Why the null checks?

>+      this._favicon.setAttribute("src", browser.mIconURL ? browser.mIconURL.spec :
>+                                                           kDefaultFavIconURL);

Can just use browser.mIconURL || kDefaultFavIconURL if mIconURL is a string.

I agree with mfinkle on both his points.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

One thing I thought of just now is that instead of putting dragData on each tab, it might be simpler to just trigger the existing location change code in browser.js's webprogresslistener, the way that Firefox's tabbrowser does. I think that would require reworking our progresslistener setup to be more similar to that one though (have the "deckbrowser" keep a reference to the browser WPL and filter calls from each of it's browsers to it, instead of setting the browser WPL directly on each <browser>). I also noticed while looking into this that we're not using a BrowserStatusFilter, which is likely negatively impacting our pageload perf. I guess I'll file a bug on this.

Comment 5

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

Attachment: Unbitrotted previous patch and adds review changes

This patch just removes the bitrot and deals with the review comments.

Comment 6

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

The patch also fixes a problem where DOMTitleChanged event handlers needed to be called. I moved the browsr.js DOMTitleChanged handler into browser-ui.js at the same time.

Comment 7

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

changeset:   108:db3419045da4
tag:         tip
user:        Mark Finkle <[e-mail]>
date:        Wed Aug 20 02:09:22 2008 -0400
summary:     b=448080, p=enn & mfinkle, r=gavin. switching between tabs fails to update parts UI

Comment 8

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090821
Fennec/1.0b3pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre)
Gecko/20090808 Fennec/1.0b3pre