751922 - Asynchronously add favicons to back/forward and history menus.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, enhancement)

Description

[Philip Chee]

This ports the relevant parts of:
Bug 389478 - Add favicons on back/forward menus.
Bug 544762 - Implement menuitem-with-favicon class in toolkit and use it in appropriate places.
Bug 737133 - getFaviconURLForPage and getFaviconDataForPage should invoke nsIFaviconDataCallback even if the favicon is not available.
Bug 728141 - Replace old synchronous favicons calls in browser.

Comment 1

[Philip Chee]

Attachment: Patch v1.0

I took the opportunity to reformat the code so that it is closer to current style guidelines since I was touching so much of the code anyway.

Comment 2

[Philip Chee]

Attachment: Patch v1.0

I took the opportunity to reformat the code so that it is closer to current style guidelines since I was touching so much of the code anyway.

Comment 3

[Philip Chee]

Attachment: Patch v1.0w hg qdiff -pwbU8

hg qdiff -w as requested.

Comment 4

[[e-mail]]

Comment on attachment 621084 [details] [diff] [review]
Patch v1.0w hg qdiff -pwbU8

As you're editing these, might as well get rid of superflous ()s.

>+      end = (index > MAX_HISTORY_MENU_ITEMS) ?
>+                     index - MAX_HISTORY_MENU_ITEMS : 0;
Nit: poor wrapping. [x2; in the third case, no wrapping...]

>+      if ((index - 1) < end)
Nit: if (index <= end) [or >= end below]

>+      var end = count > MAX_HISTORY_MENU_ITEMS ? count - MAX_HISTORY_MENU_ITEMS : 0;
Redeclaration of var end. [Did you mean to use 3xlet?]

>+  menuitem.setAttribute("label", aEntry.title || aEntry.URI.spec);
nsSHEntry::GetTitle already falls back on the spec, so no point doing it. [x2]

>+      if (aURI) {
>+        let iconURL = PlacesUtils.favicons.getFaviconLinkForIcon(aURI).spec;
>+        menuitem.style.listStyleImage = "url(" + iconURL + ")";
>+      }
if (aURI)
  menuitem.setAttribute("image", ...spec); [x2]

>+  menuitem.className = "menuitem-iconic menuitem-with-favicon";
Duplicate?

>+  if (aChecked)
>+  {
>     menuitem.setAttribute("type", "radio");
>       menuitem.setAttribute("checked", "true");
>+  }
>+  else
>+  {
>+    menuitem.className = "menuitem-iconic menuitem-with-favicon";
>+    PlacesUtils.favicons.getFaviconURLForPage(aEntry.URI,
Annoyingly, this would actually work on the Mac because it supports both image and checkmark on the same menuitem. Ask stefanh if he wants a followup?

Alternatively, perhaps we should switch to using default="true"? Having one radio menuitem in a sea of favicons does look odd...

Comment 5

[[e-mail]]

(In reply to comment #4)
> (From update of attachment 621084 [details] [diff] [review])
> >+        let iconURL = PlacesUtils.favicons.getFaviconLinkForIcon(aURI).spec;
> >+        menuitem.style.listStyleImage = "url(" + iconURL + ")";
>   menuitem.setAttribute("image", ...spec); [x2]
Firefox only uses listStyleImage because they want the skin to be able to override it for some hover effect to match IE...

Comment 6

[Philip Chee]

>>+      if (aURI) {
>>+        let iconURL = PlacesUtils.favicons.getFaviconLinkForIcon(aURI).spec;
>>+        menuitem.style.listStyleImage = "url(" + iconURL + ")";
>>+      }
> if (aURI)
>   menuitem.setAttribute("image", ...spec); [x2]
Hmm. In Bug 414366 Firefox changed from setAttribute("image",...) to style.listStyleImage. this allows the [moz-menuactive] menu item to show a direction indicator (<,>) instead of the favicon. I haven't ported that bit yet so I'll revert that change.

>>+  menuitem.className = "menuitem-iconic menuitem-with-favicon";
> Duplicate?
AFAIK In xul.css .menuitem-iconic turns on the left icon image. Then MOZ_WIDGET_GTK + (-moz-images-in-menus) turns off images in menus. Then .menuitem-with-favicon overrides the GTK Theme override.

Comment 7

[Philip Chee]

Attachment: Patch v1.1 Fix issues.

Comment 8

[Philip Chee]

Attachment: Patch v1.1w hg qdiff -pwbU8

> As you're editing these, might as well get rid of superflous ()s.
> 
>>+      end = (index > MAX_HISTORY_MENU_ITEMS) ?
>>+                     index - MAX_HISTORY_MENU_ITEMS : 0;
> Nit: poor wrapping. [x2; in the third case, no wrapping...]
Fixed.

>>+      if ((index - 1) < end)
> Nit: if (index <= end) [or >= end below]
Fixed.

>>+      var end = count > MAX_HISTORY_MENU_ITEMS ? count - MAX_HISTORY_MENU_ITEMS : 0;
> Redeclaration of var end. [Did you mean to use 3xlet?]
Fixed. I tried let but kept getting redeclaration of let so switched back to var but had too much search and replace.

>>+  menuitem.setAttribute("label", aEntry.title || aEntry.URI.spec);
> nsSHEntry::GetTitle already falls back on the spec, so no point doing it. [x2]
Fixed.

>>+      if (aURI) {
>>+        let iconURL = PlacesUtils.favicons.getFaviconLinkForIcon(aURI).spec;
>>+        menuitem.style.listStyleImage = "url(" + iconURL + ")";
>>+      }
> if (aURI)
>   menuitem.setAttribute("image", ...spec); [x2]
Hmm. In Bug 414366 Firefox changed from setAttribute("image",...) to style.listStyleImage. this allows the [moz-menuactive] menu item to show a direction indicator (<,>) instead of the favicon. I haven't ported that bit yet so I'll revert that change.
Fixed.

>>+  menuitem.className = "menuitem-iconic menuitem-with-favicon";
> Duplicate?
Ah oops.

>>+  if (aChecked)
>>+  {
>>     menuitem.setAttribute("type", "radio");
>>       menuitem.setAttribute("checked", "true");
>>+  }
>>+  else
>>+  {
>>+    menuitem.className = "menuitem-iconic menuitem-with-favicon";
>>+    PlacesUtils.favicons.getFaviconURLForPage(aEntry.URI,
> Annoyingly, this would actually work on the Mac because it supports both image and checkmark on the same menuitem. Ask stefanh if he wants a followup?
> 
> Alternatively, perhaps we should switch to using default="true"? Having one radio menuitem in a sea of favicons does look odd...
Sounds good. I'll cc stefanh on this. Ping Stefan!

Comment 9

[[e-mail]]

(In reply to Philip Chee from comment #8)
>(From update of attachment 621249 [details] [diff] [review])
>>>+      var end = count > MAX_HISTORY_MENU_ITEMS ? count - MAX_HISTORY_MENU_ITEMS : 0;
>> Redeclaration of var end. [Did you mean to use 3xlet?]
>Fixed. I tried let but kept getting redeclaration of let so switched back to
>var but had too much search and replace.
Ah yes, case: doesn't start a new block, so let is equivalent to var here.

> Hmm. In Bug 414366 Firefox changed from setAttribute("image",...) to
> style.listStyleImage. this allows the [moz-menuactive] menu item to show a
> direction indicator (<,>) instead of the favicon. I haven't ported that bit
> yet so I'll revert that change.
That's what I was trying to say earlier. (I'm not sure we need that change.)

Comment 10

[[e-mail]]

Comment on attachment 621249 [details] [diff] [review]
Patch v1.1w hg qdiff -pwbU8

r=me once we come to an agreement as to how to display the current page in the Go menu, of out of at least four options:
1. Always just show a radio checkmark.
2. Always just show a favicon, but make the menuitem bold.
3. Show the radio checkmark, but on the Mac show the favicon too.
4. Always show the radio checkmark, the favicon and make the menuitem bold
   (this only works on the Mac; Windows/Linux gets 2. instead).
5. ???
6. Profit!

>+      end  = (count - index) > MAX_HISTORY_MENU_ITEMS ? index + MAX_HISTORY_MENU_ITEMS
Nit: can actually get rid of more ()s here, also double space before =

Comment 11

[Stefan [:stefanh]]

Comment on attachment 621249 [details] [diff] [review]
Patch v1.1w hg qdiff -pwbU8

As for the current page, I'd expect both favicon and checkmark. Making the label bold isn't really a mac option. But I'm not sure you would want it on windows/linux if you have the checkmark?

I've noticed one issue: we don't seem to load the default favicon.

Comment 12

[Stefan [:stefanh]]

Attachment: missing default favicon

(In reply to Stefan [:stefanh] from comment #11)
 
> I've noticed one issue: we don't seem to load the default favicon.

Comment 13

[Philip Chee]

Attachment: Patch v1.2 Fix Mac and default favicon.

> As for the current page, I'd expect both favicon and checkmark. Making the
> label bold isn't really a mac option. But I'm not sure you would want it
> on windows/linux if you have the checkmark?

So I've gone for:
> 3. Show the radio checkmark, but on the Mac show the favicon too.
Since the current page already has a favicon on the tab and on the urlbar.

> I've noticed one issue: we don't seem to load the default favicon.
Fixed using class .bookmark-item.

Comment 14

[Philip Chee]

Attachment: Patch v1.2w hg qdiff -pwbU8

> As for the current page, I'd expect both favicon and checkmark. Making the
> label bold isn't really a mac option. But I'm not sure you would want it
> on windows/linux if you have the checkmark?

So I've gone for:
> 3. Show the radio checkmark, but on the Mac show the favicon too.
Since the current page already has a favicon on the tab and on the urlbar.

> I've noticed one issue: we don't seem to load the default favicon.
Fixed using class .bookmark-item.

Comment 15

[[e-mail]]

Comment on attachment 624099 [details] [diff] [review]
Patch v1.2w hg qdiff -pwbU8

>+      end = count - index > MAX_HISTORY_MENU_ITEMS ? index + MAX_HISTORY_MENU_ITEMS
>+                                                      : count - 1;
Nit: align : with ?

>+function createGoMenuItem(aParent, aIndex, aEntry, aAnchor, aChecked)
Nit: wrong name, because we now use it for back/forward items too.

>+    menuitem.className = "bookmark-item menuitem-iconic menuitem-with-favicon";
Nit: "menuitem-iconic bookmark-item menuitem-with-favicon"

>+  if (aAnchor)
>     aParent.insertBefore(menuitem, aAnchor);
>+  else
>+    aParent.appendChild(menuitem);
Nit: Don't need to do this, .appendChild(foo) == .insertBefore(foo, null).

Comment 16

[Philip Chee]

Attachment: Patch v1.2a as checked-in r=Neil

>>+      end = count - index > MAX_HISTORY_MENU_ITEMS ? index + MAX_HISTORY_MENU_ITEMS
>>+                                                      : count - 1;
> Nit: align : with ?
Fixed.

>>+function createGoMenuItem(aParent, aIndex, aEntry, aAnchor, aChecked)
> Nit: wrong name, because we now use it for back/forward items too.
Now using creatHistoryMenuItem.

>>+    menuitem.className = "bookmark-item menuitem-iconic menuitem-with-favicon";
> Nit: "menuitem-iconic bookmark-item menuitem-with-favicon"
Fixed.

>>+  if (aAnchor)
>>     aParent.insertBefore(menuitem, aAnchor);
>>+  else
>>+    aParent.appendChild(menuitem);
> Nit: Don't need to do this, .appendChild(foo) == .insertBefore(foo, null).
Fixed.

Comment 17

[Philip Chee]

Oops. I forgot that I need Stefanh's Mac review as well before pushing. I'll fix any nits in a follow up patch.

Comment 18

[Stefan [:stefanh]]

Hmm, I don't see the favicon for the current page in the Go menu... But I think this is because the part of the widget code that deals with icons in menuitems doesn't handle the checked case: http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#168 (at least from the comment).

Comment 19

[Stefan [:stefanh]]

Comment on attachment 624099 [details] [diff] [review]
Patch v1.2w hg qdiff -pwbU8

I think you can close this, I'll see what we can do about the mac problem.

Comment 20

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #18)
> Hmm, I don't see the favicon for the current page in the Go menu... But I
> think this is because the part of the widget code that deals with icons in
> menuitems doesn't handle the checked case:
> http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.
> mm#168 (at least from the comment).

So, the code doesn't set an icon for checkbox/radio menuitems and menuseparators.

Comment 21

[Philip Chee]

comm-central changeset 62fef30b1453
http://hg.mozilla.org/comm-central/rev/62fef30b1453