Last Comment Bug 554908 - Make site icons work in places history and bookmarks
: Make site icons work in places history and bookmarks
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: seamonkey2.1a2
Assigned To: Robert Kaiser
:
Mentors:
: 570791 (view as bug list)
Depends on: SMPlacesBMarks 558673
Blocks: SMtabAPI 570791 585511 585515
  Show dependency treegraph
 
Reported: 2010-03-25 05:42 PDT by Robert Kaiser
Modified: 2010-08-08 18:21 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP1: set icons in the places database (6.53 KB, patch)
2010-04-02 18:29 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
(Bv1) Copy just getIcon() from Firefox [Moved to bug 558673] (999 bytes, patch)
2010-04-10 21:02 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
Put setIcon in the same place as Firefox, add updateIcon (6.41 KB, patch)
2010-06-19 07:31 PDT, Robert Kaiser
iann_bugzilla: review-
Details | Diff | Splinter Review
v2: address review comments (6.48 KB, patch)
2010-06-21 12:16 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v2.1: address review comments, correct arguments for listeners (6.56 KB, patch)
2010-06-21 12:53 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
use .linkedBrowser (6.62 KB, patch)
2010-06-22 06:13 PDT, Robert Kaiser
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-03-25 05:42:13 PDT
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) 2010-03-29 10:41:14 PDT
Version = 2.0 branch?! ;-)
Comment 2 Robert Kaiser 2010-03-29 11:50:57 PDT
(In reply to comment #1)
> Version = 2.0 branch?! ;-)

Umm, my daily work build gave away its data... *blush*
Comment 3 Robert Kaiser 2010-04-02 18:29:10 PDT
Created attachment 436817 [details] [diff] [review]
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) 2010-04-10 21:02:20 PDT
Created attachment 438306 [details] [diff] [review]
(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 2010-04-11 03:36:17 PDT
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] 2010-06-08 13:41:04 PDT
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 2010-06-08 18:23:00 PDT
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] 2010-06-08 18:26:21 PDT
Then I guess the dependencies between this bug and bug bug 570791 should be reversed.
Comment 9 Robert Kaiser 2010-06-19 07:31:10 PDT
Created attachment 452463 [details] [diff] [review]
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 2010-06-19 15:19:10 PDT
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 2010-06-21 11:52:48 PDT
(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 2010-06-21 12:16:57 PDT
Created attachment 452812 [details] [diff] [review]
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 2010-06-21 12:22:29 PDT
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 2010-06-21 12:48:22 PDT
(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 2010-06-21 12:53:36 PDT
Created attachment 452821 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-06-22 03:20:11 PDT
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 2010-06-22 06:13:57 PDT
Created attachment 453046 [details] [diff] [review]
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 2010-06-22 07:57:20 PDT
Note to self: If bug 573733 lands before this, the .mTabs reference should become .tabs instead.
Comment 19 Ian Neal 2010-06-22 17:19:43 PDT
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 2010-06-22 18:15:34 PDT
(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 2010-06-22 18:18:46 PDT
Pushed as http://hg.mozilla.org/comm-central/rev/ff01648cfcf7
Comment 22 Robert Kaiser 2010-06-22 18:20:26 PDT
*** Bug 570791 has been marked as a duplicate of this bug. ***
Comment 23 neil@parkwaycc.co.uk 2010-06-23 01:11:21 PDT
(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 neil@parkwaycc.co.uk 2010-06-23 01:14:12 PDT
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 2010-06-25 06:23:03 PDT
(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 neil@parkwaycc.co.uk 2010-06-30 13:28:05 PDT
(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 2010-07-22 15:44:35 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.