Make site icons work in places history and bookmarks

RESOLVED FIXED in seamonkey2.1a2

Status

SeaMonkey
Bookmarks & History
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.1a2
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
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.
Version = 2.0 branch?! ;-)
(Assignee)

Comment 2

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

Umm, my daily work build gave away its data... *blush*
Version: SeaMonkey 2.0 Branch → unspecified
(Assignee)

Comment 3

7 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED

Updated

7 years ago
Blocks: 556684
(Assignee)

Updated

7 years ago
No longer blocks: 556684
Blocks: 467867
Version: unspecified → Trunk
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.
Attachment #438306 - Flags: superreview?(neil)
Attachment #438306 - Flags: review?(neil)
(Assignee)

Comment 5

7 years ago
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).
Attachment #438306 - Attachment is obsolete: true
Attachment #438306 - Flags: superreview?(neil)
Attachment #438306 - Flags: review?(neil)
Attachment #438306 - Attachment description: (Bv1) Copy just getIcon() from Firefox → (Bv1) Copy just getIcon() from Firefox [Moved to bug 558673]
(Assignee)

Updated

7 years ago
No longer blocks: 467867
Depends on: 558673
(Assignee)

Updated

7 years ago
Blocks: 467867

Updated

7 years ago
Blocks: 570791
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();
(Assignee)

Comment 7

7 years ago
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.
Then I guess the dependencies between this bug and bug bug 570791 should be reversed.
(Assignee)

Comment 9

7 years ago
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).
Attachment #436817 - Attachment is obsolete: true
Attachment #452463 - Flags: review?(iann_bugzilla)

Comment 10

7 years ago
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?
Attachment #452463 - Flags: review?(iann_bugzilla) → review-
(Assignee)

Comment 11

7 years ago
(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?
(Assignee)

Comment 12

7 years ago
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.
Attachment #452463 - Attachment is obsolete: true
Attachment #452812 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 13

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

Comment 14

7 years ago
(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.
(Assignee)

Comment 15

7 years ago
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. :)
Attachment #452812 - Attachment is obsolete: true
Attachment #452821 - Flags: review?(iann_bugzilla)
Attachment #452812 - Flags: review?(iann_bugzilla)
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.)
(Assignee)

Comment 17

7 years ago
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.
Attachment #452821 - Attachment is obsolete: true
Attachment #453046 - Flags: review?(iann_bugzilla)
Attachment #452821 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 18

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

Comment 19

7 years ago
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?
Attachment #453046 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 20

7 years ago
(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.
(Assignee)

Comment 21

7 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/ff01648cfcf7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a2
(Assignee)

Updated

7 years ago
Duplicate of this bug: 570791
(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 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.
(Assignee)

Comment 25

7 years ago
(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?
(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.
(Assignee)

Comment 27

7 years ago
(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
(Assignee)

Updated

7 years ago
Blocks: 585511
(Assignee)

Updated

7 years ago
Blocks: 585515
You need to log in before you can comment on or make changes to this bug.