940677 - Change - Add a pref to disable auto switching between Desktop/Metro for Laptop/Slates

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Brian R. Bondy [:bbondy]]

Bug 935178 added support for a feature to auto switch environments for Intel based laptops that support laptop/slate notifications.  This bug is to add a pref to disable that functionality.

Comment 1

[Brian R. Bondy [:bbondy]]

p=1

Comment 6

[Brian R. Bondy [:bbondy]]

By the way Stephen, this preference needs to be disabled by default.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

Just had a few questions:
1. What should the pref be called?
2. Is my assumption correct that the pref has to be added to both firefox.js and metro.js? Or is metro.js just extending firefox.js, and putting it in firefox.js would be enough?
3. Should the pref in firefox.js be ifdef'd for XP_WIN?
4. Anything else I've missed?

Thanks!

Comment 8

[Brian R. Bondy [:bbondy]]

(In reply to Stephen Pohl [:spohl] from comment #7)
> Created attachment 8345337 [details] [diff] [review]
> Patch
> 
> Just had a few questions:
> 1. What should the pref be called?

I'm not sure but probably something like:
browser.shell.metro-auto-switch-enabled and browser.shell.desktop-auto-switch-enabled is good enough.

> 2. Is my assumption correct that the pref has to be added to both firefox.js
> and metro.js? Or is metro.js just extending firefox.js, and putting it in
> firefox.js would be enough?
> 3. Should the pref in firefox.js be ifdef'd for XP_WIN?

You don't even need to list it in firefox.js and meto.js, you can just default the check to false.
The only difference really is that the pref won't show up by default in about:config which is fine for this pref.

> 4. Anything else I've missed?

The pref will basically just disable this functionality:
https://hg.mozilla.org/mozilla-central/rev/594ae98c4fdd

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Brian!

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> You don't even need to list it in firefox.js and meto.js, you can just
> default the check to false.

Verified that Preferences::GetBool defaults to false if pref doesn't exist.

> The only difference really is that the pref won't show up by default in
> about:config which is fine for this pref.

Ah, that makes it easy. I assumed we wanted the pref to show.

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Patch

Forgot to update the comments.

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8345349 [details] [diff] [review]
Patch

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

Thanks for the patch!

::: widget/windows/winrt/MetroWidget.cpp
@@ +718,5 @@
>      if (aLParam && !wcsicmp(L"ConvertibleSlateMode", (wchar_t*)aLParam)) {
> +      // If we're switching away from slate mode, switch to
> +      // Desktop for hardware that supports this feature if the pref is set.
> +      if (GetSystemMetrics(SM_CONVERTIBLESLATEMODE) != 0 &&
> +          Preferences::GetBool("browser.shell.metro-auto-switch-enabled")) {

nit: Here and in nsWindow.cpp
Yep the default second parameter to GetBool is false, please just put a false explicitly for the GetBool call though just for extra code readability.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks for the quick review! Added explicit false in both places. Carrying over r+.

Comment 13

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed416f68e952

Comment 14

[Stephen A Pohl [:spohl]]

I don't have hardware to verify this, but it'd be great to confirm that this is working as expected in nightly before I request approval for aurora. Thanks!

Comment 15

[Brian R. Bondy [:bbondy]]

The Surface cover detach thing should be fixed by this btw Juan.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed416f68e952

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8345363 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 935178

User impact if declined: Firefox may switch unexpectedly between Metro and Desktop modes.

Testing completed (on m-c, etc.): Landed on m-c on 12/10.

Risk to taking this patch (and alternatives if risky): Very low risk Metro-only patch that just disables a recently-landed feature.

String or IDL/UUID changes made by this patch: None.

Comment 18

[Matt Brubeck (:mbrubeck)]

I tested this in Nightly on my Surface Pro 2 with Type Cover 2 and it disables the auto-switching as expected.  Thanks!

https://hg.mozilla.org/releases/mozilla-aurora/rev/934314077dd1

Comment 21

[Cornel Ionce [:noni] [Hubs QA]]

Matt, can you confirm this is fixed on Firefox 28 Beta 4?
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/28.0b4-candidates/build1/win32/en-US/