Closed Bug 1209601 Opened 4 years ago Closed 4 years ago

Add do not disturb management UI to preferences when a platform is using XUL alerts

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: Lina, Assigned: jaws)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

User Story

Acceptance Criteria: 

1. DND UI in Content Preferences appears in Firefox for Windows and checkbox can be turned on and off and does not override per site settings. 
2. Notification Preferences detail view should not be changed by DND being on.

Attachments

(2 files)

No description provided.
Assignee: kcambridge → jaws
Attached patch PatchSplinter Review
Attachment #8673294 - Flags: review?(MattN+bmo)
Comment on attachment 8673294 [details] [diff] [review]
Patch

Review of attachment 8673294 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/content.js
@@ +41,5 @@
> +    try {
> +      doNotDisturbAlertsEnabled = AlertsService.doNotDisturbTemporarily;
> +      // Toggle the state of do-not-disturb to see if it is available.
> +      // The service will throw if the feature is not implemented for the platform.
> +      if (doNotDisturbAlertsEnabled) {

My hope was that you wouldn't need to toggle and could just access AlertsService.doNotDisturbTemporarily. Did that not work?
Imagine in the future if there are notifications for toggling this, we wouldn't want code to run in this case.

@@ +299,5 @@
> +      } else {
> +        AlertsService.disableTemporaryDoNotDisturb();
> +      }
> +    } catch (ex) {
> +      Services.console.logStringMessage(ex.message);

Since this is running in-content you should be able to use console.error which would include more info such as the URL I think.

::: browser/components/preferences/in-content/tests/browser_notifications_do_not_disturb.js
@@ +23,5 @@
> +    alertService = Cc["@mozilla.org/alerts-service;1"]
> +                     .getService(Ci.nsIAlertsService)
> +                     .QueryInterface(Ci.nsIAlertsDoNotDisturb);
> +  } catch (ex) {
> +    todo(false, "Do not disturb is not available on this platform: " + ex.message);

This isn't really a TODO since the correct behaviour is to not run the rest. i.e. if someone is going through TODO to find things to test/fix, this wouldn't be one to consider. ok(true, …) is better IMO.
Attachment #8673294 - Flags: review?(MattN+bmo) → review+
User Story: (updated)
Attached patch follow-up patchSplinter Review
follow-up to fix failures on platforms that don't support the do-not-disturb interface.
Attachment #8676502 - Flags: review?(MattN+bmo)
Comment on attachment 8676502 [details] [diff] [review]
follow-up patch

Review of attachment 8676502 [details] [diff] [review]:
-----------------------------------------------------------------

The getter name should perhaps be changed to mention DND if that's what it's actually looking for.

::: browser/components/preferences/in-content/content.js
@@ +13,2 @@
>    } catch (ex) {
>      return;

Nit: This should return null or undefined explicitly since a getter should return a value.
Attachment #8676502 - Flags: review?(MattN+bmo) → review+
This functionality doesn't belong in the settings, since this is only a temporary pref whereas all other prefs are persistent in the settings page.
(In reply to Tim Nguyen [:ntim] from comment #9)
> This functionality doesn't belong in the settings, since this is only a
> temporary pref whereas all other prefs are persistent in the settings page.

Can you file a new bug to figure out a better place or wording for this?
Flags: needinfo?(ntim.bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > This functionality doesn't belong in the settings, since this is only a
> > temporary pref whereas all other prefs are persistent in the settings page.
> 
> Can you file a new bug to figure out a better place or wording for this?

Filed bug 1218424.
Blocks: 1218424
Flags: needinfo?(ntim.bugs)
You need to log in before you can comment on or make changes to this bug.