Closed Bug 468294 Opened 17 years ago Closed 16 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!
Status: NEW → RESOLVED
Closed: 16 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.

Attachment

General

Created:
Updated:
Size: