Show the site favicon in OS X web notifications

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
Notifications and Alerts
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Comment hidden (empty)
Created attachment 8687507 [details]
MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen

Bug 1224785 - Show the site favicon in OS X web notifications.
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.
Attachment #8687507 - Flags: feedback?(mstange)
Attachment #8687507 - Flags: feedback?(MattN+bmo)
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 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
Attachment #8687507 - Flags: feedback?(MattN+bmo) → feedback+
This was already filed as bug 1212589 btw…
Duplicate of this bug: 1212589
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?
Attachment #8687507 - Flags: feedback?(mstange)
Depends on: 1227300
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?
Flags: needinfo?(kcambridge)
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/
Attachment #8687507 - Attachment description: MozReview Request: Bug 1224785 - Show the site favicon in OS X web notifications. → MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r?wchen
Attachment #8687507 - Flags: feedback+ → review?(wchen)
Created attachment 8705377 [details]
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/
Attachment #8705377 - Flags: review?(mstange)
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. :-/ 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?
Flags: needinfo?(kcambridge) → needinfo?(mcaceres)
Blocks: 1206560
See Also: bug 1206560
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.
Attachment #8705377 - Flags: review?(mstange)
(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.
Created attachment 8708162 [details]
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?
Flags: needinfo?(mcaceres) → needinfo?(seth)
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
Attachment #8687507 - Flags: review?(wchen) → review+
> Marcos, have you found a way to get higher-quality favicons?

Sorry, been away... no, I couldn't find a way.
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/
Attachment #8687507 - Attachment description: MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r?wchen → MozReview Request: Bug 1224785, Part 1 - Implement alert favicons backend. r=wchen
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/
Attachment #8705377 - Flags: review?(mstange)
Blocks: 1247390
Plan discussed with :edwong is to land this patch now, and fix the 1x resolution in a follow-up bug.
Flags: needinfo?(seth)
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
Attachment #8705377 - Flags: review?(mstange) → review+

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d5b329a9cd6b
https://hg.mozilla.org/mozilla-central/rev/0457ade4bd4b
https://hg.mozilla.org/mozilla-central/rev/58d7cfbf806c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
See Also: → bug 1249808
You need to log in before you can comment on or make changes to this bug.