Call SendScreenRects() when the content has been resized.

Call SendScreenRects() when the content has been resized.

RenderWidgetHostView::WasResized() will only update the RenderWidget's
size and propagate the update to its WebWidget. However, window.outer*
will use window_screen_rect_, updated when receiving an
UpdateScreenRects message. We need to make sure that value is updated
before notifying about a resize.

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

[jdduke (slow), 2014-04-22 19:27:25]

aelias@ or dtrainor@ are better reviewers for this kind of change.

https://codereview.chromium.org/247933002/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/247933002/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:1376: RenderWidgetHostImpl*
host = RenderWidgetHostImpl::From(
I'm probably not the best reviewer for this, but coupling the side-effects here
seems a little odd.  If they should indeed be coupled, probably best to let
RenderWidgetHostViewAndroid do that, but again I'm somewhat ignorant in this
regard.

[aelias_OOO_until_Jul13, 2014-04-22 21:24:39]

There is a call to UpdateScreenInfo in
ContentViewCoreImpl::SendOrientationChangeEventInternal.  I assume that happens
too late?  Can we remove that one if we're adding a new one?

Secondly, would it work to call RenderWidgetHostViewBase::UpdateScreenInfo()
instead?  It looks like that should call both SendScreenRects and WasResized.

[mlamouri (slow - plz ping), 2014-04-25 13:42:28]

(Argh, I was wondering why no one replied and I realised I forgot to actually
click on "send"...)

On 2014/04/22 21:24:39, aelias wrote:
> There is a call to UpdateScreenInfo in
> ContentViewCoreImpl::SendOrientationChangeEventInternal.  I assume that
happens
> too late?  Can we remove that one if we're adding a new one?

If I remember correctly, the screen orientation happens _before_. We could
probably remove the call in the screen orientation function though.

> Secondly, would it work to call RenderWidgetHostViewBase::UpdateScreenInfo()
> instead?  It looks like that should call both SendScreenRects and WasResized.

Not really. UpdateScreenInfo() is abused. I am looking into doing some cleanup
around that in order to have a better way to propagate proper screen info
changes. See https://code.google.com/p/chromium/issues/detail?id=366848

UpdateScreenInfo() was not meant to update the screen information as in when
WebScreenInfo changes. It was meant to fix this bug:
https://code.google.com/p/chromium/issues/detail?id=126586 Basically,
UpdateScreenInfo() should be called when a window has moved and might have
changed screen in a multi-screen configuration.

Calling WasResized() was the right thing to do here but UpdateScreenInfo()
called by the screen orientation code used to call SendScreenRects with the
right values. This is no longer the case because the view might not have been
resized when we know about the screen orientation change. The right thing to do
is to make sure we call SendScreenRect() and it seems that there are other
places where those two methods are called in this order.

Also, it's worth noting that this is fixing a regression and it would be great
to keep the patch as the bare minimum to reduce the risk of cherry-picking it to
a branch. I would be in favour of keep UpdateScreenInfo() in the screen
orientation update code for that. I am going to rewrite part of that anyway.

Finally, it is possible that the problem is bigger and we do not want to expose
a screen orientation change to the page before the window was properly resized.
Though, this is an orthogonal problem.

[aelias_OOO_until_Jul13, 2014-04-25 20:01:40]

OK.  lgtm modulo comment below.

On 2014/04/25 13:42:28, Mounir Lamouri wrote:
> (Argh, I was wondering why no one replied and I realised I forgot to actually
> click on "send"...)
> 
> On 2014/04/22 21:24:39, aelias wrote:
> > There is a call to UpdateScreenInfo in
> > ContentViewCoreImpl::SendOrientationChangeEventInternal.  I assume that
> happens
> > too late?  Can we remove that one if we're adding a new one?
> 
> If I remember correctly, the screen orientation happens _before_. We could
> probably remove the call in the screen orientation function though.

OK.  Please remove the call in the screen orientation function, then.

[mlamouri (slow - plz ping), 2014-04-28 16:42:33]

On 2014/04/25 20:01:40, aelias wrote:
> OK.  lgtm modulo comment below.
> 
> On 2014/04/25 13:42:28, Mounir Lamouri wrote:
> > (Argh, I was wondering why no one replied and I realised I forgot to
actually
> > click on "send"...)
> > 
> > On 2014/04/22 21:24:39, aelias wrote:
> > > There is a call to UpdateScreenInfo in
> > > ContentViewCoreImpl::SendOrientationChangeEventInternal.  I assume that
> > happens
> > > too late?  Can we remove that one if we're adding a new one?
> > 
> > If I remember correctly, the screen orientation happens _before_. We could
> > probably remove the call in the screen orientation function though.
> 
> OK.  Please remove the call in the screen orientation function, then.

Actually, sorry for being inaccurate. It makes sense to have that nowadays
because WebScreenInfo caries the orientation information too. We don't yet
really use it but I have CL that makes use of it and uses a similar call at that
place.

[mlamouri (slow - plz ping), 2014-04-28 16:43:26]

jdduke@ or bulach@, could one of you rubberstamp this so I can land?

[jdduke (slow), 2014-04-28 16:44:11]

lgtm

[mlamouri (slow - plz ping), 2014-04-28 16:45:38]

On 2014/04/28 16:44:11, jdduke wrote:
> lgtm

Amazing reactivity <3

[mlamouri (slow - plz ping), 2014-04-28 16:45:42]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:46:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/247933002/1

[jdduke (slow), 2014-04-28 16:50:15]

The CQ bit was unchecked by jdduke@chromium.org

[jdduke (slow), 2014-04-28 16:51:09]

Oops, I didn't see your follow-up comment to the request from aelias@, please
run it by him first before landing, thanks.

[mlamouri (slow - plz ping), 2014-04-28 16:52:18]

On 2014/04/28 16:42:33, Mounir Lamouri wrote:
> On 2014/04/25 20:01:40, aelias wrote:
> > OK.  lgtm modulo comment below.
> > 
> > On 2014/04/25 13:42:28, Mounir Lamouri wrote:
> > > (Argh, I was wondering why no one replied and I realised I forgot to
> actually
> > > click on "send"...)
> > > 
> > > On 2014/04/22 21:24:39, aelias wrote:
> > > > There is a call to UpdateScreenInfo in
> > > > ContentViewCoreImpl::SendOrientationChangeEventInternal.  I assume that
> > > happens
> > > > too late?  Can we remove that one if we're adding a new one?
> > > 
> > > If I remember correctly, the screen orientation happens _before_. We could
> > > probably remove the call in the screen orientation function though.
> > 
> > OK.  Please remove the call in the screen orientation function, then.
> 
> Actually, sorry for being inaccurate. It makes sense to have that nowadays
> because WebScreenInfo caries the orientation information too. We don't yet
> really use it but I have CL that makes use of it and uses a similar call at
that
> place.

Also, I want to send that patch to Beta because it's a compat issue. Removing
that call would increase the chances of creating regressions.

[aelias_OOO_until_Jul13, 2014-04-28 17:35:52]

OK, lgtm even without removing the extra call.

[mlamouri (slow - plz ping), 2014-04-28 17:41:49]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:42:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/247933002/1

[commit-bot: I haz the power, 2014-04-28 19:37:53]

Change committed as 266631

[pals, 2014-04-29 17:04:58]

On 2014/04/25 13:42:28, Mounir Lamouri wrote:
> (Argh, I was wondering why no one replied and I realised I forgot to actually
> click on "send"...)
> 
> On 2014/04/22 21:24:39, aelias wrote:
> > There is a call to UpdateScreenInfo in
> > ContentViewCoreImpl::SendOrientationChangeEventInternal.  I assume that
> happens
> > too late?  Can we remove that one if we're adding a new one?
> 
> If I remember correctly, the screen orientation happens _before_. We could
> probably remove the call in the screen orientation function though.
> 
> > Secondly, would it work to call RenderWidgetHostViewBase::UpdateScreenInfo()
> > instead?  It looks like that should call both SendScreenRects and
WasResized.
> 
> Not really. UpdateScreenInfo() is abused. I am looking into doing some cleanup
> around that in order to have a better way to propagate proper screen info
> changes. See https://code.google.com/p/chromium/issues/detail?id=366848
> 
> UpdateScreenInfo() was not meant to update the screen information as in when
> WebScreenInfo changes. It was meant to fix this bug:
> https://code.google.com/p/chromium/issues/detail?id=126586 Basically,
> UpdateScreenInfo() should be called when a window has moved and might have
> changed screen in a multi-screen configuration.
> 
> Calling WasResized() was the right thing to do here but UpdateScreenInfo()
> called by the screen orientation code used to call SendScreenRects with the
> right values. This is no longer the case because the view might not have been
> resized when we know about the screen orientation change. The right thing to
do
> is to make sure we call SendScreenRect() and it seems that there are other
> places where those two methods are called in this order.
> 
> Also, it's worth noting that this is fixing a regression and it would be great
> to keep the patch as the bare minimum to reduce the risk of cherry-picking it
to
> a branch. I would be in favour of keep UpdateScreenInfo() in the screen
> orientation update code for that. I am going to rewrite part of that anyway.
> 
> Finally, it is possible that the problem is bigger and we do not want to
expose
> a screen orientation change to the page before the window was properly
resized.
> Though, this is an orthogonal problem.