1224785 - Show the site favicon in OS X web notifications

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen

Bug 1224785 - Show the site favicon in OS X web notifications.

Comment 2

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/25215/#review22715

::: widget/cocoa/OSXNotificationCenter.mm:409
(Diff revision 1)
> +    nsCOMPtr<nsIFaviconDataCallback> callback = new FaviconDataCallback(this,

It'd be a lot cleaner if we passed the WebIDL Notification object around, instead of all these arguments. Maybe we should do that first, then rebase this patch.

Comment 3

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/25215/#review22783

Did you consider putting the favicon code in the toolkit alert service instead (passing the data to the system alert service)?

::: widget/cocoa/OSXNotificationCenter.mm:389
(Diff revision 1)
> +      return DisplayCocoaAlert(aImageUrl, aAlertCookie, aAlertListener,
> +                               aPrincipal, aInPrivateBrowsing, alertName,
> +                               notification, osxni);
> +    }
> +
> +    nsCOMPtr<nsIURI> pageURI;
> +    if (NS_WARN_IF(NS_FAILED(aPrincipal->GetURI(getter_AddRefs(pageURI))))) {
> +      return DisplayCocoaAlert(aImageUrl, aAlertCookie, aAlertListener,
> +                               aPrincipal, aInPrivateBrowsing, alertName,
> +                               notification, osxni);
> +    }
> +
> +    nsCOMPtr<mozIAsyncFavicons> asyncFavicons = do_GetService(
> +      "@mozilla.org/browser/favicon-service;1");
> +    if (!asyncFavicons) {
> +      return DisplayCocoaAlert(aImageUrl, aAlertCookie, aAlertListener,
> +                               aPrincipal, aInPrivateBrowsing, alertName,
> +                               notification, osxni);
> +    }
> +
> +    nsCOMPtr<nsIFaviconDataCallback> callback = new FaviconDataCallback(this,
> +      aImageUrl, aAlertCookie, aAlertListener, aPrincipal, aInPrivateBrowsing,
> +      alertName, notification, osxni);
> +    return asyncFavicons->GetFaviconDataForPage(pageURI, callback);
> +#else
> +    // Show the notification without a favicon if the Places API is disabled.
> +    return DisplayCocoaAlert(aImageUrl, aAlertCookie, aAlertListener,

The four calls to `DisplayCocoaAlert` in this one method make me think it should be refactored.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8687507 [details]
MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen

(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> https://reviewboard.mozilla.org/r/25215/#review22715
> 
> ::: widget/cocoa/OSXNotificationCenter.mm:409
> (Diff revision 1)
> > +    nsCOMPtr<nsIFaviconDataCallback> callback = new FaviconDataCallback(this,
> 
> It'd be a lot cleaner if we passed the WebIDL Notification object around,
> instead of all these arguments. Maybe we should do that first, then rebase
> this patch.

I think if we do this in nsAlertsService.cpp is may be less messy but I agree in general that we should see if DOM peers would support passing the WebIDL Notification object around

Comment 5

[Matthew N. [:MattN]]

This was already filed as bug 1212589 btw…

Comment 7

[Markus Stange [:mstange]]

I agree that this should be refactored. Also, you need to use an nsCOMPtr instead of a raw pointer to hold on to the nsIPrincpal and nsIObserver objects.

Looking at the existing code, the memory management of the osxni objects is unclear. Are we leaking them?

Comment 8

[Andrew Overholt [:overholt]]

Marcos Caceres once told me that at least one part of our favicon code doesn't use the highest quality icon in the .ico archive. Are pixellated icons a concern here?

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8687507 [details]
MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25215/diff/1-2/

Comment 10

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1224785, Part 2 - Show the site favicon in OS X notifications. r?mstange

Review commit: https://reviewboard.mozilla.org/r/29949/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29949/

Comment 11

[Lina Butler [:lina]]

Attachment: Screen Shot 2016-01-07 at 3.49.49 PM.png

(In reply to Andrew Overholt [:overholt] from comment #8)
> Marcos Caceres once told me that at least one part of our favicon code
> doesn't use the highest quality icon in the .ico archive. Are pixellated
> icons a concern here?

Yes. :-/ The attached screenshot shows the pixellation. Poking around with the Browser Toolbox, I see the tab bar does `<tab image="/url/to/icon">`, which gives the highest-quality icon. I'm guessing the bookmarks toolbar loads the icon from Places, so that's why it looks pixellated. Not sure if using the `moz-anno:favicon` URL for XUL alerts will be any better. Bug 492172 seems relevant.

Marcos, have you found a way to get higher-quality favicons?

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8705377 [details]
MozReview Request: Bug 1224785, Part 2 - Show the site favicon in OS X notifications. r?mstange

https://reviewboard.mozilla.org/r/29949/#review27027

::: widget/cocoa/OSXNotificationCenter.mm:221
(Diff revision 1)
> -NS_IMPL_ISUPPORTS(OSXNotificationCenter, nsIAlertsService, imgINotificationObserver, nsITimerCallback)
> +NS_IMPL_ISUPPORTS(OSXNotificationCenter, nsIAlertsService, imgINotificationObserver, nsITimerCallback,

Might as well move nsITimerCallback into the second line, too.

::: widget/cocoa/OSXNotificationCenter.mm:375
(Diff revision 1)
> +    [(NSObject*)notification setValue:icon forKey:@"_identityImage"];

I think you need to release "icon" after this line. Or you could autorelease it in the line where you allocate it.

Comment 13

[Matthew N. [:MattN]]

(In reply to Kit Cambridge [:kitcambridge] from comment #11)
> Created attachment 8705380 [details]
> Screen Shot 2016-01-07 at 3.49.49 PM.png
> 
> (In reply to Andrew Overholt [:overholt] from comment #8)
> > Marcos Caceres once told me that at least one part of our favicon code
> > doesn't use the highest quality icon in the .ico archive. Are pixellated
> > icons a concern here?

Yes but that's something that should be fixed in Places (there are many bugs on this) and then this code will get the benefits for free. I don't think we should do any special work in the alert service to get better favicons and keep a single source of truth for that in the Places Favicon service.

> Yes. :-/ The attached screenshot shows the pixellation. Poking around with
> the Browser Toolbox, I see the tab bar does `<tab image="/url/to/icon">`,
> which gives the highest-quality icon. I'm guessing the bookmarks toolbar
> loads the icon from Places, so that's why it looks pixellated. Not sure if
> using the `moz-anno:favicon` URL for XUL alerts will be any better. Bug
> 492172 seems relevant.

Please ensure that favicon patches are doing the correct thing with multi-resolution ICO files. You can test with attachment 8465124 [details] and its image (attachment 8463655 [details]) and ensure that the image shows "32" on a 2x display and "16" on a 1x display. Our image code should do this automatically now for HTML/XUL but the Cocoa code may need help with this. :Seth can help with this.

Comment 14

[Lina Butler [:lina]]

Attachment: 1x.png

(In reply to Matthew N. [:MattN] from comment #13)
> Please ensure that favicon patches are doing the correct thing with
> multi-resolution ICO files. You can test with attachment 8465124 [details]
> and its image (attachment 8463655 [details]) and ensure that the image shows
> "32" on a 2x display and "16" on a 1x display. Our image code should do this
> automatically now for HTML/XUL but the Cocoa code may need help with this.
> :Seth can help with this.

It's not quite right. "32" shows up for both 1x and 2x resolution. I can call `[icon setSize:NSMakeSize(16, 16)]`, but that'll force "16" on a 2x display.

I see https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm loads the image, then decodes the first frame and calls `CreateNSImageFromCGImage`. Do we need to do the same here?

Comment 15

[William Chen [:wchen]]

Comment on attachment 8687507 [details]
MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen

https://reviewboard.mozilla.org/r/25215/#review28151

::: toolkit/components/alerts/nsIAlertsService.idl:16
(Diff revision 2)
>  [scriptable, uuid(9e87fc34-8dbb-4b14-a724-b27be6822ec8)]

uuid

Comment 16

[Marcos Caceres [:marcosc]]

> Marcos, have you found a way to get higher-quality favicons?

Sorry, been away... no, I couldn't find a way.

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8687507 [details]
MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25215/diff/2-3/

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8705377 [details]
MozReview Request: Bug 1224785, Part 2 - Show the site favicon in OS X notifications. r?mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29949/diff/1-2/

Comment 19

[Lina Butler [:lina]]

Plan discussed with :edwong is to land this patch now, and fix the 1x resolution in a follow-up bug.

Comment 20

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffd1d930eddf

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8705377 [details]
MozReview Request: Bug 1224785, Part 2 - Show the site favicon in OS X notifications. r?mstange

https://reviewboard.mozilla.org/r/29949/#review31557

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d5b329a9cd6b
https://hg.mozilla.org/integration/fx-team/rev/0457ade4bd4b

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/58d7cfbf806c

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5b329a9cd6b
https://hg.mozilla.org/mozilla-central/rev/0457ade4bd4b
https://hg.mozilla.org/mozilla-central/rev/58d7cfbf806c