Closed
Bug 468294
Opened 16 years ago
Closed 15 years ago
Make sure BrowserUI can handle background tab loads
Categories
(Firefox for Android Graveyard :: General, defect, P2)
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.
Updated•16 years ago
|
Flags: blocking-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec A3
Updated•15 years ago
|
tracking-fennec: --- → 1.0+
Flags: blocking-fennec1.0+
Assignee | ||
Comment 1•15 years ago
|
||
Handle background tab Get rid of BrowserUI._faviconLink Make setIcon public as in tabbrowser Don't check multiple time for non existent favicon.ico
Assignee | ||
Comment 2•15 years ago
|
||
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
Assignee | ||
Comment 3•15 years ago
|
||
Sorry for the spam I've forget to hg pull && hg update
Attachment #378830 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #378831 -
Flags: review?
Assignee | ||
Comment 4•15 years ago
|
||
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?
Assignee | ||
Comment 5•15 years ago
|
||
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
Reporter | ||
Comment 6•15 years ago
|
||
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-
Assignee | ||
Comment 7•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #381522 -
Flags: review?
Assignee | ||
Comment 8•15 years ago
|
||
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?
Assignee | ||
Comment 9•15 years ago
|
||
Attachment #381522 -
Attachment is obsolete: true
Attachment #381539 -
Flags: review?
Reporter | ||
Updated•15 years ago
|
Attachment #381539 -
Flags: review? → review?(mark.finkle)
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #381539 -
Attachment is obsolete: true
Attachment #381539 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•15 years ago
|
Attachment #402328 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 12•15 years ago
|
||
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)
Reporter | ||
Comment 13•15 years ago
|
||
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-
Assignee | ||
Comment 14•15 years ago
|
||
* 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)
Reporter | ||
Updated•15 years ago
|
Attachment #405349 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 15•15 years ago
|
||
Comment on attachment 405349 [details] [diff] [review] Patch Thank you keeping this patch alive!
Reporter | ||
Comment 16•15 years ago
|
||
pushed: https://hg.mozilla.org/mobile-browser/rev/ee3d21521b3a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: B1 → B5
Comment 17•15 years ago
|
||
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
Comment 18•15 years ago
|
||
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?
Comment 19•15 years ago
|
||
Added to litmus: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9831.
Updated•14 years ago
|
Flags: in-litmus? → in-litmus+
You need to log in
before you can comment on or make changes to this bug.
Description
•