Closed
Bug 610242
Opened 14 years ago
Closed 14 years ago
Don't show tab icon if there isn't one?
Categories
(Firefox Graveyard :: Panorama, defect, P4)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: mitcho, Assigned: mitcho)
References
Details
(Whiteboard: [good first bug][visual])
Attachments
(2 files, 11 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Right now we display the "tab icon" gray area, obscuring the thumbnail, even if there's no icon. What if we don't do this if there's no icon?
I think being able to see more of the tab would be much better.
Assignee | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
The right answer here (and the one in the spec) is that if the website doesn't have a favicon (or we can't load it) is to use the default favicon that Firefox provides.
This bug is also related to a bug 600645, and the fix for that should also fix this.
I am also skeptical that we should change the UI design because of a bug.
Depends on: 600645
Comment 3•14 years ago
|
||
Comment on attachment 488780 [details] [diff] [review]
Patch v1
I agree with Kevin's assessment.
Attachment #488780 -
Flags: feedback?(ian) → feedback-
Assignee | ||
Comment 4•14 years ago
|
||
Comment on attachment 488780 [details] [diff] [review]
Patch v1
I'd like to appeal and check with Aza on this. I don't see the point of drawing the default favicon and obscuring the thumb.
Attachment #488780 -
Flags: ui-review?(aza)
Comment 5•14 years ago
|
||
While I am sort of swayed by seeing more of the thumb, I think in this case breaking the visual rhythm of the tabs by having some be full thumbs and some having the little chomps would be visually distracting.
The only place where I think this isn't the case is when the thumbnail is of an image. In that case, there is too much repetition and we shouldn't show the chomp.
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #5)
> The only place where I think this isn't the case is when the thumbnail is of an
> image. In that case, there is too much repetition and we shouldn't show the
> chomp.
Agreed.
What should be the criteria for that? If the document is an ImageDocument instead of an HTMLDocument?
Comment 7•14 years ago
|
||
That seems more reliable than detecting extension types (.png, .gif, etc.). So yes!
Comment 8•14 years ago
|
||
(In reply to comment #2)
> This bug is also related to a bug 600645, and the fix for that should also fix
> this.
Bug 600645 has landed, and I don't believe it fixes this, but its technique can certainly be borrowed for fixing this.
Updated•14 years ago
|
Attachment #488780 -
Flags: ui-review?(aza) → ui-review+
Assignee | ||
Comment 9•14 years ago
|
||
This patch checks to see that the tab's content type includes the text "html" and only displays a favicon then. This seemed to be a more reliable test than checking that the document is an instanceof nsIDOMHTMLElement as things like ImageDocuments also pass that test.
Attachment #488780 -
Attachment is obsolete: true
Attachment #498218 -
Flags: ui-review?(aza)
Attachment #498218 -
Flags: review?(ian)
Comment 10•14 years ago
|
||
Comment on attachment 498218 [details] [diff] [review]
Patch v2
Why doesn't about:config get an icon? Because it's not an html document? What else fits into that category?
What's the purpose of the setTimeout in the test? If you're waiting for the pages to load, please use an explicit load test (such as afterAllTabsLoaded in browser_tabview_privatebrowsing.js... I've been thinking that should move into head.js so it's available to all of our tests; maybe now's the time to do it). Otherwise we'll likely have an intermittent orange.
Otherwise looks good.
Attachment #498218 -
Flags: review?(ian) → review-
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10)
> Comment on attachment 498218 [details] [diff] [review]
> Patch v2
>
> Why doesn't about:config get an icon? Because it's not an html document? What
> else fits into that category?
about:config has content type "application/vnd.mozilla.xul+xml", so it's not HTML.
The reason I went with this heuristic is that checking the contentDocument |instanceof Ci.nsIDOMHTMLDocument| (my first instinct) actually doesn't distinguish between images and non-image documents, though it does distinguish XML and svg.
If anyone has a better suggestion for a heuristic, I'd appreciate it.
> What's the purpose of the setTimeout in the test? If you're waiting for the
> pages to load, please use an explicit load test (such as afterAllTabsLoaded in
> browser_tabview_privatebrowsing.js... I've been thinking that should move into
> head.js so it's available to all of our tests; maybe now's the time to do it).
> Otherwise we'll likely have an intermittent orange.
Changing it to use afterAllTabsLoaded now.
Keywords: helpwanted
Assignee | ||
Comment 12•14 years ago
|
||
It turns out afterAllTabsLoaded wasn't good enough... I've created an afterAllTabItemsUpdated which waits until GroupItems__update() touches every tab's tab item. The test passes by itself but I'm having trouble with it passing as part of the suite... I just sent it to try to see if the try server is the same, and will fight with it a bit later.
Assignee | ||
Comment 13•14 years ago
|
||
try server, not too surprisingly, also trips up at the exact same point. :( I'll attach the current WIP here. Ian or others, I'd appreciate it if I could get help on getting this test to run smoothly. :(
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #498218 -
Attachment is obsolete: true
Attachment #499629 -
Flags: feedback?(ian)
Attachment #498218 -
Flags: ui-review?(aza)
Assignee | ||
Comment 15•14 years ago
|
||
Now fully passes mochitest-other, locally, but leaks. Pushed to try to check.
Attachment #499629 -
Attachment is obsolete: true
Attachment #500484 -
Flags: review?(ian)
Attachment #499629 -
Flags: feedback?(ian)
Assignee | ||
Comment 16•14 years ago
|
||
Try passed.
Comment 17•14 years ago
|
||
(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 498218 [details] [diff] [review]
> > Patch v2
> >
> > Why doesn't about:config get an icon? Because it's not an html document? What
> > else fits into that category?
>
> about:config has content type "application/vnd.mozilla.xul+xml", so it's not
> HTML.
This seems quite bogus...
Assignee | ||
Comment 18•14 years ago
|
||
(In reply to comment #17)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Comment on attachment 498218 [details] [diff] [review]
> > > Patch v2
> > >
> > > Why doesn't about:config get an icon? Because it's not an html document? What
> > > else fits into that category?
> >
> > about:config has content type "application/vnd.mozilla.xul+xml", so it's not
> > HTML.
>
> This seems quite bogus...
Dão, can you suggest a better heuristic than the current method of checking for "html" in the content type?
Also adding Faaborg, in case he has an idea on the heuristic to use here.
Comment 19•14 years ago
|
||
(In reply to comment #18)
> Dão, can you suggest a better heuristic than the current method of checking for
> "html" in the content type?
instanceof ImageDocument
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #19)
> (In reply to comment #18)
> > Dão, can you suggest a better heuristic than the current method of checking for
> > "html" in the content type?
>
> instanceof ImageDocument
But then we would show tab icons for things like video and xml.
Comment 21•14 years ago
|
||
xml can have favicons.
Assignee | ||
Comment 22•14 years ago
|
||
(In reply to comment #21)
> xml can have favicons.
Do video have favicons as well?
In general, can you point me to where tabbrowser determines whether a tab will simply reuse the document image as the favicon (as that is, per aza's comment above, what we are trying to reduce here)? Is it really just images?
Comment 23•14 years ago
|
||
It's in the useDefaultIcon method, and yes, only image documents are reused as favicons.
Comment 24•14 years ago
|
||
Comment on attachment 500484 [details] [diff] [review]
Patch v2.2
>+ Utils.log("update");
>+ tabItem._sendToSubscribers("update");
Kill this log.
>+ shouldLoadFavIcon: function TabItems_shouldLoadFavIcon(browser) {
>+ return browser.contentWindow.document.contentType.match(/html/i) &&
>+ gBrowser.shouldLoadFavIcon(browser.contentDocument.documentURIObject);
>+ },
Let's go with Dao's heuristic (instanceof ImageDocument, or .useDefaultIcon).
>+function newWindowWithTabView(callback) {
Seems like this might be a useful function to add to head.js.
>+function afterAllTabItemsUpdated(callback, win) {
>+ win = win || window;
>+ let tabItems = win.document.getElementById("tab-view").contentWindow.TabItems;
>+
>+ let onUpdate = function(tabItem) {
>+ tabItem.removeSubscriber(tabItem, "update", onUpdate);
>+ info("updated with " + tabItems._tabsWaitingForUpdate.length + " left");
>+ if (!tabItems._tabsWaitingForUpdate.length)
>+ callback();
>+ }
>+
>+ for (let a = 0; a < win.gBrowser.tabs.length; a++) {
>+ let tabItem = win.gBrowser.tabs[a].tabItem;
>+ if (tabItem) {
>+ info("registering update");
>+ tabItem.addSubscriber(tabItem, "update", onUpdate);
>+ tabItems.update(win.gBrowser.tabs[a]);
>+ }
>+ }
>+}
Seems like this would be more reliable if you called ._update instead of .update (wouldn't have the possible delay of the heartbeat). Then you probably wouldn't even need the subscription.
Attachment #500484 -
Flags: review?(ian) → review-
Assignee | ||
Comment 25•14 years ago
|
||
> Let's go with Dao's heuristic (instanceof ImageDocument, or .useDefaultIcon).
Done.
> >+function newWindowWithTabView(callback) {
>
> Seems like this might be a useful function to add to head.js.
Great idea! I modified it slightly in this new patch.
> Seems like this would be more reliable if you called ._update instead of
> .update (wouldn't have the possible delay of the heartbeat). Then you probably
> wouldn't even need the subscription.
Ah, that worked. Simplified that way.
Assignee | ||
Comment 26•14 years ago
|
||
Attachment #500484 -
Attachment is obsolete: true
Attachment #501242 -
Flags: review?(ian)
Comment 27•14 years ago
|
||
Comment on attachment 501242 [details] [diff] [review]
Patch v2.3
>+ tabItem._sendToSubscribers("update");
Since the test is no longer using this, let's get rid of it.
>+function newWindowWithTabView(callback, completeCallback) {
>+ let charsetArg = "charset=" + window.content.document.characterSet;
>+ let win = window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no,height=800,width=800",
>+ "about:blank", charsetArg, null, null, true);
>+ let onLoad = function() {
>+ win.removeEventListener("load", onLoad, false);
>+ let onShown = function() {
>+ info("shown!");
Let's kill that info call, since this is now a shared library; if specific tests want it, they can add it in their callback.
>+ win.removeEventListener("tabviewshown", onShown, false);
>+ callback(win);
>+ if (typeof completeCallback == "function")
>+ completeCallback();
Why two callbacks? Seems like we only need one.
R+ with those addressed.
Attachment #501242 -
Flags: review?(ian) → review+
Assignee | ||
Comment 28•14 years ago
|
||
> >+ win.removeEventListener("tabviewshown", onShown, false);
> >+ callback(win);
> >+ if (typeof completeCallback == "function")
> >+ completeCallback();
>
> Why two callbacks? Seems like we only need one.
>
> R+ with those addressed.
Ah, yes. These used to have a win.close() in between them. Thanks for catching that.
Assignee | ||
Comment 29•14 years ago
|
||
Attachment #501242 -
Attachment is obsolete: true
Attachment #501559 -
Flags: approval2.0?
Assignee | ||
Comment 30•14 years ago
|
||
> > >+function newWindowWithTabView(callback) {
> >
> > Seems like this might be a useful function to add to head.js.
>
> Great idea! I modified it slightly in this new patch.
Filed followup: bug 623673.
Comment 31•14 years ago
|
||
Comment on attachment 501559 [details] [diff] [review]
Patch v2.4
>+ let charsetArg = "charset=" + window.content.document.characterSet;
This is bogus, please remove.
Updated•14 years ago
|
Keywords: helpwanted
Assignee | ||
Comment 32•14 years ago
|
||
Thanks Dāo.
Attachment #501559 -
Attachment is obsolete: true
Attachment #501744 -
Flags: approval2.0?
Attachment #501559 -
Flags: approval2.0?
Comment 33•14 years ago
|
||
Comment on attachment 501744 [details] [diff] [review]
Patch v2.5
>+function newWindowWithTabView(callback) {
>+ let win = window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no,height=800,width=800",
>+ "about:blank", charsetArg, null, null, true);
charsetArg doesn't exist anymore. You can spare all arguments after the third one.
Assignee | ||
Comment 34•14 years ago
|
||
Ah, alright. How's this look Dāo?
Attachment #501744 -
Attachment is obsolete: true
Attachment #501744 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Attachment #501750 -
Flags: approval2.0?
Comment 35•14 years ago
|
||
Looks right at first glance. Does it work? :)
Assignee | ||
Comment 36•14 years ago
|
||
(In reply to comment #35)
> Looks right at first glance. Does it work? :)
Yup, working locally. v2.4 passed try, and that's the only change.
Comment 37•14 years ago
|
||
Comment on attachment 501750 [details] [diff] [review]
Patch v2.6
a=beltzner
Attachment #501750 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 38•14 years ago
|
||
Just pushed to try to make sure it all still passes.
Attachment #501750 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 39•14 years ago
|
||
This burned on trunk; removing "checkin-needed" flag
Keywords: checkin-needed
Assignee | ||
Comment 40•14 years ago
|
||
There was an instance of the tabItem renaming, and otherwise switched to bug 624265's afterAllTabItemsUpdated.
Attachment #503052 -
Attachment is obsolete: true
Assignee | ||
Comment 41•14 years ago
|
||
Just fixing some bitrot. It's almost as if we've recently landed a bajillion patches! :D
Also pushed to try, just to be safe, as patch v2 changed the afterAllTabsUpdated code.
Attachment #503307 -
Attachment is obsolete: true
Assignee | ||
Comment 42•14 years ago
|
||
Try passed, modulo one known intermittent orange.
Keywords: checkin-needed
Assignee | ||
Comment 43•14 years ago
|
||
Nvm, will update for collision with bug 624265 which landed...
Keywords: checkin-needed
Assignee | ||
Comment 44•14 years ago
|
||
Resolves collision with 624265.
Attachment #503385 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 45•14 years ago
|
||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•