Asynchronously add favicons to back/forward and history menus.

RESOLVED FIXED

Status

SeaMonkey
Bookmarks & History
--
enhancement
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
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.
Attachment #621073 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Attachment #621073 - Attachment is patch: true
(Assignee)

Comment 2

5 years ago
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.
Attachment #621073 - Attachment is obsolete: true
Attachment #621073 - Flags: review?(neil)
Attachment #621076 - Flags: review?(neil)
(Assignee)

Comment 3

5 years ago
Created attachment 621084 [details] [diff] [review]
Patch v1.0w hg qdiff -pwbU8

hg qdiff -w as requested.
Attachment #621084 - Flags: review?(neil)

Comment 4

5 years ago
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...
Attachment #621084 - Flags: review?(neil) → review-

Comment 5

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

Comment 6

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

Comment 7

5 years ago
Created attachment 621247 [details]
Patch v1.1 Fix issues.
(Assignee)

Comment 8

5 years ago
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!
Attachment #621076 - Attachment is obsolete: true
Attachment #621084 - Attachment is obsolete: true
Attachment #621076 - Flags: review?(neil)
Attachment #621249 - Flags: review?(neil)
Attachment #621249 - Flags: feedback?(stefanh)

Comment 9

5 years ago
(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 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 =
Attachment #621249 - Flags: review?(neil) → review+

Comment 11

5 years ago
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

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

Comment 13

5 years ago
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.
Attachment #621247 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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.
Attachment #621249 - Attachment is obsolete: true
Attachment #621249 - Flags: feedback?(stefanh)
Attachment #624099 - Flags: review?(stefanh)
Attachment #624099 - Flags: review?(neil)
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).
Attachment #624099 - Flags: review?(neil) → review+
(Assignee)

Comment 16

5 years ago
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.
Attachment #624402 - Flags: review+
(Assignee)

Comment 17

5 years ago
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

5 years ago
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

5 years ago
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.
Attachment #624099 - Flags: review?(stefanh) → review+

Comment 20

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

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

5 years ago
comm-central changeset 62fef30b1453
http://hg.mozilla.org/comm-central/rev/62fef30b1453
(Assignee)

Updated

2 years ago
Depends on: 1106274
You need to log in before you can comment on or make changes to this bug.