gfx::Compositor: SchedulePaint.

gfx::Compositor: SchedulePaint.

Add CompositorDelegate as an interface between the compositor and its owner, and implement SchedulePaint through the delegate.

BUG=none
TEST=none

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

[sadrul, 2011-09-05 01:36:51]

This is an alternate CL for: http://codereview.chromium.org/7766002/. This is
necessary for aura.

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

LGTM

This breaks the Wayland port (but so did the other CL ;-)

http://codereview.chromium.org/7770002/diff/1/ui/gfx/compositor/compositor_gl.cc
File ui/gfx/compositor/compositor_gl.cc (right):

http://codereview.chromium.org/7770002/diff/1/ui/gfx/compositor/compositor_gl...
ui/gfx/compositor/compositor_gl.cc:16: #include
"third_party/skia/include/core/SkScalar.h"
probably need to #include <X11/Xlib.h> --- must be pulled in indirectly.

[sadrul, 2011-09-06 13:51:44]

I have added a check for USE_WAYLAND (I was going to use USE_X11, but I think
that is still defined for wayland).

http://codereview.chromium.org/7770002/diff/1/ui/gfx/compositor/compositor_gl.cc
File ui/gfx/compositor/compositor_gl.cc (right):

http://codereview.chromium.org/7770002/diff/1/ui/gfx/compositor/compositor_gl...
ui/gfx/compositor/compositor_gl.cc:16: #include
"third_party/skia/include/core/SkScalar.h"
On 2011/09/06 12:25:37, jonathan.backer wrote:
> probably need to #include <X11/Xlib.h> --- must be pulled in indirectly.

Done.

[sadrul, 2011-09-06 13:58:31]

On 2011/09/06 13:51:44, sadrul wrote:
> I have added a check for USE_WAYLAND (I was going to use USE_X11, but I think
> that is still defined for wayland).

I have been corrected. Using USE_X11 now.

[sky, 2011-09-06 16:08:23]

I spoke with Dan about doing something like this a while ago and he indicated
XSendEvent doesn't union these regions in anyway, which is bad. Dan also
mentioned having to round trip to the X server for this is also a bad idea.
Instead we should probably keep our own Runnable that is used to schedule an
event.

[sadrul, 2011-09-06 16:14:41]

On 2011/09/06 16:08:23, sky wrote:
> I spoke with Dan about doing something like this a while ago and he indicated
> XSendEvent doesn't union these regions in anyway, which is bad. Dan also
> mentioned having to round trip to the X server for this is also a bad idea.
> Instead we should probably keep our own Runnable that is used to schedule an
> event.

Indeed it doesn't. It does however send back multiple Expose events as
necessary. From the event-dispatcher, it is possible to know whether other
Expose events are already queued (by looking at XEvent.XExpose.count). So the
event-dispatcher can choose to union all the exposed regions, or we can do this
in here. In this case, it sounds like a good idea to do this here. I will make
this change.

[sky, 2011-09-06 16:17:25]

On Tue, Sep 6, 2011 at 9:14 AM,  <sadrul@chromium.org> wrote:
> On 2011/09/06 16:08:23, sky wrote:
>>
>> I spoke with Dan about doing something like this a while ago and he
>> indicated
>> XSendEvent doesn't union these regions in anyway, which is bad. Dan also
>> mentioned having to round trip to the X server for this is also a bad
>> idea.
>> Instead we should probably keep our own Runnable that is used to schedule
>> an
>> event.
>
> Indeed it doesn't. It does however send back multiple Expose events as
> necessary. From the event-dispatcher, it is possible to know whether other
> Expose events are already queued (by looking at XEvent.XExpose.count). So
> the
> event-dispatcher can choose to union all the exposed regions, or we can do
> this
> in here. In this case, it sounds like a good idea to do this here. I will
> make
> this change.

Can't we just schedule our own Task and not talk to X at all?

  -Scott

[sadrul, 2011-09-06 16:20:51]

On 2011/09/06 16:17:25, sky wrote:
> On Tue, Sep 6, 2011 at 9:14 AM,  <mailto:sadrul@chromium.org> wrote:
> > On 2011/09/06 16:08:23, sky wrote:
> >>
> >> I spoke with Dan about doing something like this a while ago and he
> >> indicated
> >> XSendEvent doesn't union these regions in anyway, which is bad. Dan also
> >> mentioned having to round trip to the X server for this is also a bad
> >> idea.
> >> Instead we should probably keep our own Runnable that is used to schedule
> >> an
> >> event.
> >
> > Indeed it doesn't. It does however send back multiple Expose events as
> > necessary. From the event-dispatcher, it is possible to know whether other
> > Expose events are already queued (by looking at XEvent.XExpose.count). So
> > the
> > event-dispatcher can choose to union all the exposed regions, or we can do
> > this
> > in here. In this case, it sounds like a good idea to do this here. I will
> > make
> > this change.
> 
> Can't we just schedule our own Task and not talk to X at all?

If we want to avoid talking to X at all, I think it'd be necessary to trigger a
draw in views/ or in aura/ from here, which will be very tricky.

[sky, 2011-09-06 16:56:33]

Good point.

  -Scott

[sadrul, 2011-09-06 17:26:52]

On 2011/09/06 16:56:33, sky wrote:
> Good point.
> 
>   -Scott

I have added an interface, CompositorOwner, to allow the Compositor to call back
to its owner. This will allow doing the painting without having to trigger an
expose event. What do you think?

[sadrul, 2011-09-06 17:27:05]

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor_gl.cc (right):

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor_gl.cc:498: owner()->PaintNow();
This will actually be a PostTask

[sadrul, 2011-09-06 17:27:42]

(this doesn't compile yet! Put up for review to get early feedback)

[sky, 2011-09-06 17:39:01]

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:67: class COMPOSITOR_EXPORT CompositorOwner {
Call this CompositorDelegate

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:70: virtual gfx::AcceleratedWidget
GetAcceleratedWidget() = 0;
I prefer passing this into the constructor rather than having to query the
delegate for it.

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:72: // Refreshes the textures. The Compositor
calls this after a SchedulePaint.
I don't like having yet another paint coalescing code path.
How about instead of delegating PaintNow, we delegate the schedule. That is,
SchedulePaint becomes:
void SchedulePaint() {
  delegate_->ScheduleCompositorPaint();
}
?

[sadrul, 2011-09-06 18:13:23]

I have added an implementation for windows as well and sent to the trybots. But
I haven't tested it.

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor.h
File ui/gfx/compositor/compositor.h (right):

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:67: class COMPOSITOR_EXPORT CompositorOwner {
On 2011/09/06 17:39:01, sky wrote:
> Call this CompositorDelegate

Done.

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:70: virtual gfx::AcceleratedWidget
GetAcceleratedWidget() = 0;
On 2011/09/06 17:39:01, sky wrote:
> I prefer passing this into the constructor rather than having to query the
> delegate for it.

Done.

http://codereview.chromium.org/7770002/diff/6028/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.h:72: // Refreshes the textures. The Compositor
calls this after a SchedulePaint.
On 2011/09/06 17:39:01, sky wrote:
> I don't like having yet another paint coalescing code path.
> How about instead of delegating PaintNow, we delegate the schedule. That is,
> SchedulePaint becomes:
> void SchedulePaint() {
>   delegate_->ScheduleCompositorPaint();
> }
> ?

Sounds like a good plan. Done.

[sadrul, 2011-09-06 18:36:58]

Added fix for aura. With this change, there needs to be a UI message-loop before
the compositor does painting (otherwise we get a crash in
Desktop::ScheduleCompositorPaint because MessageLoop::current is NULL). So I
made the change to have the Desktop create and own the UI message-loop on
creation. (CC'ing Ben for the aura change)

[sky, 2011-09-06 20:10:07]

http://codereview.chromium.org/7770002/diff/13001/aura/desktop.cc
File aura/desktop.cc (left):

http://codereview.chromium.org/7770002/diff/13001/aura/desktop.cc#oldcode41
aura/desktop.cc:41: MessageLoop main_message_loop(MessageLoop::TYPE_UI);
I actually just had problems with this too and was going to remove it. I think
Desktop shouldn't own the messageloop at all. Instead callers should own it.

[sky, 2011-09-06 20:12:36]

http://codereview.chromium.org/7770002/diff/13001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7770002/diff/13001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_win.cc:505: delegate()->ScheduleCompositorPaint();
Both implementations are the same. Promote it to Compositor.

[sadrul, 2011-09-06 21:50:11]

http://codereview.chromium.org/7770002/diff/13001/aura/desktop.cc
File aura/desktop.cc (left):

http://codereview.chromium.org/7770002/diff/13001/aura/desktop.cc#oldcode41
aura/desktop.cc:41: MessageLoop main_message_loop(MessageLoop::TYPE_UI);
On 2011/09/06 20:10:07, sky wrote:
> I actually just had problems with this too and was going to remove it. I think
> Desktop shouldn't own the messageloop at all. Instead callers should own it.

I have made the change so callers create and own the ML

http://codereview.chromium.org/7770002/diff/13001/ui/gfx/compositor/composito...
File ui/gfx/compositor/compositor_win.cc (right):

http://codereview.chromium.org/7770002/diff/13001/ui/gfx/compositor/composito...
ui/gfx/compositor/compositor_win.cc:505: delegate()->ScheduleCompositorPaint();
On 2011/09/06 20:12:36, sky wrote:
> Both implementations are the same. Promote it to Compositor.

Done. (I added the implementation in compositor.h since compositor.cc is not
compiled with compositor_gl/_win, and Compositor::WidgetSizeChanged is also
defined in compositor.h)

[sky, 2011-09-06 22:40:20]

LGTM