cc: Only allow trees created at the current viewport size to draw.

cc: Only allow trees created at the current viewport size to draw.

When we're on a small page whose WebKit layout size is equal to the
viewport size, and the viewport size grows, we don't have enough content
to fill the screen until the pending tree finishes rasterizing.  This
patch disallows drawing on the active tree when the viewport size
has recently changed.

NOTRY=true
BUG=173449
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182357

[aelias_OOO_until_Jul13, 2013-02-13 05:31:32]

Hi, this is the fix for the transient google maps keyboard glitch.

[nduca, 2013-02-13 05:35:21]

Nice! So, dumb question, once you know that there's a viewport mismatch as
you've done here, can you just enable clearing for that frame?

That may be more desirable because there are cases in the scheduler where
declining to draw can itself cause issues.

[aelias_OOO_until_Jul13, 2013-02-13 05:39:29]

The content ends up actually positioned incorrectly (at the bottom instead of
the top) so clearing is not enough.  We'd have to also default the renderer
towards painting at the top of the viewport instead of the bottom if it's
unfilled.

[nduca, 2013-02-13 05:41:09]

Got it. if you did, though, you'd white flash?

[danakj, 2013-02-13 05:44:30]

https://codereview.chromium.org/12212156/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12212156/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1100: m_deviceViewportSize = deviceViewportSize;
Can you just remove the m_deviceViewportSize from the host then? It's just
duplicating the value on the pending tree now, no?

In fact can you move both viewport sizes to the trees? Seems like this whole
function is redundant with what happens in the LTImpl.

https://codereview.chromium.org/12212156/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1110: m_renderer->viewportChanged();
You want this to happen when the active tree's viewport changes, right? Should
move this to LTImpl and do that?

[aelias_OOO_until_Jul13, 2013-02-13 06:37:29]

Yes, it would white flash instead of black/corrupt flash.

OK, in this patchset I moved the values out of LTHI entirely.  I was a little
ambivalent about it because it means treating the pendingTree value as the
canonical current viewport size, which is not something we've been doing for
anything else.  But overall that's probably less confusing than holding onto a
redundant value.

[danakj, 2013-02-13 06:46:53]

On Tue, Feb 12, 2013 at 10:37 PM, <aelias@chromium.org> wrote:

> Yes, it would white flash instead of black/corrupt flash.
>
> OK, in this patchset I moved the values out of LTHI entirely.  I was a
> little
> ambivalent about it because it means treating the pendingTree value as the
> canonical current viewport size, which is not something we've been doing
> for
> anything else.  But overall that's probably less confusing than holding
> onto a
> redundant value.
>

It is actually consistent with other things like the source frame number,
the page scale factor, etc ie. state from the main thread. :)


>
>
https://codereview.chromium.**org/12212156/<https://codereview.chromium.org/1...
>

[danakj, 2013-02-13 06:51:35]

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:293: gfx::SizeF viewportSize =
gfx::ScaleSize(deviceViewportSize(), 1 / m_deviceScaleFactor);
Is this right? The rootScrollLayer() it's using here is from the active tree. Do
you want the active tree's deviceViewportSize here?

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1436:
scrollEnd.ClampToMax(gfx::RectF(scaledScrollableSize).bottom_right() -
gfx::Rect(deviceViewportSize()).bottom_right());
Should this be the active tree's deviceViewportSize()?

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1726: state->Set("device_viewport_size",
MathUtil::asValue(deviceViewportSize()).release());
This should be the active tree too right?

I feel like the deviceViewportSize() method returning the pending tree sometimes
is inconsistent with other helper methods on LTHImpl and is confusing things
here. It's a nice trick but only gives the right answer in canDraw().

Can you change the *ViewportSize() methods to return the active tree values, and
have canDraw just do the right thing there?

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_impl.cc#newc...
cc/layer_tree_impl.cc:274: {
chromium style in this file

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_impl.cc#newc...
cc/layer_tree_impl.cc:275: if (device_viewport_size_ == device_viewport_size &&
layout_viewport_size_ == layout_viewport_size)
80col

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_impl.h
File cc/layer_tree_impl.h (right):

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_impl.h#newco...
cc/layer_tree_impl.h:139: void SetViewportSize(const gfx::Size&
layout_viewport_size, const gfx::Size& device_viewport_size);
no const&

[aelias_OOO_until_Jul13, 2013-02-13 07:37:08]

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:293: gfx::SizeF viewportSize =
gfx::ScaleSize(deviceViewportSize(), 1 / m_deviceScaleFactor);
On 2013/02/13 06:51:36, danakj wrote:
> Is this right? The rootScrollLayer() it's using here is from the active tree.
Do
> you want the active tree's deviceViewportSize here?

I definitely want pendingTree size here.  pageScaleAnimations can occur when the
keyboard is brought up (to zoom in tiny text boxes) and we want to zoom in using
the post-keyboard viewport to make sure the textbox is not covered.  I checked
it causes a bug on google.com results page bottom search box if we use
activeTree here.

Probably everything else here should use pendingTree as well although it would
rarely come up... I'd rather not touch it in this patch but I'll file a bug.

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1436:
scrollEnd.ClampToMax(gfx::RectF(scaledScrollableSize).bottom_right() -
gfx::Rect(deviceViewportSize()).bottom_right());
On 2013/02/13 06:51:36, danakj wrote:
> Should this be the active tree's deviceViewportSize()?

This is dead code due for deletion, doesn't matter.

https://codereview.chromium.org/12212156/diff/7001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1726: state->Set("device_viewport_size",
MathUtil::asValue(deviceViewportSize()).release());
On 2013/02/13 06:51:36, danakj wrote:
> This should be the active tree too right?

If it drew a frame, then active tree and pending tree values are the same
anyway.  But OK, this one I'll change to explicit activeTree in case that ever
changes.

> 
> I feel like the deviceViewportSize() method returning the pending tree
sometimes
> is inconsistent with other helper methods on LTHImpl and is confusing things
> here. It's a nice trick but only gives the right answer in canDraw().
> 
> Can you change the *ViewportSize() methods to return the active tree values,
and
> have canDraw just do the right thing there?

I feel the activeTree value is bad and shouldn't be the default.  Things are
already resized throughout the browser process by the time CC hears about it. 
It's not under CC's control like scroll offset or whatnot.  Weird stuff might
happen if we act as though the old viewport size is still good.

[danakj, 2013-02-13 08:19:54]

This LGTM but I think enne should double check on it.

https://codereview.chromium.org/12212156/diff/4002/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12212156/diff/4002/cc/layer_tree_impl.cc#newc...
cc/layer_tree_impl.cc:275: if (device_viewport_size_ == device_viewport_size &&
sorry, but 2 space indent here :|

[enne (OOO), 2013-02-13 17:17:05]

I'm sorry to come back and be like "I liked the first way better", but...well, I
liked the first way better.

I find it hella complicated to read this patch and try to reason about which
tree's viewport I need to be setting and which one I need to be reading.  I can
see the reasoning that maybe viewport should be synced along with other tree
properties, but the page scale animation discussion above makes me think that
that it'd be way simpler to just think of having one viewport.

I think this patch would be simpler if you could just set a "viewport changed"
flag on LTI exactly like we do with contents purged.  You could check for that
canDraw, and also use that flag to set NEW_CONTENT_TAKES_PRIORITY in
ThreadProxy::renewTreePriority so we try to activate the pending tree as quickly
as possible.

[aelias_OOO_until_Jul13, 2013-02-13 22:12:54]

OK, I changed it like enne suggested.  I agree it's more explicit what's going
on this way.

[enne (OOO), 2013-02-13 22:41:52]

lgtm, thanks.

https://codereview.chromium.org/12212156/diff/16001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12212156/diff/16001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:331: if (syncTree->ViewportSizeInvalid())
Maybe just always do this?

https://codereview.chromium.org/12212156/diff/16001/cc/thread_proxy.cc
File cc/thread_proxy.cc (right):

https://codereview.chromium.org/12212156/diff/16001/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1179: // New content always takes priority when we the active
tree has
s/we //

https://codereview.chromium.org/12212156/diff/16001/cc/thread_proxy.cc#newcod...
cc/thread_proxy.cc:1180: // evicted resources or invalid viewport size.
s/or/or there is an invalid viewport size/

[commit-bot: I haz the power, 2013-02-14 00:47:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/12212156/21015

[commit-bot: I haz the power, 2013-02-14 00:54:30]

Change committed as 182357