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)
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•17 years ago
|
Flags: blocking-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec A3
Updated•16 years ago
|
tracking-fennec: --- → 1.0+
Flags: blocking-fennec1.0+
Assignee | ||
Comment 1•16 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•16 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•16 years ago
|
||
Sorry for the spam I've forget to hg pull && hg update
Attachment #378830 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #378831 -
Flags: review?
Assignee | ||
Comment 4•16 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•16 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•16 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•16 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•16 years ago
|
Attachment #381522 -
Flags: review?
Assignee | ||
Comment 8•16 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•16 years ago
|
||
Attachment #381522 -
Attachment is obsolete: true
Attachment #381539 -
Flags: review?
Reporter | ||
Updated•16 years ago
|
Attachment #381539 -
Flags: review? → review?(mark.finkle)
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #381539 -
Attachment is obsolete: true
Attachment #381539 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•16 years ago
|
Attachment #402328 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 12•16 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•16 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•16 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•16 years ago
|
Attachment #405349 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 15•16 years ago
|
||
Comment on attachment 405349 [details] [diff] [review]
Patch
Thank you keeping this patch alive!
Reporter | ||
Comment 16•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: B1 → B5
Comment 17•16 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•16 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•16 years ago
|
||
Added to litmus: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9831.
Updated•15 years ago
|
Flags: in-litmus? → in-litmus+
You need to log in
before you can comment on or make changes to this bug.
Description
•