Ensure autofill popup border is visible

Ensure autofill popup border is visible

Autofill popup views request a certain size in DIPs.
When we set the bounds of the view, these DIPs are converted to
pixels, rounding up to a whole number of pixels in the process. The
size in pixels is then converted back to DIPs, again rounding up.
This gets us the true size of the view in DIPs.

Because of these two round-ups, we may end up with 1 extra DIP in
either dimension over what was requested. However, the content we
draw is sized to the original request, leaving a row of pixels unused.

To add to the confusion, the autofill views have a 1 pixel border.
This border is computed from the actual size of the view, not the
requested size. Because of this, there may be a 1 pixel gap between
the border and the view's drawn content.

While we could expand the view's drawn content to meet the border,
we have the additional issue that, due to the rounding when converting
from pixels to dips, this extra DIP may be clipped during actual
rendering.

Becuase of this, it's safer to push the border in to meet the content.
This ensures that even if the border is partially clipped, you will
still see a part of it.

BUG=685867

[ericrk, 2017-02-15 10:09:01]

Patchset #2 (id:20001) has been deleted

[ericrk, 2017-02-15 10:09:07]

Patchset #1 (id:1) has been deleted

[ericrk, 2017-02-15 10:10:00]

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

[Peter Kasting, 2017-02-15 11:58:59]

I'm about to go to bed, but when I go to review this, I'm going to want to know
why we convert DIPs->px->DIPs when getting the view size.  That sounds very
wrong, and it seems like we shouldn't be doing that.  (Generally in views today
nothing should be working in px until the drawing stage.)  So unless it's
abundantly clear from the code why that's happening and why it's correct, please
explain that part in detail.

[Peter Kasting, 2017-02-16 00:30:26]

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

[Peter Kasting, 2017-02-16 00:30:26]

https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):

https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:108: // and right
of the view. We fill this with extra border pixels.
Rob, can you comment here?

It seems like the issue is that Screen::ScreenToDIPRectInWindow() and
Screen::DIPToScreenRectInWindow() both use ScaleToEnclosingRect().  The
Widget::SetBounds() call above uses the latter to convert the provided DIP
bounds to px bounds, but I guess we've (somehow) run through the former to
compute bounds() by the time we query it below(), leading to error accumulation.

This seems like a budget somewhere in the Widget level of things; calling
SetBounds() and then requesting bounds() should always give you back the same
rect.  Am I wrong?

[ericrk, 2017-02-16 01:45:27]

Description was changed from

==========
Ensure autofill popup border is visible

Autofill popup views request a certain size in DIPs.
When we set the bounds of the view, these DIPs are converted to
pixels, rounding up to a whole number of pixels in the process. The
size in pixels is then converted back to DIPs, again rounding up.
This gets us the true size of the view in DIPs.

Because of these two round-ups, we may end up with 1 extra DIP in
either dimension over what was requested. However, the content we
draw is sized to the original request, leaving a row of pixels unused.

To add to the confusion, the autofill views have a 1 pixel border.
This border is computed from the actual size of the view, not the
requested size. Because of this, there may be a 1 pixel gap between
the border and the view's drawn content.

While we could expand the view's drawn content to meet the border,
we have the additional issue that, due to the rounding when converting
from pixels to dips, this extra DIP may be during actual rendering.

Becuase of this, it's safer to push the border in to meet the content.
This ensures that even if the border is partially clipped, you will
still see a part of it.

BUG=685867
==========

to

==========
Ensure autofill popup border is visible

Autofill popup views request a certain size in DIPs.
When we set the bounds of the view, these DIPs are converted to
pixels, rounding up to a whole number of pixels in the process. The
size in pixels is then converted back to DIPs, again rounding up.
This gets us the true size of the view in DIPs.

Because of these two round-ups, we may end up with 1 extra DIP in
either dimension over what was requested. However, the content we
draw is sized to the original request, leaving a row of pixels unused.

To add to the confusion, the autofill views have a 1 pixel border.
This border is computed from the actual size of the view, not the
requested size. Because of this, there may be a 1 pixel gap between
the border and the view's drawn content.

While we could expand the view's drawn content to meet the border,
we have the additional issue that, due to the rounding when converting
from pixels to dips, this extra DIP may be clipped during actual
rendering.

Becuase of this, it's safer to push the border in to meet the content.
This ensures that even if the border is partially clipped, you will
still see a part of it.

BUG=685867
==========

[ericrk, 2017-02-16 02:13:49]

On 2017/02/16 00:30:26, Peter Kasting wrote:
>
https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):
> 
>
https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:108: // and right
> of the view. We fill this with extra border pixels.
> Rob, can you comment here?
> 
> It seems like the issue is that Screen::ScreenToDIPRectInWindow() and
> Screen::DIPToScreenRectInWindow() both use ScaleToEnclosingRect().  The
> Widget::SetBounds() call above uses the latter to convert the provided DIP
> bounds to px bounds, but I guess we've (somehow) run through the former to
> compute bounds() by the time we query it below(), leading to error
accumulation.
> 
> This seems like a budget somewhere in the Widget level of things; calling
> SetBounds() and then requesting bounds() should always give you back the same
> rect.  Am I wrong?

Sorry for the delay, missed these comments earlier. I understand your concerns,
here's a bit more info on my understanding of things:

From what I can see, we start out with bounds in DIPs that we use to size the
autofill popup. We ask the system to resize the widget to these bounds via:
"GetWidget()->SetBounds(delegate_->popup_bounds());"

Because the widget needs to be a whole number of pixels, we may need to either
round up or down when converting the requested DIPs to pixels. We chose to round
up to avoid clipping content.

After setting these bounds on the widget, we eventually hit
WindowTreeHost::UpdateRootWindowSizeInPixels. This function is responsible for
re-calculating the DIP bounds of the window (and view) from its pixel size.

Previously (before crrev.com/2263453002), we would round down (floor) during
this computation (taking the enclosed, not enclosing, rect). This would result
in the DIPs > pixels > DIPs round-tripping successfully, providing the behavior
you describe.

While this sequence makes sense logically, and means that you always get the
same thing from bounds() that you set via SetBounds(), it leads to the problem
that we may under-fill the window. This could result in black bars at the edge
of the window.

To avoid these black bars, crrev.com/2263453002 caused us to round up when
converting back to DIPs as well. This ensures that the entire window is always
filled, but also means that:
a) We sometimes get an extra DIP when going pixels > DIPs > pixels and
b) The last dip may be partially clipped - this is what led to the missing
borders on the autofill boxes.

This CL was an attempt to work within these constraints to get something that
looked good for autofill boxes - I was hesitant to change the pixel > DIP
rounding, as every approach I tried seemed to have edge cases where I would
either see black borders, or have clipped content.

I understand that it's not necessarily an ideal solution. This code isn't really
my area of expertise, so I'm very open to suggestions (or to let someone else
handle this, if that would be easier :D).

[ericrk, 2017-02-16 02:18:41]

On 2017/02/16 02:13:49, ericrk wrote:
> On 2017/02/16 00:30:26, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
> > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:108: // and
right
> > of the view. We fill this with extra border pixels.
> > Rob, can you comment here?
> > 
> > It seems like the issue is that Screen::ScreenToDIPRectInWindow() and
> > Screen::DIPToScreenRectInWindow() both use ScaleToEnclosingRect().  The
> > Widget::SetBounds() call above uses the latter to convert the provided DIP
> > bounds to px bounds, but I guess we've (somehow) run through the former to
> > compute bounds() by the time we query it below(), leading to error
> accumulation.
> > 
> > This seems like a budget somewhere in the Widget level of things; calling
> > SetBounds() and then requesting bounds() should always give you back the
same
> > rect.  Am I wrong?
> 
> Sorry for the delay, missed these comments earlier. I understand your
concerns,
> here's a bit more info on my understanding of things:
> 
> From what I can see, we start out with bounds in DIPs that we use to size the
> autofill popup. We ask the system to resize the widget to these bounds via:
> "GetWidget()->SetBounds(delegate_->popup_bounds());"
> 
> Because the widget needs to be a whole number of pixels, we may need to either
> round up or down when converting the requested DIPs to pixels. We chose to
round
> up to avoid clipping content.
> 
> After setting these bounds on the widget, we eventually hit
> WindowTreeHost::UpdateRootWindowSizeInPixels. This function is responsible for
> re-calculating the DIP bounds of the window (and view) from its pixel size.
> 
> Previously (before crrev.com/2263453002), we would round down (floor) during
> this computation (taking the enclosed, not enclosing, rect). This would result
> in the DIPs > pixels > DIPs round-tripping successfully, providing the
behavior
> you describe.
> 
> While this sequence makes sense logically, and means that you always get the
> same thing from bounds() that you set via SetBounds(), it leads to the problem
> that we may under-fill the window. This could result in black bars at the edge
> of the window.
> 
> To avoid these black bars, crrev.com/2263453002 caused us to round up when
> converting back to DIPs as well. This ensures that the entire window is always
> filled, but also means that:
> a) We sometimes get an extra DIP when going pixels > DIPs > pixels and
oops... meant DIPs > pixels > DIPs here.
> b) The last dip may be partially clipped - this is what led to the missing
> borders on the autofill boxes.
> 
> This CL was an attempt to work within these constraints to get something that
> looked good for autofill boxes - I was hesitant to change the pixel > DIP
> rounding, as every approach I tried seemed to have edge cases where I would
> either see black borders, or have clipped content.
> 
> I understand that it's not necessarily an ideal solution. This code isn't
really
> my area of expertise, so I'm very open to suggestions (or to let someone else
> handle this, if that would be easier :D).

[robliao, 2017-02-16 02:29:23]

On 2017/02/16 02:13:49, ericrk wrote:
> On 2017/02/16 00:30:26, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/autofill/autofill_popup_base_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2694183005/diff/40001/chrome/browser/ui/views...
> > chrome/browser/ui/views/autofill/autofill_popup_base_view.cc:108: // and
right
> > of the view. We fill this with extra border pixels.
> > Rob, can you comment here?
> > 
> > It seems like the issue is that Screen::ScreenToDIPRectInWindow() and
> > Screen::DIPToScreenRectInWindow() both use ScaleToEnclosingRect().  The
> > Widget::SetBounds() call above uses the latter to convert the provided DIP
> > bounds to px bounds, but I guess we've (somehow) run through the former to
> > compute bounds() by the time we query it below(), leading to error
> accumulation.
> > 
> > This seems like a budget somewhere in the Widget level of things; calling
> > SetBounds() and then requesting bounds() should always give you back the
same
> > rect.  Am I wrong?
> 
> Sorry for the delay, missed these comments earlier. I understand your
concerns,
> here's a bit more info on my understanding of things:
> 
> From what I can see, we start out with bounds in DIPs that we use to size the
> autofill popup. We ask the system to resize the widget to these bounds via:
> "GetWidget()->SetBounds(delegate_->popup_bounds());"
> 
> Because the widget needs to be a whole number of pixels, we may need to either
> round up or down when converting the requested DIPs to pixels. We chose to
round
> up to avoid clipping content.
> 
> After setting these bounds on the widget, we eventually hit
> WindowTreeHost::UpdateRootWindowSizeInPixels. This function is responsible for
> re-calculating the DIP bounds of the window (and view) from its pixel size.
> 
> Previously (before crrev.com/2263453002), we would round down (floor) during
> this computation (taking the enclosed, not enclosing, rect). This would result
> in the DIPs > pixels > DIPs round-tripping successfully, providing the
behavior
> you describe.
> 
> While this sequence makes sense logically, and means that you always get the
> same thing from bounds() that you set via SetBounds(), it leads to the problem
> that we may under-fill the window. This could result in black bars at the edge
> of the window.
> 
> To avoid these black bars, crrev.com/2263453002 caused us to round up when
> converting back to DIPs as well. This ensures that the entire window is always
> filled, but also means that:
> a) We sometimes get an extra DIP when going pixels > DIPs > pixels and
> b) The last dip may be partially clipped - this is what led to the missing
> borders on the autofill boxes.
> 
> This CL was an attempt to work within these constraints to get something that
> looked good for autofill boxes - I was hesitant to change the pixel > DIP
> rounding, as every approach I tried seemed to have edge cases where I would
> either see black borders, or have clipped content.
> 
> I understand that it's not necessarily an ideal solution. This code isn't
really
> my area of expertise, so I'm very open to suggestions (or to let someone else
> handle this, if that would be easier :D).

Cool. I was in the middle of tracing the bounds setting machinery and wasn't
able to get autofill to show up in my version of Chrome.

Taking a step back, it seems unexpected for bounds to change right after you set
them even in the face of DIP, but I can also see how the widget reserves the
right to set its bounds to a proper value based off of a scaling factor.
Round-tripping the value once should stabilize the value as both DIP and
physical receive values that are evenly divisible by the scale factor.

It is unfortunate that this is the only way to notify the caller of this
information. 
Ideally (maybe), the widget would keep track of DIPs provided to the system and
know when there are legitimate system changes so it could replay those DIP
values back to the setters maintaining DIP -> pixel -> DIP. DIP issues that
result in underfilling the window could be resolved at render time. That's
beyond the scope of this review.

[Peter Kasting, 2017-02-16 07:17:23]

Any fix we do here will be imperfect, since without the DIP-to-px conversion
work (or an equivalent like having fractional DIPs), the mismatch between views
being in DIPs and windows being in px will always cause the potential for the
window being smaller or bigger than the view expects, leaving to clipping or
unpainted areas.

It seems like people think that we should choose "clipping" over "unpainted
areas" as the failure mode here.  I'm not really sure one is better than the
other, but let's go with that.  If that's the case, then it doesn't really make
sense that when setting the window bounds, we scale the provided DIP rect to the
enclosing px rect.  That's a recipe for unpainted areas.  Instead, it seems like
this conversion should compute the enclosed px rect, potentially clipping 1 px
off what the view wants to draw.

If we make this conversion, then I think setting and then retrieving the bounds
becomes stable again, because the "scale to enclosing DIP rect" change in
https://codereview.chromium.org/2263453002 effectively inverts this.  This means
that when something asks for particular bounds in DIPs, it will actually get
them.

This still leaves the problem that the actual widget may be too small to draw
what the view wants if the view draws directly in DIPs.  It might fix this to
make things like the autofill border drawing unscale the canvas and then draw
directly in pixels on the unscaled canvas.  This is the fix we use in a ton of
other places to address problems with fractional DPI, and while it's not
perfect, it usually improves things pretty significantly.

Thoughts?

[ericrk, 2017-02-16 20:58:21]

On 2017/02/16 07:17:23, Peter Kasting wrote:
> Any fix we do here will be imperfect, since without the DIP-to-px conversion
> work (or an equivalent like having fractional DIPs), the mismatch between
views
> being in DIPs and windows being in px will always cause the potential for the
> window being smaller or bigger than the view expects, leaving to clipping or
> unpainted areas.
> 
> It seems like people think that we should choose "clipping" over "unpainted
> areas" as the failure mode here.  I'm not really sure one is better than the
> other, but let's go with that.  If that's the case, then it doesn't really
make
> sense that when setting the window bounds, we scale the provided DIP rect to
the
> enclosing px rect.  That's a recipe for unpainted areas.  Instead, it seems
like
> this conversion should compute the enclosed px rect, potentially clipping 1 px
> off what the view wants to draw.
> 
> If we make this conversion, then I think setting and then retrieving the
bounds
> becomes stable again, because the "scale to enclosing DIP rect" change in
> https://codereview.chromium.org/2263453002 effectively inverts this.  This
means
> that when something asks for particular bounds in DIPs, it will actually get
> them.
> 
> This still leaves the problem that the actual widget may be too small to draw
> what the view wants if the view draws directly in DIPs.  It might fix this to
> make things like the autofill border drawing unscale the canvas and then draw
> directly in pixels on the unscaled canvas.  This is the fix we use in a ton of
> other places to address problems with fractional DPI, and while it's not
> perfect, it usually improves things pretty significantly.
> 
> Thoughts?

I agree that taking the enclosed rect for the actual pixel bounds seems like a
good idea - it should keep the current "clipping" behavior without causing the
DIP bounds to grow when we got DIPs > pixels > DIPs.

I also agree that unscaling the canvas to draw the border should fix things.

How would you like to go about making these changes - I'm happy to take a shot
at either, but this also isn't an area of code I typically work in, so it might
be better to defer to others? Let me know.

[Peter Kasting, 2017-02-16 21:23:43]

On 2017/02/16 20:58:21, ericrk wrote:
> How would you like to go about making these changes - I'm happy to take a shot
> at either, but this also isn't an area of code I typically work in, so it
might
> be better to defer to others? Let me know.

I encourage you to take a shot at these :).  Good reviewers of the "enclosed
rect" change might be sky or robliao, good reviewers of the "unscaled drawing"
change might be me/estade.

For canvas unscaling examples, search for calls to UndoDeviceScaleFactor().

[Peter Kasting, 2017-02-17 02:38:49]

Note also https://codereview.chromium.org/2694933002/ , which might help w.r.t.
the border unscaled drawing part.