Closed Bug 468294 Opened 11 years ago Closed 10 years ago

Make sure BrowserUI can handle background tab loads

Categories

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

x86
Windows XP
defect

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0b5
Tracking Status
fennec 1.0+ ---

People

(Reporter: mfinkle, Assigned: vingtetun)

References

Details

Attachments

(1 file, 9 obsolete files)

12.91 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
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.
Flags: blocking-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec A3
tracking-fennec: --- → 1.0+
Flags: blocking-fennec1.0+
Attached patch Path to handle favicon change (obsolete) — Splinter Review
Handle background tab
Get rid of BrowserUI._faviconLink
Make setIcon public as in tabbrowser
Don't check multiple time for non existent favicon.ico
Attached patch Updated patch (obsolete) — Splinter Review
Patch clean up + change the css, otherwise in the tabs panel there is only a closed icon when a background tab is opened
Attachment #378701 - Attachment is obsolete: true
Attached patch Updated patch on the trunk (obsolete) — Splinter Review
Sorry for the spam I've forget to hg pull && hg update
Attachment #378830 - Attachment is obsolete: true
Attached patch Updated patch (obsolete) — Splinter Review
Drop off CSS change, some clean up, and sync on the trunk
Assignee: nobody → 21
Attachment #378831 - Attachment is obsolete: true
Attachment #378831 - Flags: review?
Attached patch Updated patch (obsolete) — Splinter Review
I don't know whats wrong with me and this patch!
I've forget -p -U 8 in hg diff...
Attachment #381055 - Attachment is obsolete: true
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.
Attachment #381068 - Flags: review-
Attached patch Updated patch (obsolete) — Splinter Review
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))
Attachment #381068 - Attachment is obsolete: true
Comment on attachment 381522 [details] [diff] [review]
Updated patch

Don't review this one, it has an hidden bug for now
Attachment #381522 - Flags: review?
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #381522 - Attachment is obsolete: true
Attachment #381539 - Flags: review?
Attachment #381539 - Flags: review? → review?(mark.finkle)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #381539 - Attachment is obsolete: true
Attachment #381539 - Flags: review?(mark.finkle)
Attached patch Patch (obsolete) — Splinter Review
In this patch I moved setIcon to Browser.js and try to rewrite some few parts of the code to be clearer.
Attachment #402328 - Attachment is obsolete: true
Attachment #404840 - Flags: review?(mark.finkle)
Attachment #402328 - Flags: review?(mark.finkle)
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
Attachment #404840 - Flags: review?(mark.finkle) → review-
Attached patch PatchSplinter Review
* 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 :)
Attachment #404840 - Attachment is obsolete: true
Attachment #405349 - Flags: review?(mark.finkle)
Attachment #405349 - Flags: review?(mark.finkle) → review+
Comment on attachment 405349 [details] [diff] [review]
Patch

Thank you keeping this patch alive!
pushed:
https://hg.mozilla.org/mobile-browser/rev/ee3d21521b3a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: B1 → B5
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
Status: RESOLVED → VERIFIED
We need a test case to regression check if the browser can load tabs in the background  in the tabbed browsing subgroup.
Flags: in-litmus?
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.