Closed Bug 610242 Opened 14 years ago Closed 13 years ago

Don't show tab icon if there isn't one?

Categories

(Firefox Graveyard :: Panorama, defect, P4)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mitcho, Assigned: mitcho)

References

Details

(Whiteboard: [good first bug][visual])

Attachments

(2 files, 11 obsolete files)

8.93 KB, image/png
Details
12.82 KB, patch
Details | Diff | Splinter Review
Attached image A tab with no icon
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.
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mitcho
Status: NEW → ASSIGNED
Attachment #488780 - Flags: feedback?(ian)
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 on attachment 488780 [details] [diff] [review]
Patch v1

I agree with Kevin's assessment.
Attachment #488780 - Flags: feedback?(ian) → feedback-
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)
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.
(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?
That seems more reliable than detecting extension types (.png, .gif, etc.). So yes!
(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.
Attachment #488780 - Flags: ui-review?(aza) → ui-review+
Attached patch Patch v2 (obsolete) — Splinter Review
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 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-
(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
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.
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. :(
Attachment #498218 - Attachment is obsolete: true
Attachment #499629 - Flags: feedback?(ian)
Attachment #498218 - Flags: ui-review?(aza)
Attached patch Patch v2.2 (obsolete) — Splinter Review
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)
Try passed.
(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...
(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.
(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
(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.
xml can have favicons.
(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?
It's in the useDefaultIcon method, and yes, only image documents are reused as favicons.
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-
> 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.
Attached patch Patch v2.3 (obsolete) — Splinter Review
Attachment #500484 - Attachment is obsolete: true
Attachment #501242 - Flags: review?(ian)
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+
> >+      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.
Attached patch Patch v2.4 (obsolete) — Splinter Review
Attachment #501242 - Attachment is obsolete: true
Attachment #501559 - Flags: approval2.0?
Blocks: 623673
> > >+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 on attachment 501559 [details] [diff] [review]
Patch v2.4

>+  let charsetArg = "charset=" + window.content.document.characterSet;

This is bogus, please remove.
Keywords: helpwanted
Attached patch Patch v2.5 (obsolete) — Splinter Review
Thanks Dāo.
Attachment #501559 - Attachment is obsolete: true
Attachment #501744 - Flags: approval2.0?
Attachment #501559 - Flags: approval2.0?
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.
Attached patch Patch v2.6 (obsolete) — Splinter Review
Ah, alright. How's this look Dāo?
Attachment #501744 - Attachment is obsolete: true
Attachment #501744 - Flags: approval2.0?
Attachment #501750 - Flags: approval2.0?
Looks right at first glance. Does it work? :)
(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.
No longer blocks: 618828
No longer blocks: 622835
Comment on attachment 501750 [details] [diff] [review]
Patch v2.6

a=beltzner
Attachment #501750 - Flags: approval2.0? → approval2.0+
Attached patch Patch for checkin (obsolete) — Splinter Review
Just pushed to try to make sure it all still passes.
Attachment #501750 - Attachment is obsolete: true
This burned on trunk; removing "checkin-needed" flag
Keywords: checkin-needed
Attached patch Patch for checkin, v2 (obsolete) — Splinter Review
There was an instance of the tabItem renaming, and otherwise switched to bug 624265's afterAllTabItemsUpdated.
Attachment #503052 - Attachment is obsolete: true
Attached patch Patch for checkin, v3 (obsolete) — Splinter Review
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
Try passed, modulo one known intermittent orange.
Keywords: checkin-needed
Nvm, will update for collision with bug 624265 which landed...
Keywords: checkin-needed
Resolves collision with 624265.
Attachment #503385 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/3753db4c0387
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: