Last Comment Bug 751922 - Asynchronously add favicons to back/forward and history menus.
: Asynchronously add favicons to back/forward and history menus.
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 1106274
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-04 09:29 PDT by Philip Chee
Modified: 2014-11-29 09:59 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 (7.59 KB, patch)
2012-05-04 09:36 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.0 (7.59 KB, patch)
2012-05-04 09:42 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.0w hg qdiff -pwbU8 (5.57 KB, patch)
2012-05-04 10:12 PDT, Philip Chee
neil: review-
Details | Diff | Splinter Review
Patch v1.1 Fix issues. (7.58 KB, text/plain)
2012-05-04 22:03 PDT, Philip Chee
no flags Details
Patch v1.1w hg qdiff -pwbU8 (5.64 KB, patch)
2012-05-04 22:07 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review
missing default favicon (39.47 KB, image/png)
2012-05-13 12:44 PDT, Stefan [:stefanh]
no flags Details
Patch v1.2 Fix Mac and default favicon. (7.10 KB, patch)
2012-05-15 10:26 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.2w hg qdiff -pwbU8 (5.35 KB, patch)
2012-05-15 10:27 PDT, Philip Chee
neil: review+
stefanh: review+
Details | Diff | Splinter Review
Patch v1.2a as checked-in r=Neil (7.05 KB, text/plain)
2012-05-16 09:03 PDT, Philip Chee
philip.chee: review+
Details

Description Philip Chee 2012-05-04 09:29:06 PDT
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 2012-05-04 09:36:42 PDT
Created attachment 621073 [details] [diff] [review]
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 2012-05-04 09:42:44 PDT
Created attachment 621076 [details] [diff] [review]
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 2012-05-04 10:12:30 PDT
Created attachment 621084 [details] [diff] [review]
Patch v1.0w hg qdiff -pwbU8

hg qdiff -w as requested.
Comment 4 neil@parkwaycc.co.uk 2012-05-04 12:21:57 PDT
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 neil@parkwaycc.co.uk 2012-05-04 16:09:55 PDT
(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 2012-05-04 20:01:51 PDT
>>+      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 2012-05-04 22:03:56 PDT
Created attachment 621247 [details]
Patch v1.1 Fix issues.
Comment 8 Philip Chee 2012-05-04 22:07:55 PDT
Created attachment 621249 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2012-05-06 04:39:36 PDT
(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 neil@parkwaycc.co.uk 2012-05-06 04:46:16 PDT
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] 2012-05-13 12:41:54 PDT
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] 2012-05-13 12:44:22 PDT
Created attachment 623516 [details]
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 2012-05-15 10:26:11 PDT
Created attachment 624098 [details] [diff] [review]
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 2012-05-15 10:27:58 PDT
Created attachment 624099 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2012-05-16 05:33:05 PDT
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 2012-05-16 09:03:06 PDT
Created attachment 624402 [details]
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 2012-05-16 09:04:56 PDT
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] 2012-05-19 11:41:20 PDT
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] 2012-05-19 11:58:21 PDT
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] 2012-05-19 13:08:37 PDT
(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.

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