Toolkit alerts shouldn't be displayed if the user's in a fullscreen app or presentation mode

RESOLVED FIXED in mozilla13

Status

()

Toolkit
General
RESOLVED FIXED
6 years ago
7 months ago

People

(Reporter: sid0, Assigned: sid0)

Tracking

unspecified
mozilla13
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

Created attachment 571302 [details] [diff] [review]
tentative fix v1

The same as bug 499557, except for Toolkit. If we're in a fullscreen application (e.g. game) or in presentation mode then we shouldn't show alerts.

Questions:
- Should the alerts service handle this internally or should it also expose shouldShowAlerts() as an API? The attached patch does it purely internally but an API might be useful for cases like bug 499557. I'm not sure.
- Should the ShouldShowAlerts() test come before or after delegating to the platform alerts service? It doesn't matter right now, but it might start mattering at some point.
- How can we test this?
Attachment #571302 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

6 years ago
Attachment #571302 - Flags: ui-review?(limi)
I am busy with silent update for at least the next couple of weeks in case you need this to be reviewed soonish.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 706520
Comment on attachment 571302 [details] [diff] [review]
tentative fix v1

Not entirely quite sure what counts as an "alert" in this context, but if it's the little download notifications etc, I agree — they shouldn't be shown.

For further ui-reviews, use ux-review@mozilla.com
Attachment #571302 - Flags: ui-review?(limi) → ui-review+
Comment on attachment 571302 [details] [diff] [review]
tentative fix v1

Neil, could you review this patch? I haven't been able to lower my silent update review queue as of yet and suspect this will just keep falling through the cracks.
Attachment #571302 - Flags: review?(robert.bugzilla) → review?(enndeakin)

Comment 5

6 years ago
The functionality looks ok, but I can't comment on whether the windows api stuff is correct. Perhaps a windows reviewer.
Attachment #571302 - Flags: review?(enndeakin) → review?(netzen)
Thanks Neil!
Comment on attachment 571302 [details] [diff] [review]
tentative fix v1

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

::: toolkit/components/alerts/nsAlertsService.cpp
@@ +78,5 @@
> +{
> +  bool result = true;
> +
> +#ifdef XP_WIN
> +  HMODULE hShellDLL = ::LoadLibraryW(L"shell32.dll");

nit: remove the 'h' handle prefix and just go with shellDLL.
nit: Do a NULL check here on the library handle.

::: toolkit/components/alerts/nsAlertsService.h
@@ +48,5 @@
> +    QUNS_BUSY = 2,
> +    QUNS_RUNNING_D3D_FULL_SCREEN = 3,
> +    QUNS_PRESENTATION_MODE = 4,
> +    QUNS_ACCEPTS_NOTIFICATIONS = 5,
> +    QUNS_QUIET_TIME = 6

nit: Might as well also add QUNS_IMMERSIVE = 7 which is for win8
Attachment #571302 - Flags: review?(netzen) → review+
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/69d3ec27878d
Assignee: nobody → sagarwal
Status: NEW → ASSIGNED
Whiteboard: [inbound]

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/69d3ec27878d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

7 months ago
See Also: → bug 621323
You need to log in before you can comment on or make changes to this bug.