[DevTools] Implement screen orientation override.

[DevTools] Implement screen orientation override.

BUG=427199, 505293
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198077

[dgozman, 2015-06-23 16:47:58]

dgozman@chromium.org changed reviewers:
+ mlamouri@chromium.org, pfeldman@chromium.org

[dgozman, 2015-06-23 16:47:58]

Could you please take a look?

pfeldman - at devtools;
mlamouri - at screen_orientation.

Thanks,
Dmitry

[pfeldman, 2015-06-24 13:57:01]

lgtm

[mlamouri (slow - plz ping), 2015-06-24 16:51:06]

It sgtm. Though, I would like to make sure that you will not allow situations
where the screen is set to landscape but the width is smaller than the height
(and the other way around).

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationController.cpp:115: unsigned
short currentAngle = m_override ? m_overrideAngle :
frame()->host()->chromeClient().screenInfo().orientationAngle;
nit: could you have in internal method that does that logic?

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
File Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp
(right):

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp:63: *error
= "Angle should be between -360 and 360";
No, it should be between in [ 0; 360 [ (ie. >=0 && <360)

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp:102: // No
need to cleanup previous one, as it's already gone.
I'm confused. What are you trying to do here?

[dgozman, 2015-06-25 12:21:26]

PTAL

> It sgtm. Though, I would like to make sure that you will not allow situations
> where the screen is set to landscape but the width is smaller than the height
> (and the other way around).
In DevTools, we set landscape for width > height, but I cannot guarantee that
for external debuggers using our protocol. Is that a problem? I thought
screen_orientation is fairly isolated from the world around and nothing should
break.

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationController.cpp:115: unsigned
short currentAngle = m_override ? m_overrideAngle :
frame()->host()->chromeClient().screenInfo().orientationAngle;
On 2015/06/24 16:51:06, Mounir Lamouri wrote:
> nit: could you have in internal method that does that logic?

Done.

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
File Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp
(right):

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp:63: *error
= "Angle should be between -360 and 360";
On 2015/06/24 16:51:06, Mounir Lamouri wrote:
> No, it should be between in [ 0; 360 [ (ie. >=0 && <360)

Fixed.

https://codereview.chromium.org/1195793008/diff/1/Source/modules/screen_orien...
Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp:102: // No
need to cleanup previous one, as it's already gone.
On 2015/06/24 16:51:06, Mounir Lamouri wrote:
> I'm confused. What are you trying to do here?

That is a leftover from Page-based solution. Removed.

[mlamouri (slow - plz ping), 2015-06-26 10:10:53]

lgtm with the comments applied.

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:56:
WebScreenOrientationType ScreenOrientationController::computeOrientation(const
IntRect& rect, uint16_t rotation)
I'm not sure why you are changing this. You could take chromeClient and get the
effectiveAngle() from there. Is there a reason I'm missing?

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp
(right):

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp:63: *error
= "Angle should be in [0; 360) interval";
nit: what's [x; y) notation? it seems to me that it should be [x; y[ here (y
being excluded). But maybe these two notations are equivalent?

[dgozman, 2015-06-26 10:18:39]

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationController.cpp:56:
WebScreenOrientationType ScreenOrientationController::computeOrientation(const
IntRect& rect, uint16_t rotation)
On 2015/06/26 10:10:53, Mounir Lamouri wrote:
> I'm not sure why you are changing this. You could take chromeClient and get
the
> effectiveAngle() from there. Is there a reason I'm missing?

This is a static method, and we need m_overrideXXX fields for effectiveAngle.

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp
(right):

https://codereview.chromium.org/1195793008/diff/40001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientationInspectorAgent.cpp:63: *error
= "Angle should be in [0; 360) interval";
On 2015/06/26 10:10:53, Mounir Lamouri wrote:
> nit: what's [x; y) notation? it seems to me that it should be [x; y[ here (y
> being excluded). But maybe these two notations are equivalent?

Those are equivalent, it's just more natural for me. See
https://en.wikipedia.org/wiki/Interval_(mathematics). Ican change though.

[dgozman, 2015-06-26 11:35:24]

I will go ahead and land this. If you have suggestions on how to better shape
the code in ScreenOrientationController, I'll land a follow up.

[dgozman, 2015-06-26 11:35:29]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-06-26 11:35:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1195793008/#ps40001
(title: "error tests")

[commit-bot: I haz the power, 2015-06-26 11:35:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195793008/40001

[commit-bot: I haz the power, 2015-06-26 12:42:13]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197908

[amineer_google, 2015-06-28 21:58:04]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1220643002/ by amineer@google.com.

The reason for reverting is: Causing crashes
BUG=505293.

[dgozman, 2015-06-30 14:50:19]

Relanding with null checks.

[dgozman, 2015-06-30 14:50:25]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-06-30 14:50:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1195793008/#ps80001
(title: "Restore test")

[commit-bot: I haz the power, 2015-06-30 14:50:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195793008/80001

[commit-bot: I haz the power, 2015-06-30 17:49:30]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198077