Make updates to InkDropHighlight to pave the way for improved ink drops

Make updates to InkDropHighlight to pave the way for improved ink drops
at fractional scales.

Also make some updates to BookmarkBarView button types and
InkDropPaintedLayerDelegates to fix bookmark button highlights.

Test with --force-device-scale-factor=1.25x.

BUG=706228
Review-Url: https://codereview.chromium.org/2786543002
Cr-Commit-Position: refs/heads/master@{#460993}
Committed: https://chromium.googlesource.com/chromium/src/+/7caded54fe6a6ab2db2fa9c8960699bae7fe1e5c

[Evan Stade, 2017-03-29 01:49:48]

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

[Evan Stade, 2017-03-29 01:49:48]

I could keep fixing things but this seemed like a good stopping point /
discussion starter.

The general principle is that the layer will have to still be sized in integral
DIP, but what gets painted may be fractional dip (but integral px). It's easier
to keep everything in DIP and just switch from integers (like gfx::Size) to
floats (gfx::SizeF) than to try to rejigger everything to be px.

We can do this piecemeal because anything that was a size before can be
converted to sizef with no loss of functionality, then updated to the
appropriately adjusted or pixel-aligned floating point operations at a later
date.

[bruthig, 2017-03-30 16:37:08]

I just want to make sure I understand the intention here so let me re-iterate
back my understanding...

In general the InkDrop ripples and highlights will be specified in DIP's but
switching to RectF/SizeF will allow clients to use px if desired, like in the
bookmark button cases.

The PaintedLayerDelegates will be the spot that's responsible for converting the
DIP's to px boundaries.

Am I on the same page?

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:237: gfx::RectF
bounds((gfx::Rect(size())));
nit: One too many '()' set

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:240:
GetWidget()->GetLayer()->GetCompositor()->device_scale_factor(),
Will we be doing this enough to warrant a util function like 'float
PxToDip(Layer*, int px)' ?

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:362: gfx::RectF
bounds((gfx::Rect(size())));
nit: Too many '()'

[Evan Stade, 2017-03-30 17:49:38]

On 2017/03/30 16:37:08, bruthig wrote:
> I just want to make sure I understand the intention here so let me re-iterate
> back my understanding...
> 
> In general the InkDrop ripples and highlights will be specified in DIP's but
> switching to RectF/SizeF will allow clients to use px if desired, like in the
> bookmark button cases.

Close but no, they'll still be DIPs. It's just that we can specify them in
fractional DIPs which is more or less equivalent to specifying in pixels. For
example, at 1.25x we could not use DIP to specify "4 pixels". Closest we could
get is 3.75. With fractional values we can say 3.2DIP, i.e. exactly 4 pixels.
(The DIP size would vary by scale factor, generally using rounding math as you
see in BBV.)

> 
> The PaintedLayerDelegates will be the spot that's responsible for converting
the
> DIP's to px boundaries.

Almost. PLD is responsible for /making sure/ the dips convert to px boundaries.
Most callers will still use integral DIPs, and will rely on the default rounding
behavior. The only reason BBV has to do something extra is because it has the
extra and atypical desire of applying a 1px (not 1 dip) gap above and below.

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:237: gfx::RectF
bounds((gfx::Rect(size())));
On 2017/03/30 16:37:08, bruthig wrote:
> nit: One too many '()' set

that is actually necessary or the compiler complains.

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:240:
GetWidget()->GetLayer()->GetCompositor()->device_scale_factor(),
On 2017/03/30 16:37:08, bruthig wrote:
> Will we be doing this enough to warrant a util function like 'float
> PxToDip(Layer*, int px)' ?

maybe, ui/compositor/dip_util.h has similar things but not quite what we want
here.

[bruthig, 2017-03-30 17:57:01]

lgtm

[Evan Stade, 2017-03-30 18:07:51]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2017-03-30 20:37:54]

LGTM

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:237: gfx::RectF
bounds((gfx::Rect(size())));
On 2017/03/30 17:49:38, Evan Stade wrote:
> On 2017/03/30 16:37:08, bruthig wrote:
> > nit: One too many '()' set
> 
> that is actually necessary or the compiler complains.

optional: creating a rect just to create a rectf is overkill. how about (0, 0,
size().width(), size().height())?

Maybe a bit more characters.

[Evan Stade, 2017-03-31 00:04:12]

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2786543002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:237: gfx::RectF
bounds((gfx::Rect(size())));
On 2017/03/30 20:37:54, sky wrote:
> On 2017/03/30 17:49:38, Evan Stade wrote:
> > On 2017/03/30 16:37:08, bruthig wrote:
> > > nit: One too many '()' set
> > 
> > that is actually necessary or the compiler complains.
> 
> optional: creating a rect just to create a rectf is overkill. how about (0, 0,
> size().width(), size().height())?
> 
> Maybe a bit more characters.

Perhaps there should be a RectF ctor that takes a Size?

[Evan Stade, 2017-03-31 00:04:17]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-31 00:05:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 02:04:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490918657468450,
"parent_rev": "7116a59daed455bb5ea8a7968129ed6f2638d199", "commit_rev":
"7caded54fe6a6ab2db2fa9c8960699bae7fe1e5c"}

[commit-bot: I haz the power, 2017-03-31 02:05:05]

Description was changed from

==========
Make updates to InkDropHighlight to pave the way for improved ink drops
at fractional scales.

Also make some updates to BookmarkBarView button types and
InkDropPaintedLayerDelegates to fix bookmark button highlights.

Test with --force-device-scale-factor=1.25x.

BUG=706228
==========

to

==========
Make updates to InkDropHighlight to pave the way for improved ink drops
at fractional scales.

Also make some updates to BookmarkBarView button types and
InkDropPaintedLayerDelegates to fix bookmark button highlights.

Test with --force-device-scale-factor=1.25x.

BUG=706228

Review-Url: https://codereview.chromium.org/2786543002
Cr-Commit-Position: refs/heads/master@{#460993}
Committed:
https://chromium.googlesource.com/chromium/src/+/7caded54fe6a6ab2db2fa9c89606...
==========

[commit-bot: I haz the power, 2017-03-31 02:05:07]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7caded54fe6a6ab2db2fa9c89606...