554908 - Make site icons work in places history and bookmarks

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#610 is where Firefox loads icons into places to be used in bookmarks and history, we should have our own call of FaviconService.setAndLoadFaviconForPage() to get those working as well.

This intentionally is a followup to bug 498596 as the patchsets there are already fairly large.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Version = 2.0 branch?! ;-)

Comment 2

[Robert Kaiser]

(In reply to comment #1)
> Version = 2.0 branch?! ;-)

Umm, my daily work build gave away its data... *blush*

Comment 3

[Robert Kaiser]

Attachment: WIP1: set icons in the places database

After fighting a bit with the differences between our and FF's tabbrowser implementations, here's a first WIP that at least sets the icons in the places DB via faviconservice in some way.

IMHO, we should fully convert over to faviconservice, but I don't think I'll do that, there's too much code that is quite different from FF and hard to understand for me.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Copy just getIcon() from Firefox [Moved to bug 558673]

This is the same part as in KaiRo's patch WIP1.
This doesn't (fully) fix, but it helps, bug 556684.

Comment 5

[Robert Kaiser]

Comment on attachment 438306 [details] [diff] [review]
(Bv1) Copy just getIcon() from Firefox
[Moved to bug 558673]

Sorry, please file an own bug for this, this patch doesn't make icons work with places bookmarks like that bug says (actually it doesn't even help anything for that, I had just added it in the patch for convenience).

Comment 6

[Michael Kraft [:morac]]

To get the this.mTabsProgressListeners stuff working for setIcon, I would recommend doing the following:

Move the code that calls the onLinkIconAvailable callback functions from setIcon (line 653 to 675) into the get mTabProgressListener() object.  You could call it onLinkIconAvailable and make it look like the existing callback functions already there (keep in mind bug 570783). So something like this:

             onLinkIconAvailable : function()
             {
               if (this.mTabBrowser.mCurrentTab == this.mTab) {
                 this.mTabBrowser.mProgressListeners.forEach(
                   function notifyLinkIconAvailable(element) {
                     try {
                       element.onLinkIconAvailable(this.mBrowser, this.mBrowser.mIconURL);
                     } catch (e) {
                       Components.utils.reportError(e);
                     }
                   }
                 );
               }
 
              this.mTabBrowser.mTabsProgressListeners.forEach(
                 function notifyLinkIconAvailable(element) {
                   try {
                     element.onLinkIconAvailable(this.mBrowser, this.mBrowser.mIconURL);
                   } catch (e) {
                     Components.utils.reportError(e);
                   }
                 }
               , this);
             },



Then After the "this.updateIcon(aTab)" call in setIcon, add the following:

this.mTabListeners[gBrowser.getTabIndex(aTab)].onLinkIconAvailable();

Comment 7

[Robert Kaiser]

implementing onLinkIconAvailable is out of the scope of this bug, but calling it if it's available is a good idea to be done here.

Comment 8

[Michael Kraft [:morac]]

Then I guess the dependencies between this bug and bug bug 570791 should be reversed.

Comment 9

[Robert Kaiser]

Attachment: Put setIcon in the same place as Firefox, add updateIcon

This patch has been working nicely for me and users of the places bookmarks try builds for some time now, so I'll just put it up for review.

I haven't tested it yet in that configuration, but I think it should even work nicely without the places bookmarks patches and just give us icons in the history window (next to better conforming with the Firefox API).

Comment 10

[Ian Neal]

Comment on attachment 452463 [details] [diff] [review]
Put setIcon in the same place as Firefox, add updateIcon

>+            if (browser == this.mCurrentBrowser) {
>+              for (let i = 0; i < this.mProgressListeners.length; i++) {
Can we use forEach here instead, see http://mxr.mozilla.org/comm-central/source/suite/browser/tabbrowser.xml#852
>+                let p = this.mProgressListeners[i];
>+                if ('onLinkIconAvailable' in p)
>+                  try {
>+                    p.onLinkIconAvailable(browser, browser.mIconURL);
Suite's onLinkIconAvailable only takes one argument - browser.mIconURL
>+                  } catch (e) {
>+                    // don't inhibit other listeners
>+                    Components.utils.reportError(e);
>+                  }
>+              }
>+            }
>+            for (let i = 0; i < this.mTabsProgressListeners.length; i++) {
Can we use forEach here instead.
>+              let p = this.mTabsProgressListeners[i];
>+              if ('onLinkIconAvailable' in p)
>+                try {
>+                  p.onLinkIconAvailable(browser, browser.mIconURL);
As above, only one argument.
>+                } catch (e) {
>+                  // don't inhibit other listeners
>+                  Components.utils.reportError(e);
>+                }
>+            }

>-                this.mTabListeners[i].setIcon(href);
>+                this.setIcon(this.mTabs[i], href);
>                 if (this.browsers[i] == this.mCurrentBrowser) {
>                   this.mProgressListeners.forEach(
>                     function notifyLinkIconAvailable(element) {
>                       if ("onLinkIconAvailable" in element) {
>                         try {
>                           element.onLinkIconAvailable(href);
>                         } catch (e) {
>                           Components.utils.reportError(e);
Do we still need this bit, isn't it now done in this.setIcon?

Comment 11

[Robert Kaiser]

(In reply to comment #10)
> >+                    p.onLinkIconAvailable(browser, browser.mIconURL);
> Suite's onLinkIconAvailable only takes one argument - browser.mIconURL

Hmm, I wonder if that is actually bug - or does this not matter from a API-comat POV?

Comment 12

[Robert Kaiser]

Attachment: v2: address review comments

I filed bug 573534 on the arguments, trying first if the additional argument in Firefox has any value - if it has, we probably should look into changing our side.

The patch now addresses the review comments from last round.

Comment 13

[Robert Kaiser]

Oh, and I tested this patch now with a non-place-bookmarks build, seeing that it correctly makes icons appear in the history window.

Comment 14

[Robert Kaiser]

(In reply to comment #10)
> >+                  p.onLinkIconAvailable(browser, browser.mIconURL);
> As above, only one argument.

Actually, I just found out in bug 573534 and bug 519216, which fixes the former, that this comment of yours is actually wrong. the _tab_ progress listener needs the browser as the first argument, the other one doesn't have or need it (and that is the one we are actually implementing, no tab progress listener is implemented in our code right now).

I'll attach another patch with that part fixed/reverted.

Comment 15

[Robert Kaiser]

Attachment: v2.1: address review comments, correct arguments for listeners

This patch calls both listeners with the correct arguments - I added a comment to point to the difference. :)

Comment 16

[[e-mail]]

Comment on attachment 452821 [details] [diff] [review]
v2.1: address review comments, correct arguments for listeners

>+            var browser = this.getBrowserForTab(aTab);
getBrowserForTab only exists to support porting Firefox extensions;
tabbrowser.xml code should use .linkedBrowser internally.

>             let browser = aTab ? this.getBrowserForTab(aTab) : this.selectedBrowser;
(Except here where Callek stole my review.)

Comment 17

[Robert Kaiser]

Attachment: use .linkedBrowser

This patch uses .linkedBrowser and also fixes that case of .getBrowserForTab(aTab) mentioned by Neil, as that's in getIcon which is related to this anyhow.

Comment 18

[Robert Kaiser]

Note to self: If bug 573733 lands before this, the .mTabs reference should become .tabs instead.

Comment 19

[Ian Neal]

Comment on attachment 453046 [details] [diff] [review]
use .linkedBrowser

r=me with mTabs to tabs change.
Do we have a bug to port bug 519216?

Comment 20

[Robert Kaiser]

(In reply to comment #19)
> Do we have a bug to port bug 519216?

We surely can, and I think it probably will make sense to do so - the part of it that changes onLinkIconAvailable is something we have already in SeaMonkey, but the rest might be interesting to do for us as well.

Comment 21

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/ff01648cfcf7

Comment 23

[[e-mail]]

(In reply to comment #19)
> Do we have a bug to port bug 519216?
No, that's actually a port of our fix, as usual ;-)

Comment 24

[[e-mail]]

Comment on attachment 453046 [details] [diff] [review]
use .linkedBrowser

>+                aURI = makeURI(aURI);
This introduced a dependency on navigator.xul (well contentAreaUtils.js) :-(

>+                this.setIcon(this.mTabs[i], href);
This would work better with s/href/uri/, thus avoiding making a new one.

Comment 25

[Robert Kaiser]

(In reply to comment #24)
> (From update of attachment 453046 [details] [diff] [review])
> >+                aURI = makeURI(aURI);
> This introduced a dependency on navigator.xul (well contentAreaUtils.js) :-(

On contentAreaUtils.js, right, but that's also loaded in messenger.xul - are we using or supposed to use tabbrowser anywhere else?

> >+                this.setIcon(this.mTabs[i], href);
> This would work better with s/href/uri/, thus avoiding making a new one.

Should I push that as a supplemental fix with r=you?

Comment 26

[[e-mail]]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 453046 [details] [diff] [review] [details])
> > >+                aURI = makeURI(aURI);
> > This introduced a dependency on navigator.xul (well contentAreaUtils.js) :-(
> On contentAreaUtils.js, right, but that's also loaded in messenger.xul - are we
> using or supposed to use tabbrowser anywhere else?
No, but I thought tabbrowser wasn't dependent on any external scripts before.

> > >+                this.setIcon(this.mTabs[i], href);
> > This would work better with s/href/uri/, thus avoiding making a new one.
> Should I push that as a supplemental fix with r=you?
Sure, if you like.

Comment 27

[Robert Kaiser]

(In reply to comment #26)
> > > >+                this.setIcon(this.mTabs[i], href);
> > > This would work better with s/href/uri/, thus avoiding making a new one.
> > Should I push that as a supplemental fix with r=you?
> Sure, if you like.

Finally done as http://hg.mozilla.org/comm-central/rev/cf0bd5da19a1