Stop painting when receiving delegated frame

Stop painting when receiving delegated frame

The SchedulePaintInRect call in DelegatedFrameHost is only there to
eventually notify an observer on the aura::Window that damage has
occurred (for the video detector).  Instead, just pass along the
damage to the LayerDelegate API and don't schedule a paint.

R=jbauman@chromium.org
BUG=none

Committed: https://crrev.com/9a3ddb332a5ec5e21a5767b2891ba161cda5a3d0
Cr-Commit-Position: refs/heads/master@{#291582}

[enne (OOO), 2014-08-11 23:43:23]

jbauman, what do you think of this cleanup?

https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc#newcode500
ui/aura/window.cc:500: void Window::NotifyDamage(const gfx::Rect& rect) {
This code is an unfortunate duplicate of Window::SchedulePaintInRect, but I'm
not sure how to encapsulate this logic any better.

[piman, 2014-08-12 20:44:11]

+sky (for aura) and derat (for video detector)
Context is that the only reason we send explicit damage to the aura::Window from
RenderWidgetHostViewAura is for the video detector. Layer damage from the frame
is already handled properly by the DelegatedRenderLayer (or SurfaceLayer) at the
cc level. So the damage we send from SchedulePaintInRect is redundant.

[jbauman, 2014-08-12 21:11:54]

lgtm, thanks for doing this.

[enne (OOO), 2014-08-13 17:54:05]

+sky for content/browser/renderer_host/ and ui/aura/
+derat FYI

[Daniel Erat, 2014-08-13 18:41:54]

thanks for the heads-up (and for preserving the old path)

[sky, 2014-08-13 20:01:01]

https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc#newcode500
ui/aura/window.cc:500: void Window::NotifyDamage(const gfx::Rect& rect) {
Is this a notification that a paint has been scheduled, or that a layer painted?
My initial reaction is that we shouldn't have this and instead the video
detector should be listening to the compositor. But it's entirely likely I'm
missing something.

[enne (OOO), 2014-08-13 23:55:45]

https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc#newcode500
ui/aura/window.cc:500: void Window::NotifyDamage(const gfx::Rect& rect) {
On 2014/08/13 20:01:01, sky wrote:
> Is this a notification that a paint has been scheduled, or that a layer
painted?
> My initial reaction is that we shouldn't have this and instead the video
> detector should be listening to the compositor. But it's entirely likely I'm
> missing something.

This is a notification that part of the screen has changed (or will, once the
delegated frame has been drawn).  It's more in the sense of "a layer painted"
than "a paint has been scheduled", but I'm not sure I'd use the word "paint" to
describe it.

I'm not sure what you mean by the detector listening to the compositor, as that
seems to me to be what it's doing.  The NotifyDamage call in DelegatedFrameHost
is essentially plumbing through the damage from the delegated frame to the
window to the detector.  How would you suggest doing this more simply?

[sky, 2014-08-14 00:17:44]

On Wed, Aug 13, 2014 at 4:55 PM, <enne@chromium.org> wrote:
>
>
> https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc
> File ui/aura/window.cc (right):
>
> https://codereview.chromium.org/464643003/diff/1/ui/aura/window.cc#newcode500
> ui/aura/window.cc:500: void Window::NotifyDamage(const gfx::Rect& rect)
> {
> On 2014/08/13 20:01:01, sky wrote:
>>
>> Is this a notification that a paint has been scheduled, or that a
>
> layer painted?
>>
>> My initial reaction is that we shouldn't have this and instead the
>
> video
>>
>> detector should be listening to the compositor. But it's entirely
>
> likely I'm
>>
>> missing something.
>
>
> This is a notification that part of the screen has changed (or will,
> once the delegated frame has been drawn).  It's more in the sense of "a
> layer painted" than "a paint has been scheduled", but I'm not sure I'd
> use the word "paint" to describe it.

I think you're trying to reuse OnWindowPaintScheduled as that is what
the video detector is currently listing to, even though that isn't
what is happening (no schedulepaint has been invoked).

Since the Layer/Compositor draw to the screen I would expect the
notification to come from the Layer/Compositor and not have
RenderWidgetHostViewAura call into a weird API. Seems like the
compositor and/or layer should know about this and notify the
LayerDelegate (or a new CompositorObserver method). If it goes through
the LayerDelegate then Window could receive this and invoke some new
function. Alternatively it may make more sense for this to be a
function on CompositorObserver. Not sure there.

>
>
> I'm not sure what you mean by the detector listening to the compositor,
> as that seems to me to be what it's doing.  The NotifyDamage call in
> DelegatedFrameHost is essentially plumbing through the damage from the
> delegated frame to the window to the detector.  How would you suggest
> doing this more simply?
>
> https://codereview.chromium.org/464643003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-14 21:47:29]

I was trying to fix a small inefficiency, but you're asking me to fix the way it
was originally implemented.  I can do that, but it'll take me a bit to sort out
how to get from A to B.

[sky, 2014-08-14 22:47:34]

Not sure there. What I am asking is for you to fix it the right way:)

On Thu, Aug 14, 2014 at 2:47 PM,  <enne@chromium.org> wrote:
> I was trying to fix a small inefficiency, but you're asking me to fix the
> way it
> was originally implemented.  I can do that, but it'll take me a bit to sort
> out
> how to get from A to B.
>
> https://codereview.chromium.org/464643003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-14 23:03:42]

I didn't think RenderWidgetHostViewAura was a bad place to route the delegated
frame damage through since it's the one that owns the delegated frame host.

You suggest passing this through the CompositorObserver interface, but the
DelegatedFrameHost *is* the CompositorObserver.  This patch goes
DelegatedFrameHost -> RenderWidgetHostViewAura -> Window.  You're suggesting
DelegatedFrameHost -> Compositor -> CompositorObserver/DelegatedFrameHost ->
???.  I don't understand how going through the CompositorObserver solves
anything.

The only way I can see doing this through layers would be for
cc::DelegatedRendererLayer to add a client interface to send damage when it gets
some new frame.  Then ui::Layer could implement some
"OnDelegatedFrameSwap(gfx::Rect damage)" call.  Then, that could call on Window
via the LayerDelegate interface?  It can't really call
LayerDelegate::OnPaintLayer because that actually goes and paints.  Maybe
LayerDelegate could grow some "OnExternalDamage" function which could then call
OnWindowPaintScheduled inside of Window::OnExternalDamage.  Although, it seems a
bit awkward to only do this for delegated layers and not other layer types with
external content (in the ui::Layer::has_external_content sense).  So, other
cc::Layer types might need similar client interfaces to match?

Does this sound like what you're looking for? It certainly seems more
convoluted.

[enne (OOO), 2014-08-14 23:12:57]

I guess also what I didn't realize is that it seems like only the layer owned by
the window is the LayerDelegate.  So, damage to some random delegated layer in
that layer tree would have to get handed up to the right layer for the window to
hear about it via LayerDelegate?

[sky, 2014-08-15 04:20:55]

On Thu, Aug 14, 2014 at 4:12 PM,  <enne@chromium.org> wrote:
> I guess also what I didn't realize is that it seems like only the layer
> owned by
> the window is the LayerDelegate.  So, damage to some random delegated layer
> in
> that layer tree would have to get handed up to the right layer for the
> window to
> hear about it via LayerDelegate?

Isn't this roughly the same as what is happening now. RWHVA tell's its
window to schedulepaint which notifies the WindowObservers. New
approach is LayerDelegate gets a new method OnDelegatedFrameSwap,
window overrides that and forwards to its WindowObservers. You
outlined this in your last email. I suggested an approach like this,
or an observer on the compositor. I wasn't sure if the compositor
observer made sense as I don't know enough about the underlying
compositor implementation.

  -Scott

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-15 18:04:34]

I think LayerDelegate is the wrong place to put this.

The frame swap is happening on some child in the tree, but to get to the window,
the DelegateFrameHost has to hand the damage to the layer associated with the
window, not the layer associated with the frame swap.  The only reason this path
works is because the Window/DelegatedFrameHostClient can hand back its layer as
a way to get to the window itself.  This is not a general "Layer" event and so
it seems awkward to put it on the LayerDelegate API.

At any rate, I think this is wrong, but here's a patch that does what you ask. 
Please let me know if you prefer this to the previous patchset.

[sky, 2014-08-15 19:21:45]

On Fri, Aug 15, 2014 at 11:04 AM,  <enne@chromium.org> wrote:
> I think LayerDelegate is the wrong place to put this.
>
> The frame swap is happening on some child in the tree, but to get to the
> window,
> the DelegateFrameHost has to hand the damage to the layer associated with
> the
> window, not the layer associated with the frame swap.  The only reason this
> path
> works is because the Window/DelegatedFrameHostClient can hand back its layer
> as
> a way to get to the window itself.  This is not a general "Layer" event and
> so
> it seems awkward to put it on the LayerDelegate API.

Notifying observers that a paint was schedule when one was not in fact
scheduled is equally wrong.

There must be some way to make both of us happy.

I still view this as a layer is getting damaged and wants to notify
observers. Is that not a valid way to view what is happening? If so,
how to correctly wire up observers/delegates to make you happy?

  -Scott


>
> At any rate, I think this is wrong, but here's a patch that does what you
> ask.
> Please let me know if you prefer this to the previous patchset.
>
> https://codereview.chromium.org/464643003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-15 20:18:51]

On 2014/08/15 at 19:21:45, sky wrote:

> Notifying observers that a paint was schedule when one was not in fact
> scheduled is equally wrong.

Are you referring to the call to WindowObserver::OnWindowPaintScheduled in
either of my patches as being wrong? Or the previous behavior of calling
DelegatedFrameHost::SchedulePaintInRect prior to either of my patches?

> I still view this as a layer is getting damaged and wants to notify
> observers. Is that not a valid way to view what is happening? If so,
> how to correctly wire up observers/delegates to make you happy?

This *is* what is happening, but you're oversimplifying.

LayerDelegate is for things that happen to *that* layer.  See:
LayerDelegate::OnPaintLayer or LayerDelegate::PrepareForLayerBoundsChange.  That
means that *that* specific layer has something happening to it.  The layer
attached to the window is *not* the same layer that has the delegated frame swap
damage.

This leaves you with two equally poor options.  One is to call some new
LayerDelegate::OnDelegatedFrameDamage on the layer with the delegated frame swap
damage and then call this on every layer up the tree (or every layer in the
tree?) so that you make sure it gets routed to the one attached to the window. 
The other (seen in patch #2) is to call some new
LayerDelegate::OnDelegatedFrameDamage on the DelegatedFrameHostClient's layer
because that's the one attached to the window even though that's not the same
layer that had the damage.  This to me says that LayerDelegate is not actually
the right abstraction.


Can you help me understand why you don't like going through
RenderWidgetHostViewAura?

[enne (OOO), 2014-08-15 22:57:55]

I talked with sky over chat, and it seems like maybe the VideoDetector is in the
wrong place and should be a CompositorObserver and not a WindowObserver.

That said, the VideoDetector also seems to need to know things about the window
(e.g. visibility[0], full screen[1]) that the compositor doesn't know about but
the window does.  Given that restriction, I don't see how to move VideoDetector
to be a CompositorObserver instead.

I think my first patch is strictly better than the status quo and would still
like to land it.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...

[sky, 2014-08-18 15:41:29]

The existing code notifies when SchedulePaint is called. This makes
sense as SchedulePaint was and is useful without the notification.
Patchset 1 adds a new function to window whose only job is to notify
observers. It changes no window state. This feels worse to me.

How does DelegatedFrameHost end up getting bits onto the screen? I
assume the compositor is used at some point, but how? What is called
on the compositor side? Does DelegatedFrameHost go directly to cc
land, or does it talk at all to ui/compositor?

  -Scott

On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
> I talked with sky over chat, and it seems like maybe the VideoDetector is in
> the
> wrong place and should be a CompositorObserver and not a WindowObserver.
>
> That said, the VideoDetector also seems to need to know things about the
> window
> (e.g. visibility[0], full screen[1]) that the compositor doesn't know about
> but
> the window does.  Given that restriction, I don't see how to move
> VideoDetector
> to be a CompositorObserver instead.
>
> I think my first patch is strictly better than the status quo and would
> still
> like to land it.
>
> [0]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>
> https://codereview.chromium.org/464643003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-08-18 15:45:21]

On Mon, Aug 18, 2014 at 11:41 AM, Scott Violet <sky@chromium.org> wrote:

> The existing code notifies when SchedulePaint is called. This makes
> sense as SchedulePaint was and is useful without the notification.
> Patchset 1 adds a new function to window whose only job is to notify
> observers. It changes no window state. This feels worse to me.
>
> How does DelegatedFrameHost end up getting bits onto the screen? I
> assume the compositor is used at some point, but how? What is called
> on the compositor side? Does DelegatedFrameHost go directly to cc
> land, or does it talk at all to ui/compositor?
>

DelegatedFrameHost inserts a new frame on a cc::DelegatedFrameProvider
which causes the cc::DelegatedRendererLayer to be notified of new content
and commit/draw the content. ui::Compositor and ui::Layer is not involved
at all unless the size of the frame changes, in which case the
cc::DelegatedFrameProvider is destroyed and passed through the ui::Layer.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...


>
>   -Scott
>
> On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
> > I talked with sky over chat, and it seems like maybe the VideoDetector
> is in
> > the
> > wrong place and should be a CompositorObserver and not a WindowObserver.
> >
> > That said, the VideoDetector also seems to need to know things about the
> > window
> > (e.g. visibility[0], full screen[1]) that the compositor doesn't know
> about
> > but
> > the window does.  Given that restriction, I don't see how to move
> > VideoDetector
> > to be a CompositorObserver instead.
> >
> > I think my first patch is strictly better than the status quo and would
> > still
> > like to land it.
> >
> > [0]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> >
> > https://codereview.chromium.org/464643003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-18 18:16:10]

Would it make sense to have a callback to the ui::Compositor and/or
ui::Layer some where in here?

On Mon, Aug 18, 2014 at 8:44 AM, Dana Jansens <danakj@google.com> wrote:
> On Mon, Aug 18, 2014 at 11:41 AM, Scott Violet <sky@chromium.org> wrote:
>>
>> The existing code notifies when SchedulePaint is called. This makes
>> sense as SchedulePaint was and is useful without the notification.
>> Patchset 1 adds a new function to window whose only job is to notify
>> observers. It changes no window state. This feels worse to me.
>>
>> How does DelegatedFrameHost end up getting bits onto the screen? I
>> assume the compositor is used at some point, but how? What is called
>> on the compositor side? Does DelegatedFrameHost go directly to cc
>> land, or does it talk at all to ui/compositor?
>
>
> DelegatedFrameHost inserts a new frame on a cc::DelegatedFrameProvider which
> causes the cc::DelegatedRendererLayer to be notified of new content and
> commit/draw the content. ui::Compositor and ui::Layer is not involved at all
> unless the size of the frame changes, in which case the
> cc::DelegatedFrameProvider is destroyed and passed through the ui::Layer.
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...
>
>>
>>
>>   -Scott
>>
>> On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
>> > I talked with sky over chat, and it seems like maybe the VideoDetector
>> > is in
>> > the
>> > wrong place and should be a CompositorObserver and not a WindowObserver.
>> >
>> > That said, the VideoDetector also seems to need to know things about the
>> > window
>> > (e.g. visibility[0], full screen[1]) that the compositor doesn't know
>> > about
>> > but
>> > the window does.  Given that restriction, I don't see how to move
>> > VideoDetector
>> > to be a CompositorObserver instead.
>> >
>> > I think my first patch is strictly better than the status quo and would
>> > still
>> > like to land it.
>> >
>> > [0]
>> >
>> >
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>> > [1]
>> >
>> >
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>> >
>> > https://codereview.chromium.org/464643003/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-08-18 18:24:26]

On Mon, Aug 18, 2014 at 2:16 PM, Scott Violet <sky@chromium.org> wrote:

> Would it make sense to have a callback to the ui::Compositor and/or
> ui::Layer some where in here?
>

It would be a notification of damage the same way that RWHVAura right now
knows about the damage and tries to notify the layer. The difference is
that some cc:: class would call it via virtual interface instead of
RWHVAura calling it, but that doesn't seem to affect the outcome of this CL
or its discussion, or am I missing something that a callback would buy for
us?


>
> On Mon, Aug 18, 2014 at 8:44 AM, Dana Jansens <danakj@google.com> wrote:
> > On Mon, Aug 18, 2014 at 11:41 AM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> The existing code notifies when SchedulePaint is called. This makes
> >> sense as SchedulePaint was and is useful without the notification.
> >> Patchset 1 adds a new function to window whose only job is to notify
> >> observers. It changes no window state. This feels worse to me.
> >>
> >> How does DelegatedFrameHost end up getting bits onto the screen? I
> >> assume the compositor is used at some point, but how? What is called
> >> on the compositor side? Does DelegatedFrameHost go directly to cc
> >> land, or does it talk at all to ui/compositor?
> >
> >
> > DelegatedFrameHost inserts a new frame on a cc::DelegatedFrameProvider
> which
> > causes the cc::DelegatedRendererLayer to be notified of new content and
> > commit/draw the content. ui::Compositor and ui::Layer is not involved at
> all
> > unless the size of the frame changes, in which case the
> > cc::DelegatedFrameProvider is destroyed and passed through the ui::Layer.
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...
> >
> >>
> >>
> >>   -Scott
> >>
> >> On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
> >> > I talked with sky over chat, and it seems like maybe the VideoDetector
> >> > is in
> >> > the
> >> > wrong place and should be a CompositorObserver and not a
> WindowObserver.
> >> >
> >> > That said, the VideoDetector also seems to need to know things about
> the
> >> > window
> >> > (e.g. visibility[0], full screen[1]) that the compositor doesn't know
> >> > about
> >> > but
> >> > the window does.  Given that restriction, I don't see how to move
> >> > VideoDetector
> >> > to be a CompositorObserver instead.
> >> >
> >> > I think my first patch is strictly better than the status quo and
> would
> >> > still
> >> > like to land it.
> >> >
> >> > [0]
> >> >
> >> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> >> > [1]
> >> >
> >> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> >> >
> >> > https://codereview.chromium.org/464643003/
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-18 19:25:04]

IMO it's a lot cleaner if a particular system knows when it has
changed so that it can notifiy observers of said system. It's feels
like an add on when someone else has to tell the system it changed
purely so that observers can be notified. It's error prone and results
in warts on the API. For example, Window::SetPropertyInternal is a
useful function for Window, and since it's useful for observers to
know about property changes Window::SetPropertyInternal notifies
observers.

Compare that to Window::NotifyDamage. It has no use to Window. It is
only there so that other people can invoke, hopefully at the right
time. Further NotifyDamage must be public as Window doesn't know when
to invoke it. What would be a lot cleaner is if the compositor told
Window it was damaged and window then told its observers. Although I
am not stuck on having notification happen through window.
VideoDetector could equally observer the compositor and the compositor
could notify observers at appropriate. Again, to me this makes sense
as presumably the compositor knows what is going on and if didn't it
still makes more sense to have it know what's going on. Either
approach is preferable in so far as we don't end up with a function on
Window that is public and assumed to be invoked at the right time.

Have I made my point any clearer this time around? Am I still speaking
gibberish?

  -Scott

On Mon, Aug 18, 2014 at 11:24 AM, Dana Jansens <danakj@google.com> wrote:
> On Mon, Aug 18, 2014 at 2:16 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Would it make sense to have a callback to the ui::Compositor and/or
>> ui::Layer some where in here?
>
>
> It would be a notification of damage the same way that RWHVAura right now
> knows about the damage and tries to notify the layer. The difference is that
> some cc:: class would call it via virtual interface instead of RWHVAura
> calling it, but that doesn't seem to affect the outcome of this CL or its
> discussion, or am I missing something that a callback would buy for us?
>
>>
>>
>> On Mon, Aug 18, 2014 at 8:44 AM, Dana Jansens <danakj@google.com> wrote:
>> > On Mon, Aug 18, 2014 at 11:41 AM, Scott Violet <sky@chromium.org> wrote:
>> >>
>> >> The existing code notifies when SchedulePaint is called. This makes
>> >> sense as SchedulePaint was and is useful without the notification.
>> >> Patchset 1 adds a new function to window whose only job is to notify
>> >> observers. It changes no window state. This feels worse to me.
>> >>
>> >> How does DelegatedFrameHost end up getting bits onto the screen? I
>> >> assume the compositor is used at some point, but how? What is called
>> >> on the compositor side? Does DelegatedFrameHost go directly to cc
>> >> land, or does it talk at all to ui/compositor?
>> >
>> >
>> > DelegatedFrameHost inserts a new frame on a cc::DelegatedFrameProvider
>> > which
>> > causes the cc::DelegatedRendererLayer to be notified of new content and
>> > commit/draw the content. ui::Compositor and ui::Layer is not involved at
>> > all
>> > unless the size of the frame changes, in which case the
>> > cc::DelegatedFrameProvider is destroyed and passed through the
>> > ui::Layer.
>> >
>> >
>> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...
>> >
>> >>
>> >>
>> >>   -Scott
>> >>
>> >> On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
>> >> > I talked with sky over chat, and it seems like maybe the
>> >> > VideoDetector
>> >> > is in
>> >> > the
>> >> > wrong place and should be a CompositorObserver and not a
>> >> > WindowObserver.
>> >> >
>> >> > That said, the VideoDetector also seems to need to know things about
>> >> > the
>> >> > window
>> >> > (e.g. visibility[0], full screen[1]) that the compositor doesn't know
>> >> > about
>> >> > but
>> >> > the window does.  Given that restriction, I don't see how to move
>> >> > VideoDetector
>> >> > to be a CompositorObserver instead.
>> >> >
>> >> > I think my first patch is strictly better than the status quo and
>> >> > would
>> >> > still
>> >> > like to land it.
>> >> >
>> >> > [0]
>> >> >
>> >> >
>> >> >
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>> >> > [1]
>> >> >
>> >> >
>> >> >
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>> >> >
>> >> > https://codereview.chromium.org/464643003/
>> >
>> >
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-08-18 19:29:04]

On Mon, Aug 18, 2014 at 3:25 PM, Scott Violet <sky@chromium.org> wrote:

> IMO it's a lot cleaner if a particular system knows when it has
> changed so that it can notifiy observers of said system. It's feels
> like an add on when someone else has to tell the system it changed
> purely so that observers can be notified. It's error prone and results
> in warts on the API. For example, Window::SetPropertyInternal is a
> useful function for Window, and since it's useful for observers to
> know about property changes Window::SetPropertyInternal notifies
> observers.
>
> Compare that to Window::NotifyDamage. It has no use to Window. It is
> only there so that other people can invoke, hopefully at the right
> time. Further NotifyDamage must be public as Window doesn't know when
> to invoke it. What would be a lot cleaner is if the compositor told
> Window it was damaged and window then told its observers. Although I
> am not stuck on having notification happen through window.
> VideoDetector could equally observer the compositor and the compositor
> could notify observers at appropriate. Again, to me this makes sense
> as presumably the compositor knows what is going on and if didn't it
> still makes more sense to have it know what's going on. Either
> approach is preferable in so far as we don't end up with a function on
> Window that is public and assumed to be invoked at the right time.
>

What I don't get is that whether ui::Layer, ui::Compositor, or RWHVAura
calls Window::NotifyDamage, you still need a Window::NotifyDamage, don't
you?

> What would be a lot cleaner is if the compositor told
> Window it was damaged and window then told its observers.

Why does having ui::Layer call the function change how the Window interface
looks?


>
> Have I made my point any clearer this time around? Am I still speaking
> gibberish?
>
>   -Scott
>
> On Mon, Aug 18, 2014 at 11:24 AM, Dana Jansens <danakj@google.com> wrote:
> > On Mon, Aug 18, 2014 at 2:16 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> Would it make sense to have a callback to the ui::Compositor and/or
> >> ui::Layer some where in here?
> >
> >
> > It would be a notification of damage the same way that RWHVAura right now
> > knows about the damage and tries to notify the layer. The difference is
> that
> > some cc:: class would call it via virtual interface instead of RWHVAura
> > calling it, but that doesn't seem to affect the outcome of this CL or its
> > discussion, or am I missing something that a callback would buy for us?
> >
> >>
> >>
> >> On Mon, Aug 18, 2014 at 8:44 AM, Dana Jansens <danakj@google.com>
> wrote:
> >> > On Mon, Aug 18, 2014 at 11:41 AM, Scott Violet <sky@chromium.org>
> wrote:
> >> >>
> >> >> The existing code notifies when SchedulePaint is called. This makes
> >> >> sense as SchedulePaint was and is useful without the notification.
> >> >> Patchset 1 adds a new function to window whose only job is to notify
> >> >> observers. It changes no window state. This feels worse to me.
> >> >>
> >> >> How does DelegatedFrameHost end up getting bits onto the screen? I
> >> >> assume the compositor is used at some point, but how? What is called
> >> >> on the compositor side? Does DelegatedFrameHost go directly to cc
> >> >> land, or does it talk at all to ui/compositor?
> >> >
> >> >
> >> > DelegatedFrameHost inserts a new frame on a cc::DelegatedFrameProvider
> >> > which
> >> > causes the cc::DelegatedRendererLayer to be notified of new content
> and
> >> > commit/draw the content. ui::Compositor and ui::Layer is not involved
> at
> >> > all
> >> > unless the size of the frame changes, in which case the
> >> > cc::DelegatedFrameProvider is destroyed and passed through the
> >> > ui::Layer.
> >> >
> >> >
> >> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...
> >> >
> >> >>
> >> >>
> >> >>   -Scott
> >> >>
> >> >> On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
> >> >> > I talked with sky over chat, and it seems like maybe the
> >> >> > VideoDetector
> >> >> > is in
> >> >> > the
> >> >> > wrong place and should be a CompositorObserver and not a
> >> >> > WindowObserver.
> >> >> >
> >> >> > That said, the VideoDetector also seems to need to know things
> about
> >> >> > the
> >> >> > window
> >> >> > (e.g. visibility[0], full screen[1]) that the compositor doesn't
> know
> >> >> > about
> >> >> > but
> >> >> > the window does.  Given that restriction, I don't see how to move
> >> >> > VideoDetector
> >> >> > to be a CompositorObserver instead.
> >> >> >
> >> >> > I think my first patch is strictly better than the status quo and
> >> >> > would
> >> >> > still
> >> >> > like to land it.
> >> >> >
> >> >> > [0]
> >> >> >
> >> >> >
> >> >> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> >> >> > [1]
> >> >> >
> >> >> >
> >> >> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
> >> >> >
> >> >> > https://codereview.chromium.org/464643003/
> >> >
> >> >
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-18 19:40:49]

On Mon, Aug 18, 2014 at 12:28 PM, Dana Jansens <danakj@google.com> wrote:
> On Mon, Aug 18, 2014 at 3:25 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> IMO it's a lot cleaner if a particular system knows when it has
>> changed so that it can notifiy observers of said system. It's feels
>> like an add on when someone else has to tell the system it changed
>> purely so that observers can be notified. It's error prone and results
>> in warts on the API. For example, Window::SetPropertyInternal is a
>> useful function for Window, and since it's useful for observers to
>> know about property changes Window::SetPropertyInternal notifies
>> observers.
>>
>> Compare that to Window::NotifyDamage. It has no use to Window. It is
>> only there so that other people can invoke, hopefully at the right
>> time. Further NotifyDamage must be public as Window doesn't know when
>> to invoke it. What would be a lot cleaner is if the compositor told
>> Window it was damaged and window then told its observers. Although I
>> am not stuck on having notification happen through window.
>> VideoDetector could equally observer the compositor and the compositor
>> could notify observers at appropriate. Again, to me this makes sense
>> as presumably the compositor knows what is going on and if didn't it
>> still makes more sense to have it know what's going on. Either
>> approach is preferable in so far as we don't end up with a function on
>> Window that is public and assumed to be invoked at the right time.
>
>
> What I don't get is that whether ui::Layer, ui::Compositor, or RWHVAura
> calls Window::NotifyDamage, you still need a Window::NotifyDamage, don't
> you?

Yes, but it'll be something like OnLayerDamaged with an OVERRIDE and
ideally private.

>
>> What would be a lot cleaner is if the compositor told
>> Window it was damaged and window then told its observers.
>
> Why does having ui::Layer call the function change how the Window interface
> looks?

Two things:
. It becomes clear where this call is coming from as it's an override
of LayerDelegate.
. If we added in some new code path that damaged the layer things
would still just work.

  -Scott

>
>>
>>
>> Have I made my point any clearer this time around? Am I still speaking
>> gibberish?
>>
>>   -Scott
>>
>> On Mon, Aug 18, 2014 at 11:24 AM, Dana Jansens <danakj@google.com> wrote:
>> > On Mon, Aug 18, 2014 at 2:16 PM, Scott Violet <sky@chromium.org> wrote:
>> >>
>> >> Would it make sense to have a callback to the ui::Compositor and/or
>> >> ui::Layer some where in here?
>> >
>> >
>> > It would be a notification of damage the same way that RWHVAura right
>> > now
>> > knows about the damage and tries to notify the layer. The difference is
>> > that
>> > some cc:: class would call it via virtual interface instead of RWHVAura
>> > calling it, but that doesn't seem to affect the outcome of this CL or
>> > its
>> > discussion, or am I missing something that a callback would buy for us?
>> >
>> >>
>> >>
>> >> On Mon, Aug 18, 2014 at 8:44 AM, Dana Jansens <danakj@google.com>
>> >> wrote:
>> >> > On Mon, Aug 18, 2014 at 11:41 AM, Scott Violet <sky@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> The existing code notifies when SchedulePaint is called. This makes
>> >> >> sense as SchedulePaint was and is useful without the notification.
>> >> >> Patchset 1 adds a new function to window whose only job is to notify
>> >> >> observers. It changes no window state. This feels worse to me.
>> >> >>
>> >> >> How does DelegatedFrameHost end up getting bits onto the screen? I
>> >> >> assume the compositor is used at some point, but how? What is called
>> >> >> on the compositor side? Does DelegatedFrameHost go directly to cc
>> >> >> land, or does it talk at all to ui/compositor?
>> >> >
>> >> >
>> >> > DelegatedFrameHost inserts a new frame on a
>> >> > cc::DelegatedFrameProvider
>> >> > which
>> >> > causes the cc::DelegatedRendererLayer to be notified of new content
>> >> > and
>> >> > commit/draw the content. ui::Compositor and ui::Layer is not involved
>> >> > at
>> >> > all
>> >> > unless the size of the frame changes, in which case the
>> >> > cc::DelegatedFrameProvider is destroyed and passed through the
>> >> > ui::Layer.
>> >> >
>> >> >
>> >> >
>> >> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...
>> >> >
>> >> >>
>> >> >>
>> >> >>   -Scott
>> >> >>
>> >> >> On Fri, Aug 15, 2014 at 3:57 PM,  <enne@chromium.org> wrote:
>> >> >> > I talked with sky over chat, and it seems like maybe the
>> >> >> > VideoDetector
>> >> >> > is in
>> >> >> > the
>> >> >> > wrong place and should be a CompositorObserver and not a
>> >> >> > WindowObserver.
>> >> >> >
>> >> >> > That said, the VideoDetector also seems to need to know things
>> >> >> > about
>> >> >> > the
>> >> >> > window
>> >> >> > (e.g. visibility[0], full screen[1]) that the compositor doesn't
>> >> >> > know
>> >> >> > about
>> >> >> > but
>> >> >> > the window does.  Given that restriction, I don't see how to move
>> >> >> > VideoDetector
>> >> >> > to be a CompositorObserver instead.
>> >> >> >
>> >> >> > I think my first patch is strictly better than the status quo and
>> >> >> > would
>> >> >> > still
>> >> >> > like to land it.
>> >> >> >
>> >> >> > [0]
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>> >> >> > [1]
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/video_detec...
>> >> >> >
>> >> >> > https://codereview.chromium.org/464643003/
>> >> >
>> >> >
>> >
>> >
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-18 20:01:50]

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:694: if (!delegate_)
DCHECK(delegated_renderer_layer_)

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_dele...
File ui/compositor/layer_delegate.h (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_dele...
ui/compositor/layer_delegate.h:25: // A notification that some sublayer in the
tree has had a delegated
that this layer not sublayer?

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_unit...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_unit...
ui/compositor/layer_unittest.cc:1132: virtual void OnDelegatedFrameDamage(
A unit test that we call this would be nice?

https://codereview.chromium.org/464643003/diff/20001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/464643003/diff/20001/ui/views/view.h#newcode1121
ui/views/view.h:1121: virtual void OnDelegatedFrameDamage(
sky: Do we need to worry about a view being the owner of the layer? What's the
argument that all tab contents layers are owned by aura::Window?

[sky, 2014-08-18 20:17:20]

https://codereview.chromium.org/464643003/diff/20001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/464643003/diff/20001/ui/views/view.h#newcode1121
ui/views/view.h:1121: virtual void OnDelegatedFrameDamage(
On 2014/08/18 20:01:49, danakj wrote:
> sky: Do we need to worry about a view being the owner of the layer?

Views can certainly be the owner of layers.

> What's the
> argument that all tab contents layers are owned by aura::Window?

The aura related WebContents classes create aura::Windows. In fact content
doesn't use views at all.

[danakj, 2014-08-18 20:39:07]

On Mon, Aug 18, 2014 at 4:17 PM, <sky@chromium.org> wrote:

>
> https://codereview.chromium.org/464643003/diff/20001/ui/views/view.h
> File ui/views/view.h (right):
>
> https://codereview.chromium.org/464643003/diff/20001/ui/
> views/view.h#newcode1121
> ui/views/view.h:1121: virtual void OnDelegatedFrameDamage(
> On 2014/08/18 20:01:49, danakj wrote:
>
>> sky: Do we need to worry about a view being the owner of the layer?
>>
>
> Views can certainly be the owner of layers.
>
>
>  What's the
>> argument that all tab contents layers are owned by aura::Window?
>>
>
> The aura related WebContents classes create aura::Windows. In fact
> content doesn't use views at all.
>

OK thanks. There are so many LayerDelegate implementers, it leaves me very
unsure of relationships :) I think PS2 LGTM if sky is happy with it (would
like that tiny unit test tho)


>
> https://codereview.chromium.org/464643003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-08-18 20:40:35]

Oh LGTM from not-email.

[piman, 2014-08-18 21:02:57]

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:694: if (!delegate_)
On 2014/08/18 20:01:49, danakj wrote:
> DCHECK(delegated_renderer_layer_)

|| surface_layer_ ?

[sky, 2014-08-18 21:22:36]

Early on enne wanted approach patchset1. I'm not sure if my arguments have been
any more convincing?

https://codereview.chromium.org/464643003/diff/20001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/aura/window.cc#newcod...
ui/aura/window.cc:496: WindowObserver, observers_, OnWindowPaintScheduled(this,
rect));
Rename OnWindowPaintScheduled to your new name and nuke the call here.

[enne (OOO), 2014-08-18 23:32:21]

https://codereview.chromium.org/464643003/diff/20001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/aura/window.cc#newcod...
ui/aura/window.cc:496: WindowObserver, observers_, OnWindowPaintScheduled(this,
rect));
On 2014/08/18 21:22:35, sky wrote:
> Rename OnWindowPaintScheduled to your new name and nuke the call here.

Done.

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:694: if (!delegate_)
On 2014/08/18 20:01:49, danakj wrote:
> DCHECK(delegated_renderer_layer_)

Done.

On 2014/08/18 21:02:57, piman (OOO) wrote:
> || surface_layer_ ?

Done.

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_dele...
File ui/compositor/layer_delegate.h (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_dele...
ui/compositor/layer_delegate.h:25: // A notification that some sublayer in the
tree has had a delegated
On 2014/08/18 20:01:49, danakj wrote:
> that this layer not sublayer?

Done.

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_unit...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/464643003/diff/20001/ui/compositor/layer_unit...
ui/compositor/layer_unittest.cc:1132: virtual void OnDelegatedFrameDamage(
On 2014/08/18 20:01:49, danakj wrote:
> A unit test that we call this would be nice?

Done.

[enne (OOO), 2014-08-18 23:34:14]

danakj talked to me about layers and windows, and I stared at some more code. 
It looks like the video detector hooks up to every window that gets created, so
you can just post the OnDelegatedFrameDamage to the window owning the ui::Layer
that owns the delegated renderer layer that had the swap and it'll get there. 
That sounds more reasonable now that I understand what's going on more.

[danakj, 2014-08-18 23:35:54]

LGTM thanks for the test

[piman, 2014-08-18 23:37:39]

lgtm

[sky, 2014-08-18 23:43:57]

LGTM with the following changes.

https://codereview.chromium.org/464643003/diff/60001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/60001/ui/aura/window.cc#newcod...
ui/aura/window.cc:499: void Window::OnDelegatedFrameDamage(const gfx::Rect&
damage_rect_in_dip) {
Move to match position of header.

https://codereview.chromium.org/464643003/diff/60001/ui/compositor/layer_unit...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/464643003/diff/60001/ui/compositor/layer_unit...
ui/compositor/layer_unittest.cc:1625: };
DISALLOW_...

https://codereview.chromium.org/464643003/diff/80001/content/browser/web_cont...
File content/browser/web_contents/aura/shadow_layer_delegate.h (right):

https://codereview.chromium.org/464643003/diff/80001/content/browser/web_cont...
content/browser/web_contents/aura/shadow_layer_delegate.h:37: const gfx::Rect&
damage_rect_in_dip) OVERRIDE {}
Don't inline this.

https://codereview.chromium.org/464643003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/464643003/diff/80001/ui/views/view.h#newcode1122
ui/views/view.h:1122: const gfx::Rect& damage_rect_in_dip) OVERRIDE {}
don't inline this (see chrome style guide).

https://codereview.chromium.org/464643003/diff/80001/ui/wm/core/image_grid.h
File ui/wm/core/image_grid.h (right):

https://codereview.chromium.org/464643003/diff/80001/ui/wm/core/image_grid.h#...
ui/wm/core/image_grid.h:142: const gfx::Rect& damage_rect_in_dip) OVERRIDE {}
don't inline this either.

[enne (OOO), 2014-08-19 17:08:51]

https://codereview.chromium.org/464643003/diff/60001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/464643003/diff/60001/ui/aura/window.cc#newcod...
ui/aura/window.cc:499: void Window::OnDelegatedFrameDamage(const gfx::Rect&
damage_rect_in_dip) {
On 2014/08/18 23:43:57, sky wrote:
> Move to match position of header.

Done.

https://codereview.chromium.org/464643003/diff/60001/ui/compositor/layer_unit...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/464643003/diff/60001/ui/compositor/layer_unit...
ui/compositor/layer_unittest.cc:1625: };
On 2014/08/18 23:43:57, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/464643003/diff/80001/content/browser/web_cont...
File content/browser/web_contents/aura/shadow_layer_delegate.h (right):

https://codereview.chromium.org/464643003/diff/80001/content/browser/web_cont...
content/browser/web_contents/aura/shadow_layer_delegate.h:37: const gfx::Rect&
damage_rect_in_dip) OVERRIDE {}
On 2014/08/18 23:43:57, sky wrote:
> Don't inline this.

Done.

https://codereview.chromium.org/464643003/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/464643003/diff/80001/ui/views/view.h#newcode1122
ui/views/view.h:1122: const gfx::Rect& damage_rect_in_dip) OVERRIDE {}
On 2014/08/18 23:43:57, sky wrote:
> don't inline this (see chrome style guide).

Done.

https://codereview.chromium.org/464643003/diff/80001/ui/wm/core/image_grid.h
File ui/wm/core/image_grid.h (right):

https://codereview.chromium.org/464643003/diff/80001/ui/wm/core/image_grid.h#...
ui/wm/core/image_grid.h:142: const gfx::Rect& damage_rect_in_dip) OVERRIDE {}
On 2014/08/18 23:43:57, sky wrote:
> don't inline this either.

Done.

[enne (OOO), 2014-08-19 17:09:06]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2014-08-19 17:09:32]

The CQ bit was unchecked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-19 17:09:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/100001

[enne (OOO), 2014-08-19 17:10:24]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-08-19 17:10:27]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-19 17:10:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/100001

[commit-bot: I haz the power, 2014-08-19 21:06:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/43762)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-19 21:15:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-19 21:15:48]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2014-08-19 23:15:09]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-19 23:16:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/140001

[commit-bot: I haz the power, 2014-08-20 01:02:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 01:10:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 01:10:50]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[enne (OOO), 2014-08-20 17:08:18]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-20 17:09:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/140001

[commit-bot: I haz the power, 2014-08-20 19:03:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2014-08-20 19:07:00]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-08-21 20:54:19]

https://codereview.chromium.org/464643003/diff/180001/athena/wm/overview_tool...
File athena/wm/overview_toolbar.cc (right):

https://codereview.chromium.org/464643003/diff/180001/athena/wm/overview_tool...
athena/wm/overview_toolbar.cc:84: virtual void OnDelegatedFrameDamage(const
gfx::Rect& damage_rect_in_dip) {}
Fix compile failure here, oops.

https://codereview.chromium.org/464643003/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/464643003/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:310:
MOCK_METHOD2(OnDelegatedFrameDamage, void(aura::Window*, const gfx::Rect&));
Changed this file to use OnDelegatedFrameDamage instead.

https://codereview.chromium.org/464643003/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:310:
MOCK_METHOD2(OnDelegatedFrameDamage, void(aura::Window*, const gfx::Rect&));
Changed this file to expect OnDelegatedFrameDamage instead so these tests
continue to pass.

https://codereview.chromium.org/464643003/diff/180001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/464643003/diff/180001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:693: void Layer::OnDelegatedFrameDamage(const gfx::Rect
&damage_rect_in_dip) {
piman, danakj: removed DCHECK about having a delegated_renderer_layer_ or
texture_layer_ here.

Unfortunately this outline of code makes that problematic:

DelegatedFrameHost::SwapDelegatedFrame
  if (output_surface_id != last_output_surface_id_)
    DelegatedFrameHost::EvictDelegatedFrame
      Layer::SetShowPaintedContent
        Layer::SwitchToLayer(content or picture layer)
  Layer::OnDelegatedFrameData
    DCHECK(is a delegated renderer layer)
    BOOM

[enne (OOO), 2014-08-22 17:26:04]

https://codereview.chromium.org/464643003/diff/180001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/464643003/diff/180001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:693: void Layer::OnDelegatedFrameDamage(const gfx::Rect
&damage_rect_in_dip) {
On 2014/08/21 20:54:19, enne wrote:
> piman, danakj: removed DCHECK about having a delegated_renderer_layer_ or
> texture_layer_ here.
> 
> Unfortunately this outline of code makes that problematic:
> 
> DelegatedFrameHost::SwapDelegatedFrame
>   if (output_surface_id != last_output_surface_id_)
>     DelegatedFrameHost::EvictDelegatedFrame
>       Layer::SetShowPaintedContent
>         Layer::SwitchToLayer(content or picture layer)
>   Layer::OnDelegatedFrameData
>     DCHECK(is a delegated renderer layer)
>     BOOM

danakj (or piman): PTAL?

[piman, 2014-08-22 17:37:38]

LGTM. I suppose we could make sure we don't Layer::OnDelegatedFrameData when we
(temporarily) switch to a content layer, but it seems more trouble than worth.

[danakj, 2014-08-22 17:40:04]

On Fri, Aug 22, 2014 at 1:37 PM, <piman@chromium.org> wrote:

> LGTM. I suppose we could make sure we don't Layer::OnDelegatedFrameData
> when we
> (temporarily) switch to a content layer, but it seems more trouble than
> worth.
>

This would only happen when we switch, and then also have an empty frame
from the renderer. We could only call OnDelegated if the frame_provider_ is
not NULL. That seems a little better if we can keep the DCHECK? The damage
rect is totally wrong anyways in this case.


>
> https://codereview.chromium.org/464643003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-08-22 17:57:14]

On Fri, Aug 22, 2014 at 10:39 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Aug 22, 2014 at 1:37 PM, <piman@chromium.org> wrote:
>
>> LGTM. I suppose we could make sure we don't Layer::OnDelegatedFrameData
>> when we
>> (temporarily) switch to a content layer, but it seems more trouble than
>> worth.
>>
>
> This would only happen when we switch, and then also have an empty frame
> from the renderer. We could only call OnDelegated if the frame_provider_ is
> not NULL. That seems a little better if we can keep the DCHECK? The damage
> rect is totally wrong anyways in this case.
>

Fair enough.


>
>
>>
>> https://codereview.chromium.org/464643003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-08-22 18:02:21]

danakj: Ok, good call.  It is always empty in this case.  Added back DCHECK and
stopped sending when empty.

[danakj, 2014-08-22 18:06:05]

LGTM

[enne (OOO), 2014-08-22 18:07:58]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:10:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/220001

[enne (OOO), 2014-08-22 18:35:33]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:36:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/240001

[commit-bot: I haz the power, 2014-08-22 19:34:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 19:46:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 19:46:52]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2014-08-22 19:49:41]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-22 19:50:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/260001

[commit-bot: I haz the power, 2014-08-22 22:05:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 22:21:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 22:21:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2014-08-22 22:26:19]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-22 22:28:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/280001

[commit-bot: I haz the power, 2014-08-23 00:01:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2014-08-23 00:40:43]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-23 04:55:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/464643003/300001

[commit-bot: I haz the power, 2014-08-23 08:09:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-23 21:27:56]

Committed patchset #16 (300001) as 20e65cfb7cb4ab20a7f1dc08cdac47e29a82a30f

[commit-bot: I haz the power, 2014-09-10 02:31:00]

Patchset 16 (id:??) landed as
https://crrev.com/9a3ddb332a5ec5e21a5767b2891ba161cda5a3d0
Cr-Commit-Position: refs/heads/master@{#291582}