678374 - Panorama/App Tabs: not shortcut icon; no icon if browser.chrome.favicons=false

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Stefan]

User Agent:  

Steps to reproduce:

1. Disable the autonomous search for favicons: browser.chrome.favicons=false
2. Open some sites which habe a regular site icon each in its own new tab , e.g
http://www.stern.de/
http://www.spiegel.de/
3. Pin the tabs as App Tabs
4. Change into Panorama (shiftctrl-e)


Actual results:

The App Tabs are signified using an ersatz icon.


Expected results:

The App Tabs are signified using the site's site icon.

Comment 1

[Stefan]

Attachment: screenshot

Comment 2

[Ioana (away)]

Attachment: Fx9.0a1 screenshots

Comment 3

[Ioana (away)]

Verified on:
Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110920 Firefox/9.0a1 (20110920085517)

This issue reproduces as described by the reporter.

Comment 4

[Ioana (away)]

Regression window:

Last good nightly: 2011-05-04; First bad nightly: 2011-05-05
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3c4c902e9cd&tochange=31879b88cc82

Comment 5

[(mostly gone) XtC4UaLL [:xtc4uall]]

Fallout of Bug 649347?

Comment 6

[Stefan]

Not only the app tabs have no icon in panorama but also the normal tabs don't show an icon in the upper left corner. The only page which shows an icon in panorama seems to be about:home.

Comment 7

[Raymond Lee [:raymondlee]]

I don't see why it's an issue.  When browser.chrome.favicons is set to false, you don't expect to see any favicons in the address bar, and tabs, so why we expect the favicons are shown for Panorama?

http://kb.mozillazine.org/Browser.chrome.favicons

Comment 8

[Raymond Lee [:raymondlee]]

Furthermore, Panorama is getting the favicons via nsIFaviconService and also validates to show favcions or not based on shouldLoadFavIcon().  http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#722

Therefore, we expect to see favicons in Panorama to act the same as favicons on browser tabs.

Comment 9

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #7)
> When browser.chrome.favicons is set to
> false, you don't expect to see any favicons in the address bar, and tabs, so
> why we expect the favicons are shown for Panorama?

When browser.chrome.favicons is set to false I expect only not to see FAVICONS. I do not expect firefox not to show other types of site icons.

Comment 10

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #8)
> Furthermore, Panorama is getting the favicons via nsIFaviconService and also
> validates to show favcions or not based on shouldLoadFavIcon(). 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#722
> 
> Therefore, we expect to see favicons in Panorama to act the same as favicons
> on browser tabs.

With browser.chrome.favicons=false I get here:

Does http://www.spiegel.de/ show a site icon in tab? (Yes)
Does http://www.spiegel.de/ show a site icon in app tab? (Yes)
Does http://www.spiegel.de/ show a site icon in tab in panorama? (No)
Does http://www.spiegel.de/ show a site icon in app tab in panorama? (No)

Comment 11

[Stefan]

(In reply to XtC4UaLL [:xtc4uall] from comment #5)
> Fallout of Bug 649347?

After having removed Bug #649347 (939ccc994fba) the app tab's site icons reappear.
The normal tab's site icons are not visible in panorama. This seems to be a seperature issue, right?

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: WIP v1

This patch should fix the issue.  Still need to write a test and update other tests.

Comment 13

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #12)
> This patch should fix the issue.

Yep.  Just the app tab's site icons are not visible when panorama is invoked for the first time.  After adjusting the group's size they show up. New groups have to be resized, too.

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to Stefan from comment #13)
> (In reply to Raymond Lee [:raymondlee] from comment #12)
> > This patch should fix the issue.
> 
> Yep.  Just the app tab's site icons are not visible when panorama is invoked
> for the first time.  After adjusting the group's size they show up. New
> groups have to be resized, too.

You mean when you create a new group,  the app tab's site icons are not visible until you resize that group?

Comment 15

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #14)
> You mean when you create a new group,  the app tab's site icons are not
> visible until you resize that group?

Yes.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: WIP v2

(In reply to Stefan from comment #13)
> Yep.  Just the app tab's site icons are not visible when panorama is invoked
> for the first time.  After adjusting the group's size they show up. New
> groups have to be resized, too.

This patch should fix this.

Comment 17

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #16)
> This patch should fix this.

It does.  Thx.

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: WIP v3

Fixed other tests.  Still need to write a test for this patch.

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: v1 doesn't depend on bug 673569

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: v1 depends on bug 673569

Comment 21

[Raymond Lee [:raymondlee]]

Attachment: v1 depends on bug 673569

Comment 22

[Stefan]

I get a reject in browser/base/content/tabview/groupitems.js

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v2 doesn't depend on bug 673569

Comment 24

[Raymond Lee [:raymondlee]]

(In reply to Stefan from comment #22)
> I get a reject in browser/base/content/tabview/groupitems.js

The new v2 patch should work now.

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: v2 depends on bug 673569

Comment 26

[Stefan]

No rule to make target `test_bug678374_icon16.png', needed by `libs'.  Stop.

Comment 27

[Raymond Lee [:raymondlee]]

(In reply to Stefan from comment #26)
> No rule to make target `test_bug678374_icon16.png', needed by `libs'.  Stop.

The patch contains an image as "GIT binary patch".  Please google around for importing this patch properly.

Comment 28

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #27)
The patch bug-678374-v2-1.diff applies.  However, in Panorama I get:

- For app tabs: No icons.
- In case of regular tabs: gray areas where the icon is expected.

Comment 29

[Raymond Lee [:raymondlee]]

(In reply to Stefan from comment #28)
> (In reply to Raymond Lee [:raymondlee] from comment #27)
> The patch bug-678374-v2-1.diff applies.  However, in Panorama I get:
> 
> - For app tabs: No icons.
> - In case of regular tabs: gray areas where the icon is expected.

As the name suggested for "v2 depends on bug 673569" (bug-678374-v2-1.diff), this patch depends on bug 673569.  Until bug 673569 is resolved, this patch wouldn't work.

Try "v2 doesn't depend on bug 673569" instead please.

Comment 30

[Stefan]

(In reply to Raymond Lee [:raymondlee] from comment #29)
> Try "v2 doesn't depend on bug 673569" instead please.

Thx., this patch works.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 563052 [details] [diff] [review]
v2 depends on bug 673569

Let's rather not wait for bug 673569.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 563051 [details] [diff] [review]
v2 doesn't depend on bug 673569

Review of attachment 563051 [details] [diff] [review]:
-----------------------------------------------------------------

I tried the patch but with favicons=false I get no favicons (as normal or app tab) for www.spiegel.de and www.stern.de. Shouldn't this work according to Stefan?

::: browser/base/content/tabview/content.js
@@ +36,5 @@
>  
>  "use strict";
>  
> +Components.utils.import("resource:///modules/tabview/utils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

I like that as a temporary solution. Let's not wait for bug 673569.

@@ +110,5 @@
> +  shouldLoadFavIcon: function WMH_shouldLoadFavIcon(cx) {
> +    let uri = content.document.documentURIObject;
> +    let shouldLoad = (uri &&
> +                      Services.prefs.getBoolPref("browser.chrome.site_icons") &&
> +                      Services.prefs.getBoolPref("browser.chrome.favicons") &&

This means that we don't get any icons if "browser.chrome.favicons" is false?  I think we need to load site icons, but not favicons (/favicon.ico) in this case.

::: browser/base/content/tabview/ui.js
@@ +1624,3 @@
>    // Function: getFavIconUrlForTab
>    // Gets fav icon url for the given xul:tab.
> +  getFavIconUrlForTab: function UI_getFavIconUrlForTab(tab, callback) {

Could you please add "UI._isImageDocument(tab, callback)" and "UI._shouldLoadFavIcon(tab, callback)" that basically just do the message work and call the given callback with a boolean? That should make this function a bit more readable.

@@ +1629,5 @@
>  
> +    mm.addMessageListener(message, function onMessage(cx) {
> +      mm.removeMessageListener(cx.name, onMessage);
> +      if (cx.json.isImageDocument) {
> +        callback(tab.pinned ? tab.image : null);

Why do unpinned image documents don't have a favicon?

Comment 33

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to Tim Taubert [:ttaubert] from comment #32)
> I tried the patch but with favicons=false I get no favicons (as normal or
> app tab) for www.spiegel.de and www.stern.de. Shouldn't this work according
> to Stefan?
>

It works fine for me.  May be something to do with your profile?
 
> ::: browser/base/content/tabview/content.js
> @@ +36,5 @@
> >  
> >  "use strict";
> >  
> > +Components.utils.import("resource:///modules/tabview/utils.jsm");
> > +Components.utils.import("resource://gre/modules/Services.jsm");
> 
> I like that as a temporary solution. Let's not wait for bug 673569.
> 
> @@ +110,5 @@
> > +  shouldLoadFavIcon: function WMH_shouldLoadFavIcon(cx) {
> > +    let uri = content.document.documentURIObject;
> > +    let shouldLoad = (uri &&
> > +                      Services.prefs.getBoolPref("browser.chrome.site_icons") &&
> > +                      Services.prefs.getBoolPref("browser.chrome.favicons") &&
> 
> This means that we don't get any icons if "browser.chrome.favicons" is
> false?  I think we need to load site icons, but not favicons (/favicon.ico)
> in this case.
> 

Please check the UI_getFavIconUrlForTab(), the WMH_shouldLoadFavIcon() is only called when tab.image is not available.  Therefore, we use it to determine to load the default/cached icon or not and ensure we don't show the default icon for about:-style error pages

Ref:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#734

> ::: browser/base/content/tabview/ui.js
> @@ +1624,3 @@
> >    // Function: getFavIconUrlForTab
> >    // Gets fav icon url for the given xul:tab.
> > +  getFavIconUrlForTab: function UI_getFavIconUrlForTab(tab, callback) {
> 
> Could you please add "UI._isImageDocument(tab, callback)" and
> "UI._shouldLoadFavIcon(tab, callback)" that basically just do the message
> work and call the given callback with a boolean? That should make this
> function a bit more readable.
> 

Done

> @@ +1629,5 @@
> >  
> > +    mm.addMessageListener(message, function onMessage(cx) {
> > +      mm.removeMessageListener(cx.name, onMessage);
> > +      if (cx.json.isImageDocument) {
> > +        callback(tab.pinned ? tab.image : null);
> 
> Why do unpinned image documents don't have a favicon?

See bug 610242 comment 6

Comment 34

[Stefan]

Do you have a patch which applies to the current revision?

Comment 35

[Stefan]

This commit breaks the v2 patch:
Bug 682791 - Ensure that all functions have an internal name for Panorama r=tim 
http://hg.mozilla.org/mozilla-central/rev/121549dd2e34

Comment 36

[Raymond Lee [:raymondlee]]

Attachment: v4

unrotted

Comment 37

[Raymond Lee [:raymondlee]]

Attachment: v5

Updated the patch after the patch change of Panorama module.

Comment 38

[Raymond Lee [:raymondlee]]

Attachment: v6

Unrotted

Comment 39

[Raymond Lee [:raymondlee]]

It's best to fix Bug 728142 first and then this one.  Adding the dependency.

Comment 40

[Raymond Lee [:raymondlee]]

Attachment: v7

Unrotted

Comment 41

[Raymond Lee [:raymondlee]]

Tim: After reviewing the changes needed in both bug 728142 and this bug, I think it's better to land the patch in this bug first because it adds callback function as second param to several methods.  This would make it much easier for switching to aync fav icon for bug 728142.

Comment 42

[Raymond Lee [:raymondlee]]

Attachment: v8

There were some random oranges caused by the patch v7.  This new patch includes some fixes for those issues.

Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=ffd3dea5b8a8

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 599922 [details] [diff] [review]
v8

Review of attachment 599922 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/tabview/content.js
@@ +108,5 @@
> +
> +  // ----------
> +  // Function: shouldLoadFavIcon
> +  // Checks if fav icon should be loaded or not.
> +  shouldLoadFavIcon: function WMH_shouldLoadFavIcon(cx) {

We don't need this to be in the content script. Accessing tab.linkedBrowser.currentURI is perfectly fine with e10s. Please make sure we cache those pref values to not query them every time this function is called.

::: browser/components/tabview/groupitems.js
@@ +238,5 @@
>    AllTabs.tabs.forEach(function(xulTab) {
> +    if (xulTab.pinned) {
> +      // only adjust tray when it's the last app tab.
> +      self.addAppTab(xulTab,
> +                     { adjustTrayForLastAppTab: true,

Couldn't you just use pinnedTabCount and _numPinnedTabs instead of introducing this new option?

@@ +1196,2 @@
>    addAppTab: function GroupItem_addAppTab(xulTab, options) {
>      let self = this;

Please use .bind(this).

@@ +2097,5 @@
>    _updateAppTabIcons: function GroupItems__updateAppTabIcons(xulTab) {
>      if (!xulTab.pinned)
>        return;
>  
> +    let self = this;

Please use .bind(this).

@@ +2100,5 @@
>  
> +    let self = this;
> +    this.getAppTabFavIconUrl(xulTab, function GroupItems__updateAppTabIcons_getAppTabFavIconUrl(iconUrl) {
> +      self.groupItems.forEach(function(groupItem) {
> +        iQ(".appTabIcon", groupItem.$appTabTray).each(function(icon) {

Please just call iQ(".appTabIcon") and then iterate through all app tab icons. We need to check all of those elements but I think that's better than calling iQ() repeatedly for every groupItem.

::: browser/components/tabview/tabitems.js
@@ +990,5 @@
> +        if (iconUrl) {
> +          if (tabItem.$favImage[0].src != iconUrl)
> +            tabItem.$favImage[0].src = iconUrl;
> +          iQ(tabItem.$fav[0]).show();
> +          tabItem._sendToSubscribers("iconUpdated", { show: true });

Do we really need this additional data here? This is easy to query and no place needs this so far.

::: browser/components/tabview/test/browser_tabview_bug678374.js
@@ +25,5 @@
> +        pinTab(newTabTwo, function() {
> +          let icon = cw.iQ(".appTabIcon", groupItem.$appTabTray)[0];
> +          is(icon.src, iconUrl, "The app tab is showing the right icon");
> +
> +          Services.prefs.clearUserPref("browser.chrome.favicons");

Please move this into a registerCleanupFunction() call to make sure we unset this pref again.

::: browser/components/tabview/test/head.js
@@ +377,5 @@
>    pb.privateBrowsingEnabled = !pb.privateBrowsingEnabled;
>  }
> +
> +// ----------
> +function pinTab(xulTab, callback, win) {

The function name isn't really clear about it waiting for the app tab icon to be added. Can you please move all code except "gBrowser.pinTab()" to whenAppTabIconAdded() and use that in the tests? So we don't need this function and the tests are clear about what they're doing and waiting for.

@@ +389,5 @@
> +  win.gBrowser.pinTab(xulTab);
> +}
> +
> +// ----------
> +function whenTabIsPinned(groupItem, callback) {

whenAppTabIconAdded() would be a better name for this.

::: browser/components/tabview/ui.js
@@ +1631,5 @@
> +    mm.addMessageListener(message, function onMessage(cx) {
> +      mm.removeMessageListener(cx.name, onMessage);
> +      let url = null;
> +      if (cx.json.shouldLoad)
> +        url = gFavIconService.getFaviconImageForPage(tab.linkedBrowser.currentURI).spec;

You're fetching the favicon here and in getFavIconUrlForTab() again?

@@ +1641,5 @@
>    // ----------
>    // Function: getFavIconUrlForTab
>    // Gets fav icon url for the given xul:tab.
> +  getFavIconUrlForTab: function UI_getFavIconUrlForTab(tab, callback) {
> +    let self = this;

Please use .bind(this).

@@ +1648,5 @@
> +      if (isImageDoc) {
> +        callback(tab.pinned ? tab.image : null);
> +      } else {
> +        if (tab.image) {
> +          // if starts with http/https, fetch icon from favicon service via the moz-anno protocal

*protocol

@@ +1652,5 @@
> +          // if starts with http/https, fetch icon from favicon service via the moz-anno protocal
> +          if (/^https?:/.test(tab.image))
> +            callback(gFavIconService.getFaviconLinkForIcon(gWindow.makeURI(tab.image)).spec);
> +          else
> +            callback(tab.image);

Please use a variable here.

Comment 44

[Raymond Lee [:raymondlee]]

Attachment: v9

(In reply to Tim Taubert [:ttaubert] from comment #43)
> Comment on attachment 599922 [details] [diff] [review]
> v8
> 
> Review of attachment 599922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/tabview/content.js
> @@ +108,5 @@
> > +
> > +  // ----------
> > +  // Function: shouldLoadFavIcon
> > +  // Checks if fav icon should be loaded or not.
> > +  shouldLoadFavIcon: function WMH_shouldLoadFavIcon(cx) {
> 
> We don't need this to be in the content script. Accessing
> tab.linkedBrowser.currentURI is perfectly fine with e10s. Please make sure
> we cache those pref values to not query them every time this function is
> called.

Done

> 
> ::: browser/components/tabview/groupitems.js
> @@ +238,5 @@
> >    AllTabs.tabs.forEach(function(xulTab) {
> > +    if (xulTab.pinned) {
> > +      // only adjust tray when it's the last app tab.
> > +      self.addAppTab(xulTab,
> > +                     { adjustTrayForLastAppTab: true,
> 
> Couldn't you just use pinnedTabCount and _numPinnedTabs instead of
> introducing this new option?
> 

Fixed

> @@ +1196,2 @@
> >    addAppTab: function GroupItem_addAppTab(xulTab, options) {
> >      let self = this;
> 
> Please use .bind(this).
> 

Fixed

> @@ +2097,5 @@
> >    _updateAppTabIcons: function GroupItems__updateAppTabIcons(xulTab) {
> >      if (!xulTab.pinned)
> >        return;
> >  
> > +    let self = this;
> 
> Please use .bind(this).
> 

Fixed

> @@ +2100,5 @@
> >  
> > +    let self = this;
> > +    this.getAppTabFavIconUrl(xulTab, function GroupItems__updateAppTabIcons_getAppTabFavIconUrl(iconUrl) {
> > +      self.groupItems.forEach(function(groupItem) {
> > +        iQ(".appTabIcon", groupItem.$appTabTray).each(function(icon) {
> 
> Please just call iQ(".appTabIcon") and then iterate through all app tab
> icons. We need to check all of those elements but I think that's better than
> calling iQ() repeatedly for every groupItem.
> 

Fixed

> ::: browser/components/tabview/tabitems.js
> @@ +990,5 @@
> > +        if (iconUrl) {
> > +          if (tabItem.$favImage[0].src != iconUrl)
> > +            tabItem.$favImage[0].src = iconUrl;
> > +          iQ(tabItem.$fav[0]).show();
> > +          tabItem._sendToSubscribers("iconUpdated", { show: true });
> 
> Do we really need this additional data here? This is easy to query and no
> place needs this so far.
> 

No, removed the additional data.

> ::: browser/components/tabview/test/browser_tabview_bug678374.js
> @@ +25,5 @@
> > +        pinTab(newTabTwo, function() {
> > +          let icon = cw.iQ(".appTabIcon", groupItem.$appTabTray)[0];
> > +          is(icon.src, iconUrl, "The app tab is showing the right icon");
> > +
> > +          Services.prefs.clearUserPref("browser.chrome.favicons");
> 
> Please move this into a registerCleanupFunction() call to make sure we unset
> this pref again.

Moved

> 
> ::: browser/components/tabview/test/head.js
> @@ +377,5 @@
> >    pb.privateBrowsingEnabled = !pb.privateBrowsingEnabled;
> >  }
> > +
> > +// ----------
> > +function pinTab(xulTab, callback, win) {
> 
> The function name isn't really clear about it waiting for the app tab icon
> to be added. Can you please move all code except "gBrowser.pinTab()" to
> whenAppTabIconAdded() and use that in the tests? So we don't need this
> function and the tests are clear about what they're doing and waiting for.
> 
> @@ +389,5 @@
> > +  win.gBrowser.pinTab(xulTab);
> > +}
> > +
> > +// ----------
> > +function whenTabIsPinned(groupItem, callback) {
> 
> whenAppTabIconAdded() would be a better name for this.
> 

Updated

> ::: browser/components/tabview/ui.js
> @@ +1631,5 @@
> > +    mm.addMessageListener(message, function onMessage(cx) {
> > +      mm.removeMessageListener(cx.name, onMessage);
> > +      let url = null;
> > +      if (cx.json.shouldLoad)
> > +        url = gFavIconService.getFaviconImageForPage(tab.linkedBrowser.currentURI).spec;
> 
> You're fetching the favicon here and in getFavIconUrlForTab() again?

Removed

> 
> @@ +1641,5 @@
> >    // ----------
> >    // Function: getFavIconUrlForTab
> >    // Gets fav icon url for the given xul:tab.
> > +  getFavIconUrlForTab: function UI_getFavIconUrlForTab(tab, callback) {
> > +    let self = this;
> 
> Please use .bind(this).

Fixed

> 
> @@ +1648,5 @@
> > +      if (isImageDoc) {
> > +        callback(tab.pinned ? tab.image : null);
> > +      } else {
> > +        if (tab.image) {
> > +          // if starts with http/https, fetch icon from favicon service via the moz-anno protocal
> 
> *protocol
> 
> @@ +1652,5 @@
> > +          // if starts with http/https, fetch icon from favicon service via the moz-anno protocal
> > +          if (/^https?:/.test(tab.image))
> > +            callback(gFavIconService.getFaviconLinkForIcon(gWindow.makeURI(tab.image)).spec);
> > +          else
> > +            callback(tab.image);
> 
> Please use a variable here.

Updated

Comment 45

[Raymond Lee [:raymondlee]]

Attachment: v10

There is leak in patch v9.  Here is a new one and passed tests.

https://tbpl.mozilla.org/?tree=Try&rev=07b24b5d2ef7

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 603225 [details] [diff] [review]
v10

Review of attachment 603225 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/tabview/groupitems.js
@@ +237,5 @@
> +  let pinnedTabCount = gBrowser._numPinnedTabs;
> +  for (let i = 0; i < pinnedTabCount; i++) {
> +    // only adjust tray when it's the last app tab.
> +    this.addAppTab(gBrowser.tabs[i],
> +                   { adjustTrayForLastAppTab: true,

I actually meant that you could write something like this:

let pinnedTabCount = gBrowser._numPinnedTabs;
AllTabs.tabs.forEach(function (xulTab, index) {
  if (xulTab.pinned)
    this.addAppTab(xulTab, {dontAdjustTray: index + 1 < pinnedTabCount});
}, this);

That way we don't need to introduce the new option 'adjustTrayForLastAppTab' and don't overspecify the function, retain more control for the caller.

@@ +1228,5 @@
> +
> +      this._sendToSubscribers("appTabIconAdded", { item: $appTab });
> +    }
> +    GroupItems.getAppTabFavIconUrl(xulTab,
> +                                   GroupItem_addAppTab_getAppTabFavIconUrl.bind(this));

Nit: Why not

GroupItems.getAppTabFavIconUrl(xulTab, function (iconUrl) {
  ...
}.bind(this));

@@ +2105,2 @@
>  
> +         if (iconUrl != $icon.attr("src"))

We don't need a |return true| here. You can just combine those two if conditions.

@@ +2115,5 @@
>    // Function: getAppTabFavIconUrl
>    // Gets the fav icon url for app tab.
> +  getAppTabFavIconUrl: function GroupItems_getAppTabFavIconUrl(xulTab, callback) {
> +    UI.getFavIconUrlForTab(xulTab, function GroupItems_getAppTabFavIconUrl_getFavIconUrlForTab(iconUrl) {
> +      callback(iconUrl ? iconUrl : gFavIconService.defaultFavicon.spec);

Nit: callback(iconUrl || gFavIconService.defaultFavicon.spec);

::: browser/components/tabview/tabitems.js
@@ +989,5 @@
> +      UI.getFavIconUrlForTab(tab, function TabItems__update_getFavIconUrlCallback(iconUrl) {
> +        if (iconUrl) {
> +          if (tabItem.$favImage[0].src != iconUrl)
> +            tabItem.$favImage[0].src = iconUrl;
> +          iQ(tabItem.$fav[0]).show();

Please create variables for .$favImage[0] and .$fav[0] at the top of the callback.

@@ +990,5 @@
> +        if (iconUrl) {
> +          if (tabItem.$favImage[0].src != iconUrl)
> +            tabItem.$favImage[0].src = iconUrl;
> +          iQ(tabItem.$fav[0]).show();
> +          tabItem._sendToSubscribers("iconUpdated");

(see below)

@@ +995,5 @@
> +        } else {
> +          if (tabItem.$favImage[0].hasAttribute("src"))
> +            tabItem.$favImage[0].removeAttribute("src");
> +          iQ(tabItem.$fav[0]).hide();
> +          tabItem._sendToSubscribers("iconUpdated");

We don't need _sendToSubscribers() in both branches, please move it outside the if block.

::: browser/components/tabview/test/browser_tabview_apptabs.js
@@ +51,5 @@
> +          "there's an app tab icon in the second group");
> +
> +     // When the tab was pinned, the last active group with an item got the focus.
> +     // Therefore, switching the focus back to group item one.
> +     contentWindow.UI.setActive(groupItemOne);

Nit: indentation is off.

@@ +84,5 @@
> +
> +          finish();
> +        };
> +
> +        window.addEventListener("tabviewhidden", onTabViewHidden, false);

Nit: whenTabViewHidden(...)

@@ +87,5 @@
> +
> +        window.addEventListener("tabviewhidden", onTabViewHidden, false);
> +
> +        let appTabIcons = groupItemOne.container.getElementsByClassName("appTabIcon");
> +        EventUtils.sendMouseEvent({ type: "click" }, appTabIcons[0], contentWindow);

Please use showTabView() at the top of this file so that EventUtils doesn't fail because of missing focus.

::: browser/components/tabview/test/browser_tabview_bug595965.js
@@ +71,2 @@
>  
> +    let pinnedSomeTabs = function() {

Nit: function pinnedSomeTabs() {

::: browser/components/tabview/test/browser_tabview_bug610242.js
@@ +68,5 @@
>      }
>    }
>  
>    afterAllTabsLoaded(function() {
>      afterAllTabItemsUpdated(function() {

Do we even need afterAllTabsLoaded() and afterAllTabItemsUpdated()? Can't we just wait for "iconUpdated" because that means the tabs have been loaded and updated? We definitely can't rely on "iconUpdated" to be called first when all children have been updated, the order might be arbitrary.

::: browser/components/tabview/test/browser_tabview_bug678374.js
@@ +11,5 @@
> +    let newTabOne = win.gBrowser.tabs[1];
> +    let newTabTwo = win.gBrowser.tabs[2];
> +    let cw = win.TabView.getContentWindow();
> +    let groupItem = cw.GroupItems.groupItems[0];
> +    let iconUrl = "moz-anno:favicon:http://example.com/browser/browser/components/tabview/test/test_bug678374_icon16.png";

Please make that url a const at the top of the file.

@@ +15,5 @@
> +    let iconUrl = "moz-anno:favicon:http://example.com/browser/browser/components/tabview/test/test_bug678374_icon16.png";
> +
> +    // test tab item
> +    let newTabItemOne = newTabOne._tabViewTabItem;
> +    afterAllTabItemsUpdated(function() {

Do we need afterAllTabItemsUpdated()? Waiting for "iconUpdated" should be sufficient.

@@ +37,5 @@
> +      win.close(); 
> +   });
> +
> +    win.gBrowser.loadOneTab("http://example.com/browser/browser/components/tabview/test/test_bug678374.html");
> +    win.gBrowser.loadOneTab("http://example.com/browser/browser/components/tabview/test/test_bug678374.html");

Please make those urls consts at the top of the file.

::: browser/components/tabview/ui.js
@@ +147,5 @@
> +  // Used to keep track of the browser.chrome.favicons pref value.
> +  _prefFavicons: null,
> +
> +  // Used to keep track of the preference observer.
> +  _prefObserver: null,

We don't need this (see below).

@@ +867,5 @@
>    // ----------
> +  // Function: _addPrefObservers
> +  // Adds preference observers.
> +  _addPrefObservers: function UI__addPrefObservers() {
> +    this._prefObserver = function UI__addPrefObservers_prefObserver(subject, topic, data) {

Please let UI implement an "observe" method so that we don't need to track an extra observer function.

@@ +1667,5 @@
> +  // ----------
> +  // Function: _shouldLoadFavIcon
> +  // Checks whether fav icon should be loaded for a given tab.
> +  _shouldLoadFavIcon: function UI__shouldLoadFavIcon(tab) {
> +    let uri = tab.linkedBrowser.currentURI;

Let's just return false if (!uri) so we don't need to read the prefs.

@@ +1669,5 @@
> +  // Checks whether fav icon should be loaded for a given tab.
> +  _shouldLoadFavIcon: function UI__shouldLoadFavIcon(tab) {
> +    let uri = tab.linkedBrowser.currentURI;
> +
> +    if (!this._prefSiteIcons) {

You're re-reading the pref value when it's false.

|if (this._prefSiteIcons === null)|

Additionally we wouldn't need to read the second pref's value if this is false and we return early.

@@ +1673,5 @@
> +    if (!this._prefSiteIcons) {
> +      this._prefSiteIcons =
> +        Services.prefs.getBoolPref("browser.chrome.site_icons");
> +    }
> +    if (!this._prefFavicons) {

Same here.

@@ +1696,5 @@
> +          // if starts with http/https, fetch icon from favicon service via the moz-anno protocal
> +          if (/^https?:/.test(tabImage))
> +            callback(gFavIconService.getFaviconLinkForIcon(gWindow.makeURI(tab.image)).spec);
> +          else
> +            callback(tabImage);

Nit:

if (/^https?:/.test(tabImage))
  tabImage = gFavIconService.getFaviconLinkForIcon(gWindow.makeURI(tab.image)).spec;

callback(tabImage);

@@ +1710,2 @@
>      }
> +    this._isImageDocument(tab,  UI_getFavIconUrlForTab_isImageDocument.bind(this));

Nit: Why not just

this._isImageDocument(tab, function (isImageDoc) {
  ...
}.bind(this));

like we do in all the other places?

Comment 47

[Raymond Lee [:raymondlee]]

Attachment: v11

> ::: browser/components/tabview/test/browser_tabview_bug610242.js
> @@ +68,5 @@
> >      }
> >    }
> >  
> >    afterAllTabsLoaded(function() {
> >      afterAllTabItemsUpdated(function() {
> 
> Do we even need afterAllTabsLoaded() and afterAllTabItemsUpdated()? Can't we
> just wait for "iconUpdated" because that means the tabs have been loaded and
> updated? We definitely can't rely on "iconUpdated" to be called first when
> all children have been updated, the order might be arbitrary.

I have tried to remove one of them and both, the test is either timeout and fails. We have to make sure that all tabs and tab items have been loaded first.

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 603325 [details] [diff] [review]
v11

Review of attachment 603325 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for fixing this!

r=me - with nits and comments fixed, and the question answered

::: browser/components/tabview/groupitems.js
@@ +1187,5 @@
>    //   options - change how the app tab is added.
>    //
>    // Options:
> +  //   position - the position of the app tab should be added to.
> +  //   adjustTrayForLastAppTab - (boolean) if true, adjustAppTabTray() should 

Please revert the documentation here do "dontAdjustTray".

@@ +1211,2 @@
>  
> +      if (options && options.position != null) {

if (options && "position" in options)

otherwise one wouldn't be able to inject a tab into the first position (index=0).

@@ +1214,2 @@
>  
> +        if (children.length == 0 || options.position >= children.length)

The second condition would always be false when "children.length == 0" so we don't need the first one here.

::: browser/components/tabview/tabitems.js
@@ +988,4 @@
>        // ___ icon
> +      UI.getFavIconUrlForTab(tab, function TabItems__update_getFavIconUrlCallback(iconUrl) {
> +        let favImage = tabItem.$favImage[0];
> +        let fav = tabItem.$fav[0];

let fav = tabItem.$fav;

@@ +991,5 @@
> +        let fav = tabItem.$fav[0];
> +        if (iconUrl) {
> +          if (favImage.src != iconUrl)
> +            favImage.src = iconUrl;
> +          iQ(fav).show();

We don't need iQ() anymore (see above).

@@ +995,5 @@
> +          iQ(fav).show();
> +        } else {
> +          if (favImage.hasAttribute("src"))
> +            favImage.removeAttribute("src");
> +          iQ(fav).hide();

(see above)

::: browser/components/tabview/test/browser_tabview_apptabs.js
@@ +11,3 @@
>    ok(TabView.isVisible(), "Tab View is visible");
>  
>    let contentWindow = document.getElementById("tab-view").contentWindow;

Nit: While you're at it - TabView.getContentWindow().

::: browser/components/tabview/test/browser_tabview_bug610242.js
@@ +66,5 @@
>      }
>    }
>  
>    afterAllTabsLoaded(function() {
>      afterAllTabItemsUpdated(function() {

You said you tried removing those and it failed. I tried locally and it worked. We still can't ensure that the first icon is updated *after* every tabItem has been loaded and updated. That might become another orange if we leave it like that.

::: browser/components/tabview/ui.js
@@ +248,5 @@
>        this._addTabActionHandlers();
>  
> +      // ___ add preference observers
> +      Services.prefs.addObserver("browser.chrome.site_icons", this, false);
> +      Services.prefs.addObserver("browser.chrome.favicons", this, false);

Please make those preference names constants and define those at the top of the file.

@@ +1671,5 @@
> +        Services.prefs.getBoolPref("browser.chrome.favicons");
> +
> +    return (uri &&
> +            this._prefSiteIcons && this._prefFavicons &&
> +            ("schemeIs" in uri) && (uri.schemeIs("http") || uri.schemeIs("https")));

We already checked "uri" and "this._prefSiteIcons" some lines above and thus can remove them from the condition.

Comment 49

[Raymond Lee [:raymondlee]]

Attachment: v12

(In reply to Tim Taubert [:ttaubert] from comment #48)
> Comment on attachment 603325 [details] [diff] [review]
> v11
> 
> Review of attachment 603325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for fixing this!
> 
> r=me - with nits and comments fixed, and the question answered
> 

I have fixed all the nits and comments, except the below.

> ::: browser/components/tabview/test/browser_tabview_bug610242.js
> @@ +66,5 @@
> >      }
> >    }
> >  
> >    afterAllTabsLoaded(function() {
> >      afterAllTabItemsUpdated(function() {
> 
> You said you tried removing those and it failed. I tried locally and it
> worked. We still can't ensure that the first icon is updated *after* every
> tabItem has been loaded and updated. That might become another orange if we
> leave it like that.
> 

After removing those two lines, it didn't work for me locally if I ran the test only.  The problem is that the iconUpdated is fired for a new empty tab before the actual url is loaded, therefore, I have added a condition to ensure the desired url is loaded before do the checking. 


+  children.forEach(function(tabItem) {
+    tabItem.addSubscriber("iconUpdated", function onIconUpdated() {
+      // the tab is not loaded completely so ignore it.
+      if (tabItem.tab.linkedBrowser.currentURI.spec == "about:blank")
+        return;

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 605690 [details] [diff] [review]
v12

Review of attachment 605690 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks! That looks like a good solution to me. Can you please push that to try before marking it as checkin-needed?

Comment 51

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Passed Try.
https://tbpl.mozilla.org/?tree=Try&rev=4c1f93ba7abe

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/e7f495b11aa6

Comment 53

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e7f495b11aa6