omnibox: Don't paint children from inside OnPaint().

omnibox: Don't paint children from inside OnPaint().

OnPaint() is not part of the recursion, it won't have access to the
neccessary pieces to do recursion.

If you need to do painting while inside the recursion, this should
happen in PaintChildren(). So move the contents of OnPaint() to
PaintChildren().

R=pkasting
BUG=466426
Committed: https://crrev.com/46b6dbf670b260257fc26ac118deab8178bea6a0
Cr-Commit-Position: refs/heads/master@{#322520}

[danakj, 2015-03-26 00:51:44]

danakj@chromium.org changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[Peter Kasting, 2015-03-26 03:02:56]

I don't really understand the CL description -- "the recursion" refers to
something I'm not familiar with -- so I don't know how correct the comment below
is.

https://codereview.chromium.org/1016033011/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/1016033011/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:426: // Top
border.
Seems like maybe painting the top and bottom border should still be in
OnPaint(), and the stuff above here in PaintChildren()?

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

On Wed, Mar 25, 2015 at 8:02 PM, <pkasting@chromium.org> wrote:

> I don't really understand the CL description -- "the recursion" refers to
> something I'm not familiar with -- so I don't know how correct the comment
> below
> is.
>

PaintChildren recurses on its children, the paint walk is the recursion I'm
referring to. I'm going to make PaintChildren take something other than a
canvas. OnPaint will only get a canvas, so it won't be able to call
PaintChildren (and it's not meant to do that). Other subclasses of View do
painting that they need to do after/between children inside of
PaintChildren.


>
>
> https://codereview.chromium.org/1016033011/diff/1/chrome/
> browser/ui/views/omnibox/omnibox_popup_contents_view.cc
> File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc
> (right):
>
> https://codereview.chromium.org/1016033011/diff/1/chrome/
> browser/ui/views/omnibox/omnibox_popup_contents_view.cc#newcode426
> chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:426: //
> Top border.
> Seems like maybe painting the top and bottom border should still be in
> OnPaint(), and the stuff above here in PaintChildren()?
>

That would change the order of the painting such that they are painted
before (ie below) children. If that's okay then I'm happy to do that, doing
painting in OnPaint is always nicer for me :)

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

[Peter Kasting, 2015-03-26 19:10:37]

On Thu, Mar 26, 2015 at 9:13 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Mar 25, 2015 at 8:02 PM, <pkasting@chromium.org> wrote:
>
>> Seems like maybe painting the top and bottom border should still be in
>>
> OnPaint(), and the stuff above here in PaintChildren()?
>>
>
> That would change the order of the painting such that they are painted
> before (ie below) children. If that's okay then I'm happy to do that, doing
> painting in OnPaint is always nicer for me :)
>

The easiest way to check would be to simply take a look at the omnibox
dropdown before and after such a change, to make sure that the divider line
between the toolbar and the dropdown is still there, as well as the bottom
border/shadow.  As long as it is, the change is fine.

If those get partially overwritten, it's probably still possible to fix it
by adjusting the clipPath() call in PaintChildren(), it just depends how
much effort we care to put in.

PK

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

[danakj, 2015-03-26 22:07:52]

Thanks Peter. PTAL

I removed a virtual (not overridden) function, changed the Rect->Path->Clip to
just Rect->Clip which is cheaper, and moved the borders to OnPaint.

I had to adjust the clip in PaintChildren to clip out the top border, the bottom
was already okay.

[Peter Kasting, 2015-03-27 00:20:35]

LGTM

[danakj, 2015-03-27 00:57:10]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-03-27 00:57:25]

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

[commit-bot: I haz the power, 2015-03-27 02:12:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-27 02:13:35]

Patchset 2 (id:??) landed as
https://crrev.com/46b6dbf670b260257fc26ac118deab8178bea6a0
Cr-Commit-Position: refs/heads/master@{#322520}