Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1758823004:
Screen.orientation lock API implementation for Windows8 and later. (Closed)
|
Created:
4 years, 9 months ago by aleksandar.stojiljkovic Modified:
4 years, 9 months ago Reviewers:
Avi (use Gerrit), sky, mlamouri (slow - plz ping), cpu_(ooo_6.6-7.5), davidben, scottmg, Ben Goodger (Google), ojan CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-screen-orientation_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScreen.orientation lock API implementation for Windows8 and later.
Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and
unlocks orientation in full screen and windowed mode.
Tested on Windows8 Yoga 12 laptop only.
BUG=423919, 400846
Committed: https://crrev.com/4f4b5efee23bcf51ccd96c23bd67cdc7bb1e07e2
Cr-Commit-Position: refs/heads/master@{#380893}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review #4 and #5 fixes. Thanks @avi. #
Total comments: 1
Patch Set 3 : Runtime resolving of GetAutoRotationState and SetDisplayAutoRotationPreferences - not available in Windows7 user32.dll #
Total comments: 33
Patch Set 4 : Review #21 and #22 fixes. #Patch Set 5 : Disable API in multi monitor case. #
Total comments: 8
Patch Set 6 : #32 & #33 review fixes - thanks @cpu #Patch Set 7 : rebasing #
Messages
Total messages: 52 (14 generated)
Description was changed from ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. BUG=400846 ========== to ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. (Relevant bug, not the fix for it) BUG=400846 ==========
aleksandar.stojiljkovic@intel.com changed reviewers: + avi@chromium.org, davidben@chromium.org, mlamouri@chromium.org
https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... File content/browser/screen_orientation/screen_orientation_delegate_win.h (right): https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... content/browser/screen_orientation/screen_orientation_delegate_win.h:8: #define CONTENT_BROWSER_SCREEN_ORIENTATION_SCREEN_ORIENTATION_DELEGATE_WIN_H_ Include guards have to be the first thing in the file.
https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... content/browser/screen_orientation/screen_orientation_delegate_win.cc:33: || dm.dmDisplayOrientation == DMDO_180); Multiple-line if() blocks need {}. https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... content/browser/screen_orientation/screen_orientation_delegate_win.cc:84: break; Switch statements need indenting. https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements
Patch set 2: Review #4 and #5 fixes. Thanks @avi. https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... content/browser/screen_orientation/screen_orientation_delegate_win.cc:33: || dm.dmDisplayOrientation == DMDO_180); On 2016/03/02 19:28:20, Avi wrote: > Multiple-line if() blocks need {}. Done. https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... content/browser/screen_orientation/screen_orientation_delegate_win.cc:84: break; On 2016/03/02 19:28:20, Avi wrote: > Switch statements need indenting. > > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements Done. https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... File content/browser/screen_orientation/screen_orientation_delegate_win.h (right): https://codereview.chromium.org/1758823004/diff/1/content/browser/screen_orie... content/browser/screen_orientation/screen_orientation_delegate_win.h:8: #define CONTENT_BROWSER_SCREEN_ORIENTATION_SCREEN_ORIENTATION_DELEGATE_WIN_H_ On 2016/03/02 19:25:12, Avi wrote: > Include guards have to be the first thing in the file. Done.
Description was changed from ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. (Relevant bug, not the fix for it) BUG=400846 ========== to ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. BUG=423919, 400846 ==========
So it LGTM re content. I don't know Windows, so I can't review that. Get an LG from a Windows API peep before landing.
avi@chromium.org changed reviewers: + cpu@chromium.org
+cpu as one of the windowsiest peeps I know
https://codereview.chromium.org/1758823004/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:94: && (autoRotationState == AR_ENABLED)); It might be better to change this to: && !(autoRotationState & AR_NOSENSOR) && !(autoRotationState & AR_NOT_SUPPORTED) For 2-in-1 laptops and for docked tablets it would set preferred orientation although it wouldn't apply it immediately. The orientation lock API is checking what is the state after this call, and it would return error. However, once tablet is undocked or when 2-in-1 switches to tablet mode, it should go to screen orientation that is set in JS API. 1. If we keep "&& (autoRotationState == AR_ENABLED));" here, it would not allow setting preferred orientation in laptop or docked mode. 2. If using "&& !(autoRotationState & AR_NOSENSOR) && !(autoRotationState & AR_NOT_SUPPORTED)", it would allow setting preferred orientation but it would return error in JS and orientation would change when entering tablet/undocked mode.
On 2016/03/03 20:34:36, aleksandar.stojiljkovic wrote: > https://codereview.chromium.org/1758823004/diff/20001/content/browser/screen_... > File content/browser/screen_orientation/screen_orientation_delegate_win.cc > (right): > > https://codereview.chromium.org/1758823004/diff/20001/content/browser/screen_... > content/browser/screen_orientation/screen_orientation_delegate_win.cc:94: && > (autoRotationState == AR_ENABLED)); > It might be better to change this to: > && !(autoRotationState & AR_NOSENSOR) && !(autoRotationState & AR_NOT_SUPPORTED) > For 2-in-1 laptops and for docked tablets it would set preferred orientation > although it wouldn't apply it immediately. > The orientation lock API is checking what is the state after this call, and it > would return error. > However, once tablet is undocked or when 2-in-1 switches to tablet mode, it > should go to screen orientation that is set in JS API. > > 1. If we keep "&& (autoRotationState == AR_ENABLED));" here, it would not allow > setting preferred orientation in laptop or docked mode. > > 2. If using "&& !(autoRotationState & AR_NOSENSOR) && !(autoRotationState & > AR_NOT_SUPPORTED)", it would allow setting preferred orientation but it would > return error in JS and orientation would change when entering tablet/undocked > mode. This is resolved in Patch set 3. After testing on Windows 7 added one more change: Patch Set 3 : Runtime resolving of GetAutoRotationState and SetDisplayAutoRotationPreferences as they are not available in Windows7 user32.dll
(Removing myself from reviewers since it looks like Avi's got this from the content end and I'm not a content OWNER anymore.)
On 2016/03/04 12:30:26, aleksandar.stojiljkovic wrote: > On 2016/03/03 20:34:36, aleksandar.stojiljkovic wrote: > > > https://codereview.chromium.org/1758823004/diff/20001/content/browser/screen_... > > File content/browser/screen_orientation/screen_orientation_delegate_win.cc > > (right): > > > > > https://codereview.chromium.org/1758823004/diff/20001/content/browser/screen_... > > content/browser/screen_orientation/screen_orientation_delegate_win.cc:94: && > > (autoRotationState == AR_ENABLED)); > > It might be better to change this to: > > && !(autoRotationState & AR_NOSENSOR) && !(autoRotationState & > AR_NOT_SUPPORTED) > > For 2-in-1 laptops and for docked tablets it would set preferred orientation > > although it wouldn't apply it immediately. > > The orientation lock API is checking what is the state after this call, and it > > would return error. > > However, once tablet is undocked or when 2-in-1 switches to tablet mode, it > > should go to screen orientation that is set in JS API. > > > > 1. If we keep "&& (autoRotationState == AR_ENABLED));" here, it would not > allow > > setting preferred orientation in laptop or docked mode. > > > > 2. If using "&& !(autoRotationState & AR_NOSENSOR) && !(autoRotationState & > > AR_NOT_SUPPORTED)", it would allow setting preferred orientation but it would > > return error in JS and orientation would change when entering tablet/undocked > > mode. > > This is resolved in Patch set 3. > After testing on Windows 7 added one more change: > Patch Set 3 : Runtime resolving of GetAutoRotationState and > SetDisplayAutoRotationPreferences as they are not available in Windows7 > user32.dll This still lg for content, and you still need a Windows reviewer.
aleksandar.stojiljkovic@intel.com changed reviewers: + ben@chromium.org, ojan@chromium.org, sky@chromium.org
> On 2016/03/04 19:13:52, Avi wrote: > This still lg for content, and you still need a Windows reviewer @ben, @ojan, @sky, Someone with Windows expertise should also review the patch - could you please help? Thanks.
I'm not an owner in content. On Fri, Mar 4, 2016 at 12:09 PM, <aleksandar.stojiljkovic@intel.com> wrote: >> On 2016/03/04 19:13:52, Avi wrote: >> This still lg for content, and you still need a Windows reviewer > > @ben, @ojan, @sky, > > Someone with Windows expertise should also review the patch - could you > please > help? Thanks. > > https://codereview.chromium.org/1758823004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ben@chromium.org changed reviewers: + scottmg@chromium.org
I am an OWNER in content but scottmg knows all the new cool win32 stuff better than I do. scottmg, feel free to pass the buck onward if there's someone better to look at the windows bits. I'm happy to provide a stamp once someone who knows what they're doing has OK'ed.
On 2016/03/04 21:32:31, Ben Goodger (Google) wrote: > I am an OWNER in content but scottmg knows all the new cool win32 stuff better > than I do. scottmg, feel free to pass the buck onward if there's someone better > to look at the windows bits. I'm happy to provide a stamp once someone who knows > what they're doing has OK'ed. Hello? I'm a content owner and I have already LGed this patch. Don't worry about getting a content OK. It already has one! Just please review the Windows code.
https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:7: #include <windows.h> nit: newline between 7/8. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:27: ORIENTATION_PREFERENCE orientation) { nit: run git cl format as your formatting is off here. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:36: set_display_auto_rotation_preferences_func(orientation); How come the docs for this say it takes a pointer? https://msdn.microsoft.com/en-us/library/windows/desktop/dn629268(v=vs.85).aspx https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:40: static BOOL GetAutoRotationStateWrapper(PAR_STATE pState) { p_state https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:40: static BOOL GetAutoRotationStateWrapper(PAR_STATE pState) { Is there a reason to use windows types here rather than standard c++ types? https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:41: typedef BOOL(WINAPI *GetAutoRotationStatePtr)(PAR_STATE); typedef->using https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:51: static void GetCurrentDisplaySettings(bool *landscape, bool *flipped) { Generally we put functions like this in an anonymous namespace and not static. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:55: if (!EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &dm)) nullptr https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:55: if (!EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &dm)) Why do you use current and not the display the WebContents is on? https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:57: if (flipped) { You always pass in non-null, so why check the variables? https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:58: *flipped = (dm.dmDisplayOrientation == DMDO_270 Why not 90? https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:81: case blink::WebScreenOrientationLockLandscapeSecondary: What do primary and secondary mean in this context? The header doesn't seem to have a description, so I'm asking. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:117: AR_STATE autoRotationState; auto_rotation_state
I've had a quick look. Will let sky@ review the Windows API usage and check after if your implementation matches the requirements of the screen orientation api but from a first look, it should be okay. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:13: content::ScreenOrientationProvider::SetDelegate(this); nit: no need for "content::" https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:17: content::ScreenOrientationProvider::SetDelegate(nullptr); ditto https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:21: content::WebContents* web_contents) { ditto (and probably some more below this) https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:58: *flipped = (dm.dmDisplayOrientation == DMDO_270 On 2016/03/04 at 23:37:36, sky wrote: > Why not 90? 90 is "landscape-primary" and 270 "landscape-secondary" per screen orientation spec.
Patchset 4: Review 22 and 23 fixes. Thanks @sky and @mlamouri https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:7: #include <windows.h> On 2016/03/04 23:37:36, sky wrote: > nit: newline between 7/8. Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:13: content::ScreenOrientationProvider::SetDelegate(this); On 2016/03/05 00:39:14, Mounir Lamouri wrote: > nit: no need for "content::" Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:17: content::ScreenOrientationProvider::SetDelegate(nullptr); On 2016/03/05 00:39:14, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:21: content::WebContents* web_contents) { On 2016/03/05 00:39:14, Mounir Lamouri wrote: > ditto (and probably some more below this) Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:27: ORIENTATION_PREFERENCE orientation) { On 2016/03/04 23:37:35, sky wrote: > nit: run git cl format as your formatting is off here. Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:36: set_display_auto_rotation_preferences_func(orientation); On 2016/03/04 23:37:36, sky wrote: > How come the docs for this say it takes a pointer? > https://msdn.microsoft.com/en-us/library/windows/desktop/dn629268(v=vs.85).aspx Documentation is wrong there. A place where is it correct: https://msdn.microsoft.com/en-us/library/vs/alm/dn376361(v=vs.85).aspx https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:40: static BOOL GetAutoRotationStateWrapper(PAR_STATE pState) { On 2016/03/04 23:37:36, sky wrote: > p_state Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:40: static BOOL GetAutoRotationStateWrapper(PAR_STATE pState) { On 2016/03/04 23:37:36, sky wrote: > Is there a reason to use windows types here rather than standard c++ types? As it is a wrapper, wanted to keep the same signature and didn't want to have implicit BOOL->bool cast. Also, copied the whole wrapper approach for functions not available in previous Windows versions from here [1]. [1] https://chromium.googlesource.com/chromium/src/+blame/master/ui/base/win/touc... https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:41: typedef BOOL(WINAPI *GetAutoRotationStatePtr)(PAR_STATE); On 2016/03/04 23:37:36, sky wrote: > typedef->using Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:51: static void GetCurrentDisplaySettings(bool *landscape, bool *flipped) { On 2016/03/04 23:37:36, sky wrote: > Generally we put functions like this in an anonymous namespace and not static. Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:55: if (!EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &dm)) On 2016/03/04 23:37:36, sky wrote: > nullptr Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:55: if (!EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &dm)) On 2016/03/04 23:37:36, sky wrote: > Why do you use current and not the display the WebContents is on? I have renamed the method GetCurrentDisplaySettings to GetCurrentDisplayOrientation because "current settings ENUM_CURRENT_SETTINGS" with "current display" could be a bit misleading. First parameter (NULL) in documentation is described as "A NULL value specifies the current display device on the computer on which the calling thread is running." ENUM_CURRENT_SETTINGS - specifies currently active settings for current device. How Windows handles the cases where browser is displayed on extended screens where one of those is rotated and the other isn't? First parameter (NULL) is actually about "main" monitor - when setting multiple windows one of the monitors is marked as main (Desktop->right mouse button key->Screen resolution). Main monitor is by default the tablet monitor and in multiple monitor mode user can change orientation of screens only through Windows preferences for whole system (not per process like with this API). But the question is what would be the size of problem if this API returns e.g. portrait from attached monitor with pivot rotation that user set to be main monitor in preferences? Then, while multiple monitors are attached, there would be no orientation change comig from this API. Even after multiple monitors gets detached (then orientation api change kicks in) and attached again, orientation gets disabled again and it is in default state. Orientation change kicks in only when multiple monitors get detached (or when tablet is undocked, 2-in-1 transformed to tablet, etc). And in this case, it would be intuitive to lock in the orientation it was displayed and explicitly marked as main screen - so, keep the portrait. Important thing is that user doesn't need to restart game after detaching to get the orientation lock working. Another ways would be to: 1) assume landscape 0 rotation in case with multiple monitors 2) disable orientation lock preference setting when there are multiple monitors attached but both 1 and 2 seems wrong - I think the current way is correct. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:57: if (flipped) { On 2016/03/04 23:37:36, sky wrote: > You always pass in non-null, so why check the variables? Done. https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:81: case blink::WebScreenOrientationLockLandscapeSecondary: On 2016/03/04 23:37:36, sky wrote: > What do primary and secondary mean in this context? The header doesn't seem to > have a description, so I'm asking. Got the information from spec [1] and copied the logic from @mlamouri's Android patches [2]. Using this mapping: Orientation API | Android | Windows portrait-primary | SCREEN_ORIENTATION_PORTRAIT | ORIENTATION_PREFERENCE_PORTRAIT portrait-secondary | SCREEN_ORIENTATION_REVERSE_PORTRAIT | ORIENTATION_PREFERENCE_PORTRAIT_FLIPPED [1] https://www.w3.org/TR/screen-orientation/#idl-def-OrientationLockType.portrai... [2] https://chromium.googlesource.com/chromium/src/+blame/master/content/public/a... https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:117: AR_STATE autoRotationState; On 2016/03/04 23:37:36, sky wrote: > auto_rotation_state Done.
https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:55: if (!EnumDisplaySettings(NULL, ENUM_CURRENT_SETTINGS, &dm)) On 2016/03/06 10:34:15, aleksandar.stojiljkovic wrote: > On 2016/03/04 23:37:36, sky wrote: > > Why do you use current and not the display the WebContents is on? > I have renamed the method GetCurrentDisplaySettings to > GetCurrentDisplayOrientation because "current settings ENUM_CURRENT_SETTINGS" > with "current display" could be a bit misleading. > > First parameter (NULL) in documentation is described as "A NULL value specifies > the current display device on the computer on which the calling thread is > running." > > ENUM_CURRENT_SETTINGS - specifies currently active settings for current device. > > How Windows handles the cases where browser is displayed on extended screens > where one of those is rotated and the other isn't? > First parameter (NULL) is actually about "main" monitor - when setting multiple > windows one of the monitors is marked as main (Desktop->right mouse button > key->Screen resolution). Main monitor is by default the tablet monitor and in > multiple monitor mode user can change orientation of screens only through > Windows preferences for whole system (not per process like with this API). Again, why wouldn't you want to use the monitor the webcontents is on? > But the question is what would be the size of problem if this API returns e.g. > portrait from attached monitor with pivot rotation that user set to be main > monitor in preferences? > Then, while multiple monitors are attached, there would be no orientation change > comig from this API. Even after multiple monitors gets detached (then > orientation api change kicks in) and attached again, orientation gets disabled > again and it is in default state. > Orientation change kicks in only when multiple monitors get detached (or when > tablet is undocked, 2-in-1 transformed to tablet, etc). And in this case, it > would be intuitive to lock in the orientation it was displayed and explicitly > marked as main screen - so, keep the portrait. Important thing is that user > doesn't need to restart game after detaching to get the orientation lock > working. > > Another ways would be to: > 1) assume landscape 0 rotation in case with multiple monitors > 2) disable orientation lock preference setting when there are multiple monitors > attached but both 1 and 2 seems wrong - I think the current way is correct.
On 2016/03/07 16:20:30, sky wrote: > https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... > File content/browser/screen_orientation/screen_orientation_delegate_win.cc > (right): > Again, why wouldn't you want to use the monitor the webcontents is on? How to handle extended desktop case where parts of page are on multiple monitors - which monitor to check. Or you are referring to case only when page is fullscreen mode on external display? Even in that case, this API is usually applied on game start - not sure if it is correct to have page initially displayed on external pivot rotation screen (90 percent portrait) when API specifies "any" or "natural" to apply the portrait to tablet once external monitors get unplugged (don't know, maybe it has different aspect ratio and whole thing doesn't makes sense). Maybe it is just better to assume that for windows tablet with external monitors attached natural and any means landscape no rotation.
On 2016/03/07 16:59:53, aleksandar.stojiljkovic wrote: > On 2016/03/07 16:20:30, sky wrote: > > > https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... > > File content/browser/screen_orientation/screen_orientation_delegate_win.cc > > (right): > > > Again, why wouldn't you want to use the monitor the webcontents is on? > > How to handle extended desktop case where parts of page are on multiple monitors > - which monitor to check. > > Or you are referring to case only when page is fullscreen mode on external > display? > > Even in that case, this API is usually applied on game start - not sure if it is > correct to have page initially displayed on external pivot rotation screen (90 > percent portrait) when API specifies "any" or "natural" to apply the portrait to > tablet once external monitors get unplugged (don't know, maybe it has different > aspect ratio and whole thing doesn't makes sense). Maybe it is just better to > assume that for windows tablet with external monitors attached natural and any > means landscape no rotation. Actually, it is better if I leave multiple monitor case to @mlamouri to cover as he is the owner of orientation API. I'll prepare a patch disabling the API for multiple monitors case.
On 2016/03/07 at 17:30:42, aleksandar.stojiljkovic wrote: > On 2016/03/07 16:59:53, aleksandar.stojiljkovic wrote: > > On 2016/03/07 16:20:30, sky wrote: > > > > > https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... > > > File content/browser/screen_orientation/screen_orientation_delegate_win.cc > > > (right): > > > > > Again, why wouldn't you want to use the monitor the webcontents is on? > > > > How to handle extended desktop case where parts of page are on multiple monitors > > - which monitor to check. As far as I know, a Chrome window is only associated with one screen. The API should apply to this screen.
On Mon, Mar 7, 2016 at 8:59 AM, <aleksandar.stojiljkovic@intel.com> wrote: > On 2016/03/07 16:20:30, sky wrote: >> > https://codereview.chromium.org/1758823004/diff/40001/content/browser/screen_... >> File content/browser/screen_orientation/screen_orientation_delegate_win.cc >> (right): > >> Again, why wouldn't you want to use the monitor the webcontents is on? > > How to handle extended desktop case where parts of page are on multiple > monitors > - which monitor to check. If a window spans multiple monitors isn't there a notion of the main monitor the window is on? This is important for multiple displays with differing dpi as well. -Scott > > Or you are referring to case only when page is fullscreen mode on external > display? > > Even in that case, this API is usually applied on game start - not sure if > it is > correct to have page initially displayed on external pivot rotation screen > (90 > percent portrait) when API specifies "any" or "natural" to apply the > portrait to > tablet once external monitors get unplugged (don't know, maybe it has > different > aspect ratio and whole thing doesn't makes sense). Maybe it is just better > to > assume that for windows tablet with external monitors attached natural and > any > means landscape no rotation. > > > > https://codereview.chromium.org/1758823004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patch set 5: Disable API in multi monitor case. On 2016/03/07 18:53:57, sky wrote: > If a window spans multiple monitors isn't there a notion of the main > monitor the window is on? This is important for multiple displays with > differing dpi as well. > > -Scott Not aware of such API. There is a "main" monitor for the system (commented on it in #23). On 2016/03/07 18:30:36, Mounir Lamouri wrote: > As far as I know, a Chrome window is only associated with one screen. The API > should apply to this screen. It is clear from the beginning,... I tried to misuse the API to cover also multiple displays use case, but that is wrong as it makes the implementation confusing and potentially introducing bugs. Feel I was wasting your time with this - sorry. The patch should be clear now.
On 2016/03/07 23:26:49, aleksandar.stojiljkovic wrote: > Patch set 5: Disable API in multi monitor case. > > On 2016/03/07 18:53:57, sky wrote: > > If a window spans multiple monitors isn't there a notion of the main > > monitor the window is on? This is important for multiple displays with > > differing dpi as well. > > > > -Scott > > Not aware of such API. There is a "main" monitor for the system (commented on it > in #23). > > > On 2016/03/07 18:30:36, Mounir Lamouri wrote: > > As far as I know, a Chrome window is only associated with one screen. The API > > should apply to this screen. > > It is clear from the beginning,... I tried to misuse the API to cover also > multiple displays use case, but that is wrong as it makes the implementation > confusing and potentially introducing bugs. > > Feel I was wasting your time with this - sorry. The patch should be clear now. @sky, @scottmg Does the patch now look OK? Avi is content owner and gave LGTM related to content but it was needed to have Windows API expert check on Windows API usage. Thanks.
LGTM
https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:39: DEVMODE dm; dm = {}; zeroes it out nicer. https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:43: return; return false; else how is the caller telling success from not?
https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:118: AR_STATE auto_rotation_state; same here = {}
https://codereview.chromium.org/1758823004/diff/80001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1758823004/diff/80001/content/browser/browser... content/browser/browser_main_loop.cc:600: screen_orientation_delegate_.reset(new ScreenOrientationDelegateWin()); can this be later? like in PostMainMessageLoopStart?
Patch set 6:#32 & #33 review fixes - thanks @cpu https://codereview.chromium.org/1758823004/diff/80001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1758823004/diff/80001/content/browser/browser... content/browser/browser_main_loop.cc:600: screen_orientation_delegate_.reset(new ScreenOrientationDelegateWin()); On 2016/03/11 02:12:01, cpu wrote: > can this be later? like in PostMainMessageLoopStart? Don't know. Same thing for Android is in this method (line 660) so I thought it is better to keep them together. https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_delegate_win.cc (right): https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:39: DEVMODE dm; On 2016/03/11 02:08:17, cpu wrote: > dm = {}; zeroes it out nicer. Done. https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:43: return; On 2016/03/11 02:08:16, cpu wrote: > return false; > > else how is the caller telling success from not? Done. Calling method (ScreenOrientationDelegateWin::Lock) doesn't return value and my initial idea was the same as discussed for multiple monitors case - that best effort locking is better than no locking (in case of multiple monitors). This is corrected later so good to correct it here - in case that EnumDisplaySettings windows API fails, no locking happens. https://codereview.chromium.org/1758823004/diff/80001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_delegate_win.cc:118: AR_STATE auto_rotation_state; On 2016/03/11 02:09:36, cpu wrote: > same here = {} Done.
lgtm
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1758823004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1758823004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1758823004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1758823004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, sky@chromium.org, cpu@chromium.org Link to the patchset: https://codereview.chromium.org/1758823004/#ps120001 (title: "rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1758823004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1758823004/120001
Message was sent while issue was closed.
Description was changed from ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. BUG=423919, 400846 ========== to ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. BUG=423919, 400846 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. BUG=423919, 400846 ========== to ========== Screen.orientation lock API implementation for Windows8 and later. Enabled for Windows 8 and later. Properly eabled in Tablet mode only; locks and unlocks orientation in full screen and windowed mode. Tested on Windows8 Yoga 12 laptop only. BUG=423919, 400846 Committed: https://crrev.com/4f4b5efee23bcf51ccd96c23bd67cdc7bb1e07e2 Cr-Commit-Position: refs/heads/master@{#380893} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4f4b5efee23bcf51ccd96c23bd67cdc7bb1e07e2 Cr-Commit-Position: refs/heads/master@{#380893}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2761213003/ by aleksandar.stojiljkovic@intel.com. The reason for reverting is: Random behavior on different Windows tablets. Probable cause of Issue 702435. Needs to be debugged - it works on some hardware and somewhere it doesn't, we need to evaluate if it makes sense to take the risk with this one. . |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
