Implement a preference that can disable favicon support in web notifications

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: canuckistani, Assigned: lina)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

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?
Sounds good to me! Adding the pref should be a really straightforward change.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
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 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?
Attachment #8722153 - Flags: review?(MattN+bmo) → review+
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?
Flags: needinfo?(jgriffiths)
(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!
Flags: needinfo?(jgriffiths)
See Also: → 1250285
https://hg.mozilla.org/mozilla-central/rev/043b74a82fa4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.