views: Remove View::PaintCommon, merge it into View::Paint.

views: Remove View::PaintCommon, merge it into View::Paint.

This unifies the painting code path between Views that are and are not
backed by layers, they all go through View::Paint now.

View::Paint will only translate the canvas relative to the parent when
layer() is false, keeping the origin at the origin of the view when it
is backed by a layer().

R=sky
BUG=466426
Committed: https://crrev.com/6f9ccd4f30c31527f2a8f38878842b414b61c5fe
Cr-Commit-Position: refs/heads/master@{#322394}

[danakj, 2015-03-25 20:52:36]

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc#newcode1487
ui/views/view.cc:1487: if (!layer()->fills_bounds_opaquely())
I'm feeling more confident about this, so I removed the !layer() check here.
This is the LayerDelegate, it's only called by ui::Layer, so there must be such
a layer(). Bots should show us if tests expect something else.. but codesearch
suggests it won't.

[sky, 2015-03-25 22:55:05]

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc#newcode821
ui/views/view.cc:821: // match the position we will be painting at and
UpdateRootBounds() does the
I don't understand, bounds in the cullset are relative to the paint root.

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc#newcode1487
ui/views/view.cc:1487: if (!layer()->fills_bounds_opaquely())
On 2015/03/25 20:52:36, danakj wrote:
> I'm feeling more confident about this, so I removed the !layer() check here.
> This is the LayerDelegate, it's only called by ui::Layer, so there must be
such
> a layer(). Bots should show us if tests expect something else.. but codesearch
> suggests it won't.

Agreed. If we wanted something it should be a DCHECK(layer()). But I'm fine with
removing entirely.

[danakj, 2015-03-25 22:57:11]

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc#newcode821
ui/views/view.cc:821: // match the position we will be painting at and
UpdateRootBounds() does the
On 2015/03/25 22:55:05, sky wrote:
> I don't understand, bounds in the cullset are relative to the paint root.

I didn't dig too deep, but when I remove the ||layer() the white line disappears
under the active window in the ash taskbar. My comment might be wrong... how far
do you want me to dig here? This was meant to preserve behaviour for now.

I think the problem is when the PaintRoot() != this but layer() is true. Then
the bounds are relative to some PaintRoot() != |this|, but the painting is done
relative to |this|.

[sky, 2015-03-25 23:34:15]

If everything paints fine without your patch, then it's something in
your patch and you need to dig a bit. Otherwise you're disabling the
cullset.

On Wed, Mar 25, 2015 at 3:57 PM,  <danakj@chromium.org> wrote:
>
> https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc
> File ui/views/view.cc (right):
>
> https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc#newcode821
> ui/views/view.cc:821: // match the position we will be painting at and
> UpdateRootBounds() does the
> On 2015/03/25 22:55:05, sky wrote:
>>
>> I don't understand, bounds in the cullset are relative to the paint
>
> root.
>
> I didn't dig too deep, but when I remove the ||layer() the white line
> disappears under the active window in the ash taskbar. My comment might
> be wrong... how far do you want me to dig here? This was meant to
> preserve behaviour for now.
>
> I think the problem is when the PaintRoot() != this but layer() is true.
> Then the bounds are relative to some PaintRoot() != |this|, but the
> painting is done relative to |this|.
>
> https://codereview.chromium.org/1035833002/

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

[danakj, 2015-03-26 00:12:05]

On Wed, Mar 25, 2015 at 4:34 PM, Scott Violet <sky@chromium.org> wrote:

> If everything paints fine without your patch, then it's something in
> your patch and you need to dig a bit. Otherwise you're disabling the
> cullset.
>

Before my patch, OnPaintLayer calls PaintCommon() with an empty CullSet,
which disables the CullSet for its subtree. This is doing the same thing
assuming layer() is true iff OnPaintLayer is called, which I think that it
is?

This is the bug with CullSet I was asking about in a previous email.

On Wed, Mar 25, 2015 at 3:57 PM,  <danakj@chromium.org> wrote:
> >
> > https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc
> > File ui/views/view.cc (right):
> >
> >
> https://codereview.chromium.org/1035833002/diff/1/ui/views/view.cc#newcode821
> > ui/views/view.cc:821: // match the position we will be painting at and
> > UpdateRootBounds() does the
> > On 2015/03/25 22:55:05, sky wrote:
> >>
> >> I don't understand, bounds in the cullset are relative to the paint
> >
> > root.
> >
> > I didn't dig too deep, but when I remove the ||layer() the white line
> > disappears under the active window in the ash taskbar. My comment might
> > be wrong... how far do you want me to dig here? This was meant to
> > preserve behaviour for now.
> >
> > I think the problem is when the PaintRoot() != this but layer() is true.
> > Then the bounds are relative to some PaintRoot() != |this|, but the
> > painting is done relative to |this|.
> >
> > https://codereview.chromium.org/1035833002/
>

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

[sky, 2015-03-26 13:23:30]

Ah, ok. LGTM

[danakj, 2015-03-26 16:03:26]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-03-26 16:04:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1035833002/1

[commit-bot: I haz the power, 2015-03-26 16:08:33]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-26 16:09:54]

Patchset 1 (id:??) landed as
https://crrev.com/6f9ccd4f30c31527f2a8f38878842b414b61c5fe
Cr-Commit-Position: refs/heads/master@{#322394}