Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=549621
Committed: https://crrev.com/ea9b0ba419a3598bdb7f7c62afdf4409afa73ab1
Cr-Commit-Position: refs/heads/master@{#361695}

[bruthig, 2015-11-24 21:44:18]

bruthig@chromium.org changed reviewers:
+ pkasting@chromium.org

[bruthig, 2015-11-24 21:44:18]

pkasting@, can you PTAL?

[Peter Kasting, 2015-11-24 22:39:43]

I understand nothing about layers in Views, so most of the functional changes in
here are lost on me.  If you want a review for those you'll need to ask someone
more familiar with that stuff.

Otherwise, LGTM.

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host.cc (right):

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:89: SetDialogPosition(dlg_rect);
Nit: Just: SetDialogPosition(GetDialogPosition(gfx::Rect()));

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:151: return;
Should this hide the host if it's currently visible?

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:198: gfx::Rect dlg_rect =
GetDialogPosition(gfx::Rect());
This temp is now unused.

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host.h (right):

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.h:77: // in the z-order (HWND_TOP, not
HWND_TOPMOST for windows) shows the widget
Nit: Comma before "shows"

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.h:157: // between a shown and hidden
states.
Nit: "a" -> "the"

[bruthig, 2015-11-25 13:28:54]

bruthig@chromium.org changed reviewers:
+ ajuma@chromium.org

[bruthig, 2015-11-25 13:28:55]

ajuma@, can you take a look at the use of layers in this patch and verify
correctness?

[ajuma, 2015-11-25 14:34:02]

On 2015/11/25 13:28:55, bruthig wrote:
> ajuma@, can you take a look at the use of layers in this patch and verify
> correctness?

Use of layers lgtm. Please fix the bug number in the description.

[bruthig, 2015-11-25 16:01:57]

Description was changed from

==========
Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=649621
==========

to

==========
Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=549621
==========

[bruthig, 2015-11-25 16:55:07]

I have addressed comments and will assume previous LGTM's still stand.

pkasting@, can you look at my changes to DropdownBarHost::SetDialogPosition() in
dropdown_bar_host.cc though?

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host.cc (right):

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:89: SetDialogPosition(dlg_rect);
On 2015/11/24 22:39:43, Peter Kasting wrote:
> Nit: Just: SetDialogPosition(GetDialogPosition(gfx::Rect()));

Done.

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:151: return;
On 2015/11/24 22:39:43, Peter Kasting wrote:
> Should this hide the host if it's currently visible?

No, not from what I can tell anyway.  From what I can tell the host()->Show()
call below shouldn't be necessary either so I've removed that.  It should be
more consistent now.

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:198: gfx::Rect dlg_rect =
GetDialogPosition(gfx::Rect());
On 2015/11/24 22:39:43, Peter Kasting wrote:
> This temp is now unused.

Done.

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host.h (right):

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.h:77: // in the z-order (HWND_TOP, not
HWND_TOPMOST for windows) shows the widget
On 2015/11/24 22:39:43, Peter Kasting wrote:
> Nit: Comma before "shows"

Done.

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.h:157: // between a shown and hidden
states.
On 2015/11/24 22:39:43, Peter Kasting wrote:
> Nit: "a" -> "the"

Done.

[bruthig, 2015-11-25 17:52:20]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-11-25 17:52:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1472023004/#ps20001
(title: "Addressed comments from patch set 1.")

[commit-bot: I haz the power, 2015-11-25 17:53:40]

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

[commit-bot: I haz the power, 2015-11-25 18:00:16]

Description was changed from

==========
Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=549621
==========

to

==========
Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=549621
==========

[commit-bot: I haz the power, 2015-11-25 18:00:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-25 18:01:24]

Description was changed from

==========
Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=549621
==========

to

==========
Updated DropdownBarHost to use Layer clipping instead of OnPaint() clipping.

The Material Design ink drop ripple was recently applied to the material
design find in page dialog (https://codereview.chromium.org/1396903003).
This change required that some Views to paint to layers unfortunately
the clipping mechanism used for the find in page bar only applied to the
OnPaint() methods and did not work to clip layer trees.

This CL sets the contents view for the DropdownBarHost::widget_ to be a
new clipping view, and the FindBarView is added as a child to the
clipping view instead of as the DropdownBarHost::widget_'s contents view.

This CL has the added benefit that the the drop down bars will not need
to be repainted anymore for each animation frame.

TEST=Open the find in page bar with material design turned on. Click and
hold on the close button so the ink drop ripple is visible. Hit the Esc
key to dismiss the dialog.  The back button/ripple should not appear over
top of the toolbar and should be clipped at the same horizontal line as
the rest of the dialog.

BUG=549621

Committed: https://crrev.com/ea9b0ba419a3598bdb7f7c62afdf4409afa73ab1
Cr-Commit-Position: refs/heads/master@{#361695}
==========

[commit-bot: I haz the power, 2015-11-25 18:01:25]

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

[Peter Kasting, 2015-11-25 23:16:54]

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host.cc (right):

https://codereview.chromium.org/1472023004/diff/1/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host.cc:151: return;
On 2015/11/25 16:55:06, bruthig wrote:
> On 2015/11/24 22:39:43, Peter Kasting wrote:
> > Should this hide the host if it's currently visible?
> 
> No, not from what I can tell anyway.  From what I can tell the host()->Show()
> call below shouldn't be necessary either so I've removed that.  It should be
> more consistent now.

I don't really know what this code does so I can't speak to it either way.

You might rewrite this function for brevity as

  view_->SetSize(new_pos.size());
  if (!new_pos.IsEmpty())
    host()->SetBounds(new_pos);

But it's minor so feel free not to if you don't want.