859466 - sysAlerts should respect the value of ShouldShowAlerts

Status: RESOLVED WORKSFORME

(Toolkit :: General, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

We have sysAlerts which allows you to override the functionality of default notifications.

We have a ShouldShowAlerts that determines if alerts should be shown or not.

sysAlerts has always ignored this function.  To protect against future bugs we should have that handling first. 

Note that there is currently no bug here becuase ShouldShowAlerts only returns false on Windows Desktop, and we only provide sysAlerts on Windows Metro.  There is a bug in the handling of ShoudlShowAlerts though that I'll fix here too in case we ever remove sysAlerts in Metro.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

I was originally going to also change the implementation of ShouldShowAlert to allow showing them when in Metro, but I'm not sure if that's correct or not, and leaving it as is, is the safest way to go for now.

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 734788 [details] [diff] [review]
Patch v1.

Actually I'm going to have to fix the bug in ShouldShowAlert at least in the Metro case, please hold off on the review.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Comment 4

[William Chen [:wchen]]

Comment on attachment 734800 [details] [diff] [review]
Patch v2.

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

Moving the check further up in |ShowAlertNotification| looks right to me, although I'm not sure about the metro stuff since I don't have any experience with it.

Comment 5

[Brian R. Bondy [:bbondy]]

This patch is from 8 months ago so I'm not sure if it can be safely landed or not.  I don't think there is a pressing need though so I'm going to mark this as WFM and if we ever find it to be a problem we can take the patch.