468294 - Make sure BrowserUI can handle background tab loads

Status: VERIFIED FIXED

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

Description

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

BrowserUI currently assumes only the active browser has favicon changes. This would break for any background tab loads.

BrowserUI._faviconLink is one of the problems. Using the <browser>.mIconURL property in the same way might be the simplest fix.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Path to handle favicon change

Handle background tab
Get rid of BrowserUI._faviconLink
Make setIcon public as in tabbrowser
Don't check multiple time for non existent favicon.ico

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Patch clean up + change the css, otherwise in the tabs panel there is only a closed icon when a background tab is opened

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch on the trunk

Sorry for the spam I've forget to hg pull && hg update

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Drop off CSS change, some clean up, and sync on the trunk

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

I don't know whats wrong with me and this patch!
I've forget -p -U 8 in hg diff...

Comment 6

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

Comment on attachment 381068 [details] [diff] [review]
Updated patch

>diff -r dd49884cfcea chrome/content/browser-ui.js
>   _linkAdded : function(aEvent) {
>     var link = aEvent.originalTarget;
>     if (!link || !link.href)
>       return;
> 
>     if (/\bicon\b/i(link.rel)) {
>-      this._faviconLink = link.href;
>-
>-      // If the link changes after pageloading, update it right away.
>-      // otherwise we wait until the pageload finishes
>-      if (this._favicon.src != "")
>-        this._setIcon(this._faviconLink);
>+      var ownerDoc = link.ownerDocument;
>+      if (!ownerDoc) // no document, no icon
>+        return;
>+      let browser = Browser.getBrowserForDocument(ownerDoc);
>+      this.setIcon(browser, link.href);
>     }
>   },

This is the main thing about the patch that I don't like. You are calling "setIcon" immediately from linkAdded. We added "_faviconLink" (and related code) to delay setting the favicon during a page load because it appeared to be slowing down our page loads. Instead, we delay until the page has finished loading, down in the TOOLBARSTATE_LOADED case.

The nsIFaviconService stuff in "setIcon" is probably slower than we think.

Let me think a bit about whether we want to stop delaying the favicon update

>       case TOOLBARSTATE_LOADING:
>         ws.panTo(0, -this.toolbarH);
>         this.showToolbar();
>         icons.setAttribute("mode", "loading");
>-        this._favicon.src = "";
>-        this._faviconLink = null;
>-        this.updateIcon();
>+        this._updateIcon("");

Pass nothing in here and we'll make _updateIcon smarter

>-  updateIcon : function() {
>+  _updateIcon : function(anIconSrc) {

Add:
      anIconSrc = anIconSrc || "";

>+    var fis = Cc["@mozilla.org/browser/favicon-service;1"].getService(Ci.nsIFaviconService);
>+    if (!faviconURI || faviconURI.schemeIs("javascript") || fis.isFailedFavicon(faviconURI)) {
>+      faviconURI = ios.newURI(aBrowser.currentURI.prePath + "/favicon.ico", null, null);
>+      if (fis.isFailedFavicon(faviconURI))
>+        faviconURI = ios.newURI(kDefaultFavIconURL, null, null);
>+      }
>+
>+    fis.setAndLoadFaviconForPage(aBrowser.currentURI, faviconURI, true);
>+    aBrowser.mIconURL = faviconURI.spec;

We should cache the "fis" object. Even something cheap like:

    if (!this._fis)
      this._fis = Cc[...

then use this._fis in the rest of the function. Check to see if we use nsIFaviconService anywhere else in BrowserUI and switch to this._fis

>diff -r dd49884cfcea chrome/content/browser.js

>+  getBrowserForDocument: function( aDocument ){
>+    let browsers = this.browsers;
>+    for( let i = 0 ; i < browsers.length ; i++ ){
>+      if( browsers[i].contentDocument === aDocument )
>+        return browsers[i];

Fix up the style a little:

function(aDocument) {
for (let i = 0; i < browsers.length; i++) {
if (browsers[i].contentDocument === aDocument)

>     if (aWebProgress.DOMWindow == selectedBrowser.contentWindow) {
>       BrowserUI.setURI();
>     }

Add a blank line

>+    if (aWebProgress.DOMWindow == this.browser.contentWindow &&
>+      aWebProgress.isLoadingDocument)
>+      BrowserUI.setIcon( this.browser, null);

Put the if statement all on 1 line. It's more readable that way


Make those changes and I'll think about the user perception issue.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Patch corrected with your comments.
So, now setIcon is called only on networkStop, I've also correct a bug in networkStop to prevent calling 2 times updateThumbnail when Fennec is launched (Browser._isStartup instead of this._isStartup).

Let me now if you have any other comments?
(There is also a problem when I go on a URI starting with chrome that doesn't exists but it sounds like an other bug (networkStop is never called))

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 381522 [details] [diff] [review]
Updated patch

Don't review this one, it has an hidden bug for now

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Updated patch

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

In this patch I moved setIcon to Browser.js and try to rewrite some few parts of the code to be clearer.

Comment 13

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

Comment on attachment 404840 [details] [diff] [review]
Patch

>diff -r 2a4f30d11af3 chrome/content/browser-ui.js
>   _linkAdded : function(aEvent) {

>+      let tab = Browser.getTabForDocument(ownerDoc);
>+      tab.browser.mIconURL = link.href;

First, I wanted to move "setIcon" to the Tab object, not the Browser object.

Second, Couldn't we call tab.setIcon(link.href) ?

>       // If the link changes after pageloading, update it right away.
>       // otherwise we wait until the pageload finishes
>-      if (this._favicon.src != "")
>-        this._setIcon(this._faviconLink);
>+      if (!tab.isLoading()) {
>+        Browser.setIcon(browser, browser.mIconURL);

You don't need this call if we do the above call.

>+        if (browser === Browser.selectedBrowser)
>+          this._updateIcon(browser.mIconURL);

Add this check into the above "if" statement since this is the only part we care about now.


>diff -r 2a4f30d11af3 chrome/content/browser.js
> 
> var Browser = {

>+  _fis: null,

Hmm, could we add a gFaviconService to the place in browser-ui so this is not an issue?

>+  setIcon: function(aBrowser, aURI) {

Move to Tab and no need for the aBrowser param since a Tab already knows its browser

>+    var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);

Another gIOService browser-ui?

>+    var faviconURI = null;

"let"

>diff -r 2a4f30d11af3 themes/wince/browser-low.css
>+#urlbar-favicon[src=""] {
>+  list-style-image: url("chrome://browser/skin/images/favicon-default-30.png");

favicon-default-24.png

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

* Address comments

(I think the comments have made this patch a much much better version than the first one I've initially created. The review process is really great!)

Anyway, I hope this one will be good enough :)

Comment 15

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

Comment on attachment 405349 [details] [diff] [review]
Patch

Thank you keeping this patch alive!

Comment 16

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/ee3d21521b3a

Comment 17

[Aakash Desai [:aakashd]]

Looks great using the steps to reproduce in bug 519238 on builds:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091009
Fennec/1.0b4

Comment 18

[Aakash Desai [:aakashd]]

We need a test case to regression check if the browser can load tabs in the background  in the tabbed browsing subgroup.

Comment 19

[Al Billings [:abillings]]

Added to litmus: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9831.