GetSystemMetric with SM_CONVERTIBLESLATEMODE looks interesting. Also we get a settings changed event tied to changes in the mode.

(In reply to Jim Mathies [:jimm] from comment #1) > GetSystemMetric with SM_CONVERTIBLESLATEMODE looks interesting. Also we get > a settings changed event tied to changes in the mode. Can you elaborate on the latter? I'm not afraid to write a patch, would just need to have a little bit more info - what kind of "settings changed event" ?

Ah, you mean a WM_SETTINGSCHANGE. I will search the web first before asking questions next time. :-)

We get a WM_SETTINGCHANGE with lParam "UserInteractionMode". See: https://msdn.microsoft.com/en-us/library/windows/apps/windows.ui.viewmanagement.userinteractionmode.ASPx , I guess. Because you know, reusing the things we already have would be weird, or something. :-\ Need to run out right now, will investigate how to examine the current state when I get back.

And it seems the value is stored in HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\ImmersiveShell\TabletMode because consistency would obviously also be confusing... :-\

So, unfortunately, as best I can tell, the registry key updates aren't synced up with the message. When reading the registry key when entering tablet mode, it will say "yes, we're in tablet mode" (though sometimes not for the first window), but when exiting tablet mode and reading the registry key when the windows get the WM_SETTINGCHANGE, it still says "yes, we're in tablet mode". It seems like it would be more reliable to rely only on the registry. :-( I've also tried using apimonitor to see if I can see any SPI_GET/SET things happening, but I have not been successful. Regarding the registry, it seems like we have some APIs (nsIWinRegKey.startWatching/hasChanged/stopWatching) to watch for changes, but I don't really understand how they work. From the JS side, we'd really want an event or observer notification or whatever that tells us something changed, and it seems here you need to call hasChanged every so often, or something? Nobody seems to have actually used these APIs since they were landed in 2007, so maybe I'm just missing the point. Jim, do you have suggestions on how to proceed here?

This is what I've been using to test stuff with. I tried both direct invocation of registry APIs and using nsIWinRegKey, and neither got me the expected results.

(In reply to :Gijs Kruitbosch from comment #6) > So, unfortunately, as best I can tell, the registry key updates aren't > synced up with the message. When reading the registry key when entering > tablet mode, it will say "yes, we're in tablet mode" (though sometimes not > for the first window), but when exiting tablet mode and reading the registry > key when the windows get the WM_SETTINGCHANGE, it still says "yes, we're in > tablet mode". > > It seems like it would be more reliable to rely only on the registry. :-( > > I've also tried using apimonitor to see if I can see any SPI_GET/SET things > happening, but I have not been successful. > > Regarding the registry, it seems like we have some APIs > (nsIWinRegKey.startWatching/hasChanged/stopWatching) to watch for changes, > but I don't really understand how they work. From the JS side, we'd really > want an event or observer notification or whatever that tells us something > changed, and it seems here you need to call hasChanged every so often, or > something? Nobody seems to have actually used these APIs since they were > landed in 2007, so maybe I'm just missing the point. > > Jim, do you have suggestions on how to proceed here? We don't want to read this stuff out of the registry, and I guess you found out why. Those values are not the MS approved way of getting at this info and as such can be unpredictable. Why can't we rely on the WM_SETTINGCHANGE notification?

(In reply to Jim Mathies [:jimm] from comment #8) > Why can't we rely on the WM_SETTINGCHANGE notification? It gets fired both when going in and when going out of tablet mode, and gives no indication which is happening. We also wouldn't know when the process starts, if we are or are not in tablet mode. It also gets fired on every window, and perhaps multiple times per window - I certainly haven't yet figured out why the printf fires 3-4 times for every switch with the above patch. Are you saying we should work just with that message, and maybe an initial read of the state from the registry?

When we receive it, can we query GetSystemMetric for the current state? Also according to this intel article we should get some sort of string value with it. https://software.intel.com/en-us/articles/detecting-slateclamshell-mode-screen-orientation-in-convertible-pc

(In reply to Jim Mathies [:jimm] from comment #10) > When we receive it, can we query GetSystemMetric for the current state? > > Also according to this intel article we should get some sort of string value > with it. > > https://software.intel.com/en-us/articles/detecting-slateclamshell-mode- > screen-orientation-in-convertible-pc I didn't see us getting these messages with WM_SETTINGCHANGE in win10, so I expect it's been deprecated. It would be weird if they replaced it with something else ("UserInteractionMode") for WM_SETTINGCHANGE but left the systemmetric for it... The docs also say that "Note that this system metric doesn't apply to desktop PCs" (what the value will be in that case... no idea), suggests using the AR state instead, and when I looked for that it seemed to be in WinUser.h from Win8's SDK onwards, but ifdef'd behind a bunch of WINVER and other stuff, and copying a bunch of the definitions across got me linking failures instead. I presume we need to use the same kind of hacks as we do for the DWM if we want to use these methods, and load kernel.dll (?) dynamically? I'll doublecheck the SystemMetric, but I'm not very hopeful.

In fact, it seems like the docs ( https://msdn.microsoft.com/en-us/library/windows/desktop/ms724385%28v=vs.85%29.aspx ) have a relevant comment: > SM_SYSTEMDOCKED and SM_CONVERTIBLESLATEMODE broken, have ambiguous return codes > they return 0 on fail (ie on a desktop where the system is incapable of docking or converting to a > tablet). They also return 0 when in slate (tablet) mode, or undocked. this is ambiguous. > > It would be slightly better if they returned 0 when either incapable or docked/laptop, and 1 when in > tablet/undocked. It would be a lot better if there were 3 return codes, 0 - incapable, 1 - desktop > ish(docked, laptop), 2 - tablet-ish(undocked, slate) > > since the API is broken anyway, i doubt people will care about backwards compatibility. just change it.

(In reply to :Gijs Kruitbosch from comment #11) > I'll doublecheck the SystemMetric, but I'm not very hopeful. This returns 0 100% of the time on my VM, whether in tablet mode or not. Do you have any other ideas on what to check for?

(In reply to :Gijs Kruitbosch from comment #13) > (In reply to :Gijs Kruitbosch from comment #11) > > I'll doublecheck the SystemMetric, but I'm not very hopeful. > > This returns 0 100% of the time on my VM, whether in tablet mode or not. Do > you have any other ideas on what to check for? Not currently. There are a few developers asking the same question, so far I'm not seeing much from microsoft.

Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, f?jmathies

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Jim, could you look at what I'm still missing here? The example code we got assumes we have win10 headers. We don't, and it doesn't seem likely that we'll be able to update the minimum requirement for building Firefox to using the win10 sdk, especially as that isn't even final yet, but also considering mozilla-build, the build system and our CI will all need to be updated for that. So I tried to declare a bunch of the stuff myself... but I'm not really sure if that will get us far enough? Your mentions of IInspectable seemed to suggest that COM should make this kind of thing possible even when we don't actually currently have the right headers/libraries to include/link, but I couldn't quite figure out how to stand that up. For reference, the current code produces: https://pastebin.mozilla.org/8837028

(In reply to :Gijs Kruitbosch from comment #16) > Comment on attachment 8623225 [details] > MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode > to xul/js/css, f?jmathies > > Jim, could you look at what I'm still missing here? > > The example code we got assumes we have win10 headers. We don't, and it > doesn't seem likely that we'll be able to update the minimum requirement for > building Firefox to using the win10 sdk, especially as that isn't even final > yet, but also considering mozilla-build, the build system and our CI will > all need to be updated for that. We've run into this countless times before. Two approaches have been taken: 1) since header gunk is copyrighted ms, if we can't roll our own we simply don't support the new feature until releng gets the sdk updated. 2) import win10 sdk header gunk into our source code repo but severely munge it. > So I tried to declare a bunch of the stuff myself... but I'm not really sure > if that will get us far enough? It's one of two options you have. With winrt gunk it might be harder than it was with the old com interfaces. I've not had to do this, we made the the win8 sdk a requirement and releng rolled it out for us. > Your mentions of IInspectable seemed to > suggest that COM should make this kind of thing possible even when we don't > actually currently have the right headers/libraries to include/link, but I > couldn't quite figure out how to stand that up. I'd suggest getting the headers going over something like this.

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted https://reviewboard.mozilla.org/r/11477/#review10015 looks good! ::: widget/windows/WindowsUIUtils.cpp:20 (Diff revision 1) > + nit - lets clean up the double line spacing in here please. ::: widget/windows/WindowsUIUtils.cpp:17 (Diff revision 1) > +#include "wrl.h" do we need to include this if we already include mozwrlbase.h? ::: widget/windows/WindowsUIUtils.cpp:81 (Diff revision 1) > + &uiViewSettingsInterop); ah, takes me back to metrofx days. :/

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm

Updated this to use the hidden window. The added definitions means this now actually compiles and works in my testing. The only issue I have with this is that the code will call the UpdateTabletModeState() function once for every window that gets the WM_SETTINGCHANGE message (every one of them), which seems wrong, but I don't know how to sensibly avoid this and only call it for the hidden window... ideas on that front would be helpful.

(In reply to :Gijs Kruitbosch from comment #20) > Updated this to use the hidden window. The added definitions means this now > actually compiles and works in my testing. > > The only issue I have with this is that the code will call the > UpdateTabletModeState() function once for every window that gets the > WM_SETTINGCHANGE message (every one of them), which seems wrong, but I don't > know how to sensibly avoid this and only call it for the hidden window... > ideas on that front would be helpful. You could gate the code on a mWindowType == eWindowType_toplevel or even eWindowType_invisible to really limit it. How about that? If we got something other than a string it would be easy but I guess you always have to do that aweful string com pare to get a value, so limiting the number of windows that call this seems like the next best thing to do.

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted https://reviewboard.mozilla.org/r/11477/#review10077 ::: widget/windows/WindowsUIUtils.cpp:86 (Diff revision 2) > + __RPC__deref_out_opt void **ppv) = 0; nit - you can strip all the __RPC__* stuff here ::: widget/windows/WindowsUIUtils.cpp:88 (Diff revision 2) > +}; nit - white space ::: widget/windows/WindowsUIUtils.cpp:67 (Diff revision 2) > + __RPC__out ABI::Windows::UI::ViewManagement::UserInteractionMode *value) = 0; nit - do we need the namespace info here? ::: widget/windows/nsWindow.cpp:4632 (Diff revision 2) > + if (IsWin10OrLater() && lParam) { are you going to try to limit the number of times we execute this by checking the window type?

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm

This patch addresses all the comments and limits the update to invisible windows, which works for me locally. \o/

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted https://reviewboard.mozilla.org/r/11477/#review10119 Ship It!

Tree is closed, pushed to try (probably a good idea anyway, hope it builds there, too...) https://treeherder.mozilla.org/#/jobs?repo=try&revision=4462d4de851b

The trypush is red because of missing headers. The tinderbox builds don't use the winrt headers (anymore?), but local builds do because they use vcvars and that automatically adds them to the include path. So *presumably* this trypush is just going to work...: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8e59f36fafe

(In reply to :Gijs Kruitbosch from comment #27) > The trypush is red because of missing headers. > > The tinderbox builds don't use the winrt headers (anymore?), but local > builds do because they use vcvars and that automatically adds them to the > include path. So *presumably* this trypush is just going to work...: > > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8e59f36fafe And that timed out for reasons #releng and I are not clear on. philor hypothesized I broke the installer. I don't understand how I managed that considering my local builds work, but it's a possibility. Retriggering to exclude the other suggestion: network issues with the master/slave situation.

I think you're still missing includes, see bug 1039866. your changes: ## includes: win8 sdk includes, msvc 10 std library, directx sdk for d3d9 ## -export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/um:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/tools/sdks/dx10/include +export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/um:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/winrt:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/tools/sdks/dx10/include from the metro bug: -## includes: win8 sdk includes, winrt headers for metro, msvc 10 std library, directx sdk for d3d9 ## -export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/um:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/winrt:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/winrt/wrl:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/winrt/wrl/wrappers:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/tools/sdks/dx10/include +## includes: win8 sdk includes, msvc 10 std library, directx sdk for d3d9 ## +export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ Kits/8.0/include/um:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/tools/sdks/dx10/include https://bug1039866.bugzilla.mozilla.org/attachment.cgi?id=8593404 you need to include wrl and wrl/wrappers. That might fix it.

(In reply to Jim Mathies [:jimm] from comment #29) > I think you're still missing includes, see bug 1039866. > > your changes: > ## includes: win8 sdk includes, msvc 10 std library, directx sdk for d3d9 ## > -export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/um:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/ > tools/sdks/dx10/include > +export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/um:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/winrt:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/ > tools/sdks/dx10/include > > from the metro bug: > > -## includes: win8 sdk includes, winrt headers for metro, msvc 10 std > library, directx sdk for d3d9 ## > -export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/um:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/winrt:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/winrt/wrl:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/winrt/wrl/wrappers:${_VSPATH}/vc/include:${_VSPATH}/vc/ > atlmfc/include:/c/tools/sdks/dx10/include > +## includes: win8 sdk includes, msvc 10 std library, directx sdk for d3d9 ## > +export INCLUDE=/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/shared:/c/Program\ Files\ \(x86\)/Windows\ > Kits/8.0/include/um:${_VSPATH}/vc/include:${_VSPATH}/vc/atlmfc/include:/c/ > tools/sdks/dx10/include > > https://bug1039866.bugzilla.mozilla.org/attachment.cgi?id=8593404 > > you need to include wrl and wrl/wrappers. That might fix it. That's interesting. I could do that, but it compiles both on my win10 and win8 machines without including those. The include path produced by MSVC on those is just: %VS13%\VC\Include %SDKDIR%\include\shared %SDKDIR%\include\um %SDKDIR%\include\winrt I would also expect missing includes to fail in the same way as the first try push, not this maddening timeout with no useful error messages :(

Ben suggested waiting because this doesn't seem to be the only windows trypush that is having issues. Will try to push again tomorrow or late tonight. Going to update the patch to include the build mozconfig change and r?ted for that.

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r=jimm,ted

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Carrying over r=jimm, Ted, please see comment #29 and later as well and let me know what you'd prefer. I've not updated the 2010 things per your comments on IRC.

Status update: Slave loaner not helping so far because can't figure out how to build on it. Pushed remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcaf62c59a67 to see if that magically makes a difference.

So the reason this all fails is because the automation build system builds the precompiled scripts cache thing, for which it tries to run xpcshell with some things, which then pops up a modal prompt: "The program can't start because api-ms-win-core-winrt-l1-1-0.dll is missing from your computer. Try reinstalling the program to fix this problem." This also happens if I package up a build and try to run it on a win7 machine that doesn't have the win8 SDKs / redistributables/whatever... Jim, any idea what would be the best way to fix this? As best I can tell these are just the win8 winrt API things, so maybe I just don't want the runtimeobject.lib thing and want to ensure we link things statically at compile-time, or something? Maybe I just totally misunderstand the problem?

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm

I *think* this is the correct way of doing this. Jim, please let me know if you agree. :-)

I think we worked around this in a much simpler way by setting these libs as delay load libraries. That way we aren't dependent on them at startup. http://mxr.mozilla.org/projects-central/source/metro/toolkit/library/moz.build#33 Your method of loading dynamically works too but it's a bit messy. Would you mind trying to bring back this bit of config gunk and see if it fixes it?

better link: http://mxr.mozilla.org/projects-central/search?find=%2Fmetro%2F&string=CRTEXPDLLVERSION

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted This seems to work, I think?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7c8554abc3a has green builds on try \o/

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted I'm fine with this. Ted might have a comment on that crt constant we set in configure, I'm not sure that's needed. Honestly I don't remember why we separated that data from the moz build file down in library. Either way, wfm.

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted https://reviewboard.mozilla.org/r/11477/#review10837 Ship It! ::: configure.in:497 (Diff revision 6) > +CRTEXPDLLVERSION=1-1-0 Is there a reason this needs to be in configure and wind up as a CONFIG variable? I think it'd be fine to hardcode it in the moz.build file right now. ::: toolkit/library/moz.build:37 (Diff revision 6) > + 'API-MS-WIN-CORE-WINRT-L' + CONFIG['CRTEXPDLLVERSION'] + '.DLL', Do these need to be ALL CAPS? (Doesn't matter that much, just odd given that the ones listed previously are not.)

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Approval Request Comment [Feature/regressing bug #]: can't detect tablet mode [User impact if declined]: can't fix things on tablet mode [Describe test coverage new/current, TreeHerder]: nope [Risks and why]: reasonably low, adds some APIs for win10 use [String/UUID change made/needed]: adds an exposed IDL on win10 only, doesn't change any existing UUIDs, so should be OK?

Comment on attachment 8623225 [details] MozReview Request: Bug 1170522 - expose whether or not we're in tablet mode to xul/js/css, r?jimm,ted Prereq for Windows 10 work. UUID change is fine as this is a new IDL and so won't have add-on compat issues. Beta+ Aurora+

Setting as qe-verify- since this is prerequisite for Win 10 work. Feel free to set back if you think this needs manual testing.