Plumb damage rect to browser compositor.

Plumb damage rect to browser compositor.

Relies on WK patch: https://bugs.webkit.org/show_bug.cgi?id=73485 for damage to external texture
and WK patch: https://bugs.webkit.org/show_bug.cgi?id=67341 for scissoring optimization.

BUG=none
TEST=run GYP_DEFINES="use_aura=1 use_webkit_compositor=1" on a machine that supports GLX_MESA_copy_sub_buffer; go to http://www.webkit.org/blog-files/3d-transforms/poster-circle.html; move a different X11 window over top of the chrome window; see that only part of the chrome window (with the animation) gets redrawn

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113801

[piman, 2011-11-30 22:28:44]

http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_widget_host_view_aura.cc:339:
window_->layer()->bounds().height() - params.y - params.height,
Shouldn't that depend on whether or not we need to flip y? I'm tempted to move
that logic inside of Layer, which has the information.

[piman, 2011-11-30 22:43:02]

On Wed, Nov 30, 2011 at 2:28 PM, <piman@chromium.org> wrote:

>
> http://codereview.chromium.**org/8764001/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_aura.cc<http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc>
> File content/browser/renderer_host/**render_widget_host_view_aura.**cc
> (right):
>
> http://codereview.chromium.**org/8764001/diff/1/content/**
>
browser/renderer_host/render_**widget_host_view_aura.cc#**newcode339<http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode339>
> content/browser/renderer_host/**render_widget_host_view_aura.**cc:339:
>
> window_->layer()->bounds().**height() - params.y - params.height,
> Shouldn't that depend on whether or not we need to flip y? I'm tempted
> to move that logic inside of Layer, which has the information.
>

Actually, probably all the way to the PluginLayerChromium, as mentioned in
my review of the WebKit side.

>
>
http://codereview.chromium.**org/8764001/<http://codereview.chromium.org/8764...
>

[jonathan.backer, 2011-12-05 20:43:17]

Tested on GLX and EGL (sort of --- pretended that I had the necessary extensions
in gl_surface_egl.cc and image_transport_surface_linux.cc to verify that I don't
need any flips).

On the WK end, I've preferred to keep the name setNeedsDisplayRect because
invalidateRect seems to imply that a paint as well as a draw is necessary (and
in this instance a paint is totally unnecessary).

[jonathan.backer, 2011-12-05 22:32:43]

+apatrick

[piman, 2011-12-06 00:46:34]

http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_widget_host_view_aura.cc:339:
window_->layer()->bounds().height() - params.y - params.height,
On 2011/11/30 22:28:44, piman wrote:
> Shouldn't that depend on whether or not we need to flip y? I'm tempted to move
> that logic inside of Layer, which has the information.

Ping?

[apatrick_chromium, 2011-12-06 01:10:30]

http://codereview.chromium.org/8764001/diff/10001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8764001/diff/10001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:351:
container->GetSize().height() - params.y - params.height,
It looks like a lot of this function is reaching into window_->layer() and that
it could perhaps move there. I don't really know what window_->layer() is though
:)

http://codereview.chromium.org/8764001/diff/10001/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8764001/diff/10001/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:334: else
nit: According to chrome style, this if-else wants braces.

[jonathan.backer, 2011-12-06 01:22:57]

On 2011/12/06 00:46:34, piman wrote:
>
http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/r...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
http://codereview.chromium.org/8764001/diff/1/content/browser/renderer_host/r...
> content/browser/renderer_host/render_widget_host_view_aura.cc:339:
> window_->layer()->bounds().height() - params.y - params.height,
> On 2011/11/30 22:28:44, piman wrote:
> > Shouldn't that depend on whether or not we need to flip y? I'm tempted to
move
> > that logic inside of Layer, which has the information.
> 
> Ping?

This represents the damage in Layer space (origin is top left instead of bottom
left). No flip is necessary. I should have been more explicit --- this is what I
meant when I said that I verified this by testing with the EGL backend.

[jonathan.backer, 2011-12-06 01:25:12]

http://codereview.chromium.org/8764001/diff/10001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8764001/diff/10001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:351:
container->GetSize().height() - params.y - params.height,
On 2011/12/06 01:10:30, apatrick_chromium wrote:
> It looks like a lot of this function is reaching into window_->layer() and
that
> it could perhaps move there. I don't really know what window_->layer() is
though
> :)

I'm asking for the layer for a paint in it's co-ordinate system. It may be
possible to combine the two calls (SetExternalTexture and SchedulePaint), but
they have independent uses.

[apatrick_chromium, 2011-12-06 01:32:07]

Well apart for my nit about braces, LGTM for my part.

[jonathan.backer, 2011-12-06 17:42:33]

http://codereview.chromium.org/8764001/diff/10001/ui/gfx/compositor/layer.cc
File ui/gfx/compositor/layer.cc (right):

http://codereview.chromium.org/8764001/diff/10001/ui/gfx/compositor/layer.cc#...
ui/gfx/compositor/layer.cc:334: else
On 2011/12/06 01:10:30, apatrick_chromium wrote:
> nit: According to chrome style, this if-else wants braces.

Done.

[jonathan.backer, 2011-12-08 15:57:39]

+sky for RWHVA OWNERS

[sky, 2011-12-08 17:39:05]

http://codereview.chromium.org/8764001/diff/21001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8764001/diff/21001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:378: // Co-ords
come in GL co-ord space. Need to convert to layer space.
I've never seen coordinates abbreviated like that.

http://codereview.chromium.org/8764001/diff/21001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:379:
window_->layer()->SchedulePaint(gfx::Rect(
nit: window_->SchedulePaint should work equally well here.

http://codereview.chromium.org/8764001/diff/21001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_gl.h (right):

http://codereview.chromium.org/8764001/diff/21001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_gl.h:80: const gfx::Size& size() const { return
size_; }
Is this needed?

[jonathan.backer, 2011-12-08 18:41:44]

http://codereview.chromium.org/8764001/diff/21001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8764001/diff/21001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:378: // Co-ords
come in GL co-ord space. Need to convert to layer space.
On 2011/12/08 17:39:05, sky wrote:
> I've never seen coordinates abbreviated like that.

Done.

http://codereview.chromium.org/8764001/diff/21001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:379:
window_->layer()->SchedulePaint(gfx::Rect(
On 2011/12/08 17:39:05, sky wrote:
> nit: window_->SchedulePaint should work equally well here.

Done.

http://codereview.chromium.org/8764001/diff/21001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_gl.h (right):

http://codereview.chromium.org/8764001/diff/21001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_gl.h:80: const gfx::Size& size() const { return
size_; }
On 2011/12/08 17:39:05, sky wrote:
> Is this needed?

Is now. I've got rid of the conditional compile in RWHVA on
USE_WEBKIT_COMPOSITOR. So I've got calls from RHWVA to
Window::SchedulePaintInRect on GL and CC. I'd like to minimize divergence.

[piman, 2011-12-08 18:59:23]

lgtm

[sky, 2011-12-08 20:06:49]

LGTM

[commit-bot: I haz the power, 2011-12-08 20:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/8764001/25001

[commit-bot: I haz the power, 2011-12-08 23:47:01]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 113650, job name
was 8764001-25001 (previous was lost) (previous was lost) (previous was lost)
(retry).