Sky: update the HTMLIFrameElement's geometry during paint invalidation rather

Sky: update the HTMLIFrameElement's geometry during paint invalidation rather
than layout.

This ends up being much smoother and only triggers once per layout cycle.

R=esprehn@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/9f080001ab6aedea3a8a534ed72f560059d9f604

[Matt Perry, 2014-11-14 19:22:02]

mpcomplete@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2014-11-14 20:06:06]

This seems reasonable to me, but we should have esprehn look.

[Matt Perry, 2014-11-14 20:12:51]

mpcomplete@chromium.org changed reviewers:
+ esprehn@chromium.org

[Matt Perry, 2014-11-14 20:12:51]

adding esprehn as reviewer

[esprehn, 2014-11-14 21:46:08]

We're going to need a more complex system than just this, the view manager
probably needs to coordinate painting and placement of views so they don't seem
to lag when moving around. I think the goal is to eventually put the
mojo::View's OutputSurface in our own surface tree so that it's always in the
right spot?

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
File sky/engine/core/rendering/RenderIFrame.cpp (right):

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
sky/engine/core/rendering/RenderIFrame.cpp:34: return false;
You only return false if there's no content view, that doesn't seem like a
needed optimization. Just return void and always let the normal paint
invalidation run.

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
sky/engine/core/rendering/RenderIFrame.cpp:42: if
(mojoBounds.Equals(contentView->bounds()))
This should be inside View::SetBounds(const Rect& bounds), it should be a noop
inside mojo to set the same bounds repeatedly.

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
File sky/engine/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
sky/engine/core/rendering/RenderView.cpp:307: for (size_t i = 0; i <
iframes.size(); ++i)
You don't need to copy to a Vector, there's no synchronous script in there, just
use the iterator on the HashSet.

This also doesn't seem right, if we fully invalidate the entire viewport we
might not even get here. You want this to be a separate and explicit step inside
FrameView::updateLayoutAndStyleForPainting() like updateIframeBoundsIfNeeded();

[Matt Perry, 2014-11-14 22:03:34]

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
File sky/engine/core/rendering/RenderIFrame.cpp (right):

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
sky/engine/core/rendering/RenderIFrame.cpp:34: return false;
On 2014/11/14 21:46:08, esprehn wrote:
> You only return false if there's no content view, that doesn't seem like a
> needed optimization. Just return void and always let the normal paint
> invalidation run.

Done.

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
File sky/engine/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/730653002/diff/20001/sky/engine/core/renderin...
sky/engine/core/rendering/RenderView.cpp:307: for (size_t i = 0; i <
iframes.size(); ++i)
On 2014/11/14 21:46:08, esprehn wrote:
> You don't need to copy to a Vector, there's no synchronous script in there,
just
> use the iterator on the HashSet.

Done.

> This also doesn't seem right, if we fully invalidate the entire viewport we
> might not even get here. You want this to be a separate and explicit step
inside
> FrameView::updateLayoutAndStyleForPainting() like
updateIframeBoundsIfNeeded();

I don't follow you here. This is only for the case where we're invalidating the
whole viewport - e.g., doingFullPaintInvalidation(). In the case where that is
false, we instead crawl down the render tree and invalidate each individual
iframe if it's part of a subtree that was invalidated.
(RenderObject::invalidatePaintIfNeeded)

[Matt Perry, 2014-11-14 22:05:17]

On 2014/11/14 21:46:08, esprehn wrote:
> We're going to need a more complex system than just this, the view manager
> probably needs to coordinate painting and placement of views so they don't
seem
> to lag when moving around. I think the goal is to eventually put the
> mojo::View's OutputSurface in our own surface tree so that it's always in the
> right spot?

Yeah, I'm just taking baby steps for now. I'm not sure what will be needed
later. I need to familiarize myself with the view manager and cc more.

[Matt Perry, 2014-11-17 18:58:24]

Do you think this is good enough to check in for now, or is it going in the
wrong direction?

[esprehn, 2014-11-17 19:04:32]

lgtm seems okay to start, but you're introducing bugs in the current paint
system.

https://codereview.chromium.org/730653002/diff/40001/sky/engine/core/renderin...
File sky/engine/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/730653002/diff/40001/sky/engine/core/renderin...
sky/engine/core/rendering/RenderView.cpp:306: iframe->invalidateWidgetBounds();
We don't always repaint if you move, for example if you give the iframe
translateZ(0) today then we'll not repaint if you transform it which means we'll
never get here.

I think this needs to be an explicit step the
FrameView::updateStyleAndLayoutForPainting() process.

[Matt Perry, 2014-11-17 19:20:21]

Committed patchset #3 (id:40001) manually as
9f080001ab6aedea3a8a534ed72f560059d9f604.