Call RenderViewImpl::SetScreenOrientationForTesting to make sure that events are not sent when orie…

Call RenderViewImpl::SetScreenOrientationForTesting from MockScreenOrientationController::UpdateScreenOrientation() to make sure that events are not sent when orientation is locked.

Existing implementation calls SetScreenOrientationForTesting and sends screen
orientation events even when the test screen orientation is locked.

BUG=162827
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276517

[Inactive, 2014-05-28 12:02:20]

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:835: if
(RenderThreadImpl::current()->layout_test_mode()) {
nit: Useless curly brackets?

https://codereview.chromium.org/302553007/diff/1/content/renderer/renderer_we...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/renderer_we...
content/renderer/renderer_webkitplatformsupport_impl.cc:1089: if
(!(g_test_screen_orientation_controller == 0)) {
nit: useless curly brackets (I don't know if this matters on Chromium side).

https://codereview.chromium.org/302553007/diff/1/content/renderer/screen_orie...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/screen_orie...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:75:
listener_->didChangeScreenOrientation(current_orientation_);
BTW, we should get rid of the old listener old in a follow-up, now that the
Blink side no longer relies on it.

[mlamouri (slow - plz ping), 2014-05-28 13:45:19]

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:836:
RendererWebKitPlatformSupportImpl::ResetMockScreenOrientationRenderView();
Can't we make the render_view_ reset happen in
ResetMockScreenOrientationForTesting() instead? Adding things in
render_view_impl makes me uneasy and I given that we call
ResetMockScreenOrientationForTesting() between tests, we should be fine if we
reset the render_view_ member there, right?

https://codereview.chromium.org/302553007/diff/1/content/renderer/screen_orie...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/screen_orie...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:56: }
I guess you can do:
  assert(!render_view_ || render_view_ == render_view);
  render_view_ = render_view;

It is easier to read I think.

[ostap, 2014-05-28 19:17:04]

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:835: if
(RenderThreadImpl::current()->layout_test_mode()) {
On 2014/05/28 12:02:20, Chris Dumez wrote:
> nit: Useless curly brackets?

Done.

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:836:
RendererWebKitPlatformSupportImpl::ResetMockScreenOrientationRenderView();
On 2014/05/28 13:45:20, Mounir Lamouri wrote:
> Can't we make the render_view_ reset happen in
> ResetMockScreenOrientationForTesting() instead? Adding things in
> render_view_impl makes me uneasy and I given that we call
> ResetMockScreenOrientationForTesting() between tests, we should be fine if we
> reset the render_view_ member there, right?

I think using ResetMockScreenOrientationForTesting() is a good idea since it is
not necessary to introduce another method.
But I think it is still required to track render view deallocation because we
can get difficult to debug crash if one of tests fails to call
ResetMockScreenOrientationForTesting() for some reason.

https://codereview.chromium.org/302553007/diff/1/content/renderer/renderer_we...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/renderer_we...
content/renderer/renderer_webkitplatformsupport_impl.cc:1089: if
(!(g_test_screen_orientation_controller == 0)) {
On 2014/05/28 12:02:20, Chris Dumez wrote:
> nit: useless curly brackets (I don't know if this matters on Chromium side).

Done.

https://codereview.chromium.org/302553007/diff/1/content/renderer/screen_orie...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/screen_orie...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:56: }
On 2014/05/28 13:45:20, Mounir Lamouri wrote:
> I guess you can do:
>   assert(!render_view_ || render_view_ == render_view);
>   render_view_ = render_view;
> 
> It is easier to read I think.

Done.

[mlamouri (slow - plz ping), 2014-05-29 14:51:54]

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:836:
RendererWebKitPlatformSupportImpl::ResetMockScreenOrientationRenderView();
On 2014/05/28 19:17:05, ostap wrote:
> On 2014/05/28 13:45:20, Mounir Lamouri wrote:
> > Can't we make the render_view_ reset happen in
> > ResetMockScreenOrientationForTesting() instead? Adding things in
> > render_view_impl makes me uneasy and I given that we call
> > ResetMockScreenOrientationForTesting() between tests, we should be fine if
we
> > reset the render_view_ member there, right?
> 
> I think using ResetMockScreenOrientationForTesting() is a good idea since it
is
> not necessary to introduce another method.
> But I think it is still required to track render view deallocation because we
> can get difficult to debug crash if one of tests fails to call
> ResetMockScreenOrientationForTesting() for some reason.

AFAIK, we use the same RenderView during the LayoutTests so we shouldn't even
have to call this and ::Reset() is meant to be used exactly for cleaning up
things between tests. I'm not an owner of that file, but I think adding this
there isn't right.

[ostap, 2014-05-29 20:09:16]

On 2014/05/29 14:51:54, Mounir Lamouri wrote:
>
https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/302553007/diff/1/content/renderer/render_view...
> content/renderer/render_view_impl.cc:836:
> RendererWebKitPlatformSupportImpl::ResetMockScreenOrientationRenderView();
> On 2014/05/28 19:17:05, ostap wrote:
> > On 2014/05/28 13:45:20, Mounir Lamouri wrote:
> > > Can't we make the render_view_ reset happen in
> > > ResetMockScreenOrientationForTesting() instead? Adding things in
> > > render_view_impl makes me uneasy and I given that we call
> > > ResetMockScreenOrientationForTesting() between tests, we should be fine if
> we
> > > reset the render_view_ member there, right?
> > 
> > I think using ResetMockScreenOrientationForTesting() is a good idea since it
> is
> > not necessary to introduce another method.
> > But I think it is still required to track render view deallocation because
we
> > can get difficult to debug crash if one of tests fails to call
> > ResetMockScreenOrientationForTesting() for some reason.
> 
> AFAIK, we use the same RenderView during the LayoutTests so we shouldn't even
> have to call this and ::Reset() is meant to be used exactly for cleaning up
> things between tests. I'm not an owner of that file, but I think adding this
> there isn't right.


Chris convinced me that Reset will be called even if JS fails.

[mlamouri (slow - plz ping), 2014-05-30 09:07:56]

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083: if
(!(g_test_screen_orientation_controller == 0)) {
Why do you need a null check that you didn't need before? Should that be an
ASSERT if this is only preventive?

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1085:
g_test_screen_orientation_controller.Get().ResetRenderView();
Is there a reason why ResetRenderView() should be different from ResetData() is
ResetData() called in other situations than a test runner reset?

[ostap, 2014-05-30 15:21:43]

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083: if
(!(g_test_screen_orientation_controller == 0)) {
On 2014/05/30 09:07:57, Mounir Lamouri wrote:
> Why do you need a null check that you didn't need before? Should that be an
> ASSERT if this is only preventive?

Done.

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1085:
g_test_screen_orientation_controller.Get().ResetRenderView();
On 2014/05/30 09:07:57, Mounir Lamouri wrote:
> Is there a reason why ResetRenderView() should be different from ResetData()
is
> ResetData() called in other situations than a test runner reset?

Done.

[mlamouri (slow - plz ping), 2014-05-30 15:39:46]

LGTM. Thanks for the patch! :)

[jochen (gone - plz use gerrit), 2014-06-02 07:21:24]

https://codereview.chromium.org/302553007/diff/90001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/90001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083:
assert(!(g_test_screen_orientation_controller == 0));
DCHECK(g_test_screen_orientation_controller)

https://codereview.chromium.org/302553007/diff/90001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1084:
g_test_screen_orientation_controller.Get().ResetData();
.Get() will create an controller btw

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:73:
assert(!render_view_ || render_view_ == render_view);
DCHECK

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:89: if
(render_view_)
nit. add { } because of > 1line body

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:90:
static_cast<RenderViewImpl*>(render_view_)
how can you guarantee that e.g. the render view didn't die mid-test? was it
possible to use a RenderViewObserver to make sure that the render view is still
alive?

[ostap, 2014-06-03 00:11:53]

https://codereview.chromium.org/302553007/diff/90001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/90001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083:
assert(!(g_test_screen_orientation_controller == 0));
On 2014/06/02 07:21:25, jochen wrote:
> DCHECK(g_test_screen_orientation_controller)

LazyInstance implements only == operator, so it has to be 

DCHECK(!(g_test_screen_orientation_controller == 0));

https://codereview.chromium.org/302553007/diff/90001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1084:
g_test_screen_orientation_controller.Get().ResetData();
On 2014/06/02 07:21:25, jochen wrote:
> .Get() will create an controller btw

Yes. ResetMockScreenOrientationForTesting() shouldn't be called without
SetMockDeviceMotionDataForTesting().
That's why there is DCHECK before.

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:73:
assert(!render_view_ || render_view_ == render_view);
On 2014/06/02 07:21:25, jochen wrote:
> DCHECK

Done.

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:89: if
(render_view_)
On 2014/06/02 07:21:25, jochen wrote:
> nit. add { } because of > 1line body

Done.

https://codereview.chromium.org/302553007/diff/90001/content/renderer/screen_...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:90:
static_cast<RenderViewImpl*>(render_view_)
On 2014/06/02 07:21:25, jochen wrote:
> how can you guarantee that e.g. the render view didn't die mid-test? was it
> possible to use a RenderViewObserver to make sure that the render view is
still
> alive?

At previous patches I did call from ~RenderViewImpl() which was removed by
Mounir request. Completely forgot about RenderViewObserver.

Done.

[jochen (gone - plz use gerrit), 2014-06-03 12:52:25]

lgtm with nits addressed

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
File content/public/renderer/render_view_observer.cc (right):

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
content/public/renderer/render_view_observer.cc:16: // |render_view| can be NULL
on unit testing.
please update this comment

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
content/public/renderer/render_view_observer.cc:56: void
RenderViewObserver::SetRenderView(RenderView* render_view) {
in WebContentsObserver, we call this method Observe()

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
content/public/renderer/render_view_observer.h:123: void
SetRenderView(RenderView* render_view);
please add a comment explaining this API

https://codereview.chromium.org/302553007/diff/150001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/150001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083:
DCHECK(!(g_test_screen_orientation_controller == 0));
ok, but then it should probably be DCHECK_NE()

https://codereview.chromium.org/302553007/diff/150001/content/renderer/screen...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/150001/content/renderer/screen...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:76:
assert(this->render_view() == render_view);
DCHECK_EQ

[ostap, 2014-06-03 15:32:31]

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
File content/public/renderer/render_view_observer.cc (right):

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
content/public/renderer/render_view_observer.cc:16: // |render_view| can be NULL
on unit testing.
On 2014/06/03 12:52:25, jochen wrote:
> please update this comment

Done.

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
content/public/renderer/render_view_observer.cc:56: void
RenderViewObserver::SetRenderView(RenderView* render_view) {
On 2014/06/03 12:52:25, jochen wrote:
> in WebContentsObserver, we call this method Observe()

Done.

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/302553007/diff/150001/content/public/renderer...
content/public/renderer/render_view_observer.h:123: void
SetRenderView(RenderView* render_view);
On 2014/06/03 12:52:25, jochen wrote:
> please add a comment explaining this API

Done.

https://codereview.chromium.org/302553007/diff/150001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/150001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083:
DCHECK(!(g_test_screen_orientation_controller == 0));
On 2014/06/03 12:52:25, jochen wrote:
> ok, but then it should probably be DCHECK_NE()

Unfortunately DCHECK_NE() doesn't work in this case.
DCHECK_NE uses "!=" operator and it is not implemented for LazyInstance class.
Only "==" is implemented.

https://codereview.chromium.org/302553007/diff/150001/content/renderer/screen...
File content/renderer/screen_orientation/mock_screen_orientation_controller.cc
(right):

https://codereview.chromium.org/302553007/diff/150001/content/renderer/screen...
content/renderer/screen_orientation/mock_screen_orientation_controller.cc:76:
assert(this->render_view() == render_view);
On 2014/06/03 12:52:25, jochen wrote:
> DCHECK_EQ

Done.

[mlamouri (slow - plz ping), 2014-06-05 17:04:46]

Is there something missing to land this CL?

[ostap, 2014-06-06 00:17:47]

On 2014/06/05 17:04:46, Mounir Lamouri wrote:
> Is there something missing to land this CL?

I waited for reply about DCHECK_NE not working for LazyInstance, but it this is
not very important, it can be landed.

[jochen (gone - plz use gerrit), 2014-06-06 13:28:16]

On 2014/06/06 00:17:47, ostap wrote:
> On 2014/06/05 17:04:46, Mounir Lamouri wrote:
> > Is there something missing to land this CL?
> 
> I waited for reply about DCHECK_NE not working for LazyInstance, but it this
is
> not very important, it can be landed.

ok

[ostap, 2014-06-06 21:06:05]

The CQ bit was checked by sl.ostapenko@samsung.com

[commit-bot: I haz the power, 2014-06-06 21:07:04]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/302553007/170001

[commit-bot: I haz the power, 2014-06-06 21:49:56]

Change committed as 275535

[abarth-chromium, 2014-06-07 00:03:25]

A revert of this CL has been created in
https://codereview.chromium.org/323663002/ by abarth@chromium.org.

The reason for reverting is: Caused every LayoutTest to ASSERT in every
configuration..

[ostap, 2014-06-10 18:02:53]

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/302553007/diff/70001/content/renderer/rendere...
content/renderer/renderer_webkitplatformsupport_impl.cc:1083: if
(!(g_test_screen_orientation_controller == 0)) {
On 2014/05/30 09:07:57, Mounir Lamouri wrote:
> Why do you need a null check that you didn't need before? Should that be an
> ASSERT if this is only preventive?

Actually, now Reset is always called on every layout test shutdown. It causes
create of g_test_screen_orientation_controller on shutdown even if it was not
used in test.
The latest patch fixes this problem.

[ostap, 2014-06-11 18:29:42]

The CQ bit was checked by sl.ostapenko@samsung.com

[commit-bot: I haz the power, 2014-06-11 18:32:57]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/302553007/190001

[commit-bot: I haz the power, 2014-06-12 03:40:08]

Change committed as 276517