1250288 - Implement a preference that can disable favicon support in web notifications

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

We've resolved bug 1224785 to include favicons in notifications on OS X but we're not happy with the results and phlsa has suggested some tweaks ( logged in bug 1250285 )

Instead of backing the work out I'd rather implement a preference that disables the feature if possible so we can disable support in 47 and iterate. This could useful for iterating on XUL notification favicon support for Windows and Linux as well.

Top question of course is, how much effort is this vs backing the patch out and maintaining it as it rots?

Comment 1

[Lina Butler [:lina]]

Sounds good to me! Adding the pref should be a really straightforward change.

Comment 2

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1250288 - Add a pref for showing favicons in web notifications. r?MattN

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

Comment 3

[Lina Butler [:lina]]

As implemented, this pref would toggle favicons on all platforms, not just OS X. If we're going to disable them on OS X, I think we should do the same for XUL alerts on Windows and Linux...but please let me know if you'd like to keep the two separate.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8722153 [details]
MozReview Request: Bug 1250288 - Add a pref for showing favicons in web notifications. r?MattN

https://reviewboard.mozilla.org/r/35883/#review33487

I was waiting for an answer to:
(Quoting Kit Cambridge [:kitcambridge] from comment #3)
> If we're going to disable them on OS X, I think we should do the same
> for XUL alerts on Windows and Linux...but please let me know if you'd like
> to keep the two separate.

Please get an answer ot this before landing.

::: toolkit/components/alerts/nsAlertsService.cpp:130
(Diff revision 1)
> -  nsresult rv = ShowWithIconBackend(aBackend, aAlert, aAlertListener);
> +  nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> +  if (Preferences::GetBool("alerts.showFavicons")) {
> +    rv = ShowWithIconBackend(aBackend, aAlert, aAlertListener);
> +  }
>    if (NS_SUCCEEDED(rv)) {
>      return rv;
>    }

Instead of hard-coding a default value of `NS_ERROR_NOT_IMPLEMENTED` which may be confusing, why not wrap the `rv` declaration and NS_SUCCEEDED in the Preference check?

Comment 5

[Lina Butler [:lina]]

Edwin and I think we should pref favicons off on all platforms (not just OS X) until the UX issues are addressed. Does that sound OK, Jeff?

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Kit Cambridge [:kitcambridge] from comment #5)
> Edwin and I think we should pref favicons off on all platforms (not just OS
> X) until the UX issues are addressed. Does that sound OK, Jeff?

Yes please!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/043b74a82fa4

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/043b74a82fa4