Closed Bug 699014 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: rain1, Assigned: rain1)

References

Details

(Whiteboard: [inbound])

Attachments

(1 file)

Attached patch tentative fix v1Splinter Review
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)
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.
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)
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)
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+
http://hg.mozilla.org/integration/mozilla-inbound/rev/69d3ec27878d
Assignee: nobody → sagarwal
Status: NEW → ASSIGNED
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/69d3ec27878d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
See Also: → 621323
You need to log in before you can comment on or make changes to this bug.