Reinstate BoxPainter's unrederable background rrect logic

Reinstate BoxPainter's unrederable background rrect logic

http://crrev.com/2008913002 replaced the unrenderable case with an
assert, on the assumption that higher-level logic should ensure a
renderable rrect.

But in certain cases we compute the background rrect by shrinking the
border rrect - and this can yield degenerate/unrenderable results when
some radii are not large enough to accomodate the requested inset.

E.g.

 width: 100
 left radius: 0
 right radius: 100
 border-width: 10

We attempt to inset and shrink all radii by 5 (half border-width), but
we can only reduce the right radius since the left one is already zero.
So we end up with

 width: 90
 left radius: 0
 right radius: 95

which is not renderable.

This CL restores the clip-based fallback for unrenderable rrects.

BUG=614825
R=chrishtr@chromium.org

Committed: https://crrev.com/804f289c635c1ccf84e8079d4a5c78d4d8786516
Cr-Commit-Position: refs/heads/master@{#403321}

[chrishtr, 2016-06-30 00:48:14]

https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:426: if (info.isRoundedFill
&& !border.isRenderable()) {
Why not ensure the border is renderable at the time it's inset? That is the
place where the business logic has best opportunity
to do the right thing, rather than falling back to non-rounded in catchall logic
later. Maybe the inset logic should choose the largest value it can while still
being renderable for instance?

[f(malita), 2016-06-30 13:53:38]

https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):

https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:426: if (info.isRoundedFill
&& !border.isRenderable()) {
On 2016/06/30 00:48:14, chrishtr wrote:
> Why not ensure the border is renderable at the time it's inset? That is the
> place where the business logic has best opportunity
> to do the right thing, rather than falling back to non-rounded in catchall
logic
> later. Maybe the inset logic should choose the largest value it can while
still
> being renderable for instance?

Yeah, that was my first thought also, but it turns out we do want the
"unrenderable" version in these cases.

It mostly comes into play when computing the inner border shape, and there is
only one correct answer: the inner shape is parallel to the outer shape, inset
by border width.

If we tweak the logic to only inset by the max value which maintains the
renderable invariant, we may not inset enough - e.g. if one radius is 100%, it
leaves no room for insetting at all => our inner edge == outer edge, which is
incorrect.

Another option is to "borrow" the inset from the other radius when needed - e.g.
if top radius is 0 and bottom radius is 100% and we attempt to inset by 5, we
subtract 10 from the bottom radius only (can't reduce top at all).  But this
means our bottom radius is now smaller than it should be, which again yields
incorrect results (inner edge no longer parallel to outer edge).  E.g. we end up
with something like this http://i.imgur.com/4VIu0Sf.png instead of
http://i.imgur.com/sKWw01U.png.

So yeah, the correct geometry is "unrenderable", according to the
FloatRoundedRect definition.  Which doesn't mean we cannot render it, but that
we must use more convoluted logic rather than a single drawRRect call.

[chrishtr, 2016-06-30 20:08:53]

On 2016/06/30 at 13:53:38, fmalita wrote:
>
https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/BoxPainter.cpp:426: if
(info.isRoundedFill && !border.isRenderable()) {
> On 2016/06/30 00:48:14, chrishtr wrote:
> > Why not ensure the border is renderable at the time it's inset? That is the
> > place where the business logic has best opportunity
> > to do the right thing, rather than falling back to non-rounded in catchall
logic
> > later. Maybe the inset logic should choose the largest value it can while
still
> > being renderable for instance?
> 
> Yeah, that was my first thought also, but it turns out we do want the
"unrenderable" version in these cases.
> 
> It mostly comes into play when computing the inner border shape, and there is
only one correct answer: the inner shape is parallel to the outer shape, inset
by border width.
> 
> If we tweak the logic to only inset by the max value which maintains the
renderable invariant, we may not inset enough - e.g. if one radius is 100%, it
leaves no room for insetting at all => our inner edge == outer edge, which is
incorrect.
> 
> Another option is to "borrow" the inset from the other radius when needed -
e.g. if top radius is 0 and bottom radius is 100% and we attempt to inset by 5,
we subtract 10 from the bottom radius only (can't reduce top at all).  But this
means our bottom radius is now smaller than it should be, which again yields
incorrect results (inner edge no longer parallel to outer edge).  E.g. we end up
with something like this http://i.imgur.com/4VIu0Sf.png instead of
http://i.imgur.com/sKWw01U.png.
> 
> So yeah, the correct geometry is "unrenderable", according to the
FloatRoundedRect definition.  Which doesn't mean we cannot render it, but that
we must use more convoluted logic rather than a single drawRRect call.

The alternative, which is AIUI what the current patch implements, is to give up
and have no rounded corners at all for the un-renderable
rect if any corner is un-renderable. Isn't that worse than tweaking down the
values to fit? The developer will I think consider it very weird that in some
corner cases we seem to be ignoring their style.

[f(malita), 2016-06-30 20:23:20]

On 2016/06/30 20:08:53, chrishtr wrote:
> On 2016/06/30 at 13:53:38, fmalita wrote:
> >
>
https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2112643002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/BoxPainter.cpp:426: if
> (info.isRoundedFill && !border.isRenderable()) {
> > On 2016/06/30 00:48:14, chrishtr wrote:
> > > Why not ensure the border is renderable at the time it's inset? That is
the
> > > place where the business logic has best opportunity
> > > to do the right thing, rather than falling back to non-rounded in catchall
> logic
> > > later. Maybe the inset logic should choose the largest value it can while
> still
> > > being renderable for instance?
> > 
> > Yeah, that was my first thought also, but it turns out we do want the
> "unrenderable" version in these cases.
> > 
> > It mostly comes into play when computing the inner border shape, and there
is
> only one correct answer: the inner shape is parallel to the outer shape, inset
> by border width.
> > 
> > If we tweak the logic to only inset by the max value which maintains the
> renderable invariant, we may not inset enough - e.g. if one radius is 100%, it
> leaves no room for insetting at all => our inner edge == outer edge, which is
> incorrect.
> > 
> > Another option is to "borrow" the inset from the other radius when needed -
> e.g. if top radius is 0 and bottom radius is 100% and we attempt to inset by
5,
> we subtract 10 from the bottom radius only (can't reduce top at all).  But
this
> means our bottom radius is now smaller than it should be, which again yields
> incorrect results (inner edge no longer parallel to outer edge).  E.g. we end
up
> with something like this http://i.imgur.com/4VIu0Sf.png instead of
> http://i.imgur.com/sKWw01U.png.
> > 
> > So yeah, the correct geometry is "unrenderable", according to the
> FloatRoundedRect definition.  Which doesn't mean we cannot render it, but that
> we must use more convoluted logic rather than a single drawRRect call.
> 
> The alternative, which is AIUI what the current patch implements, is to give
up
> and have no rounded corners at all for the un-renderable
> rect if any corner is un-renderable.

RoundedInnerRectClipper handles unrenderable rrects correctly (by clipping one
rounded corner at a time), so the CL restores proper degenerate rrect handling.

[chrishtr, 2016-06-30 20:42:05]

lgtm

We discussed offline, and it seems RoundedInnerRectClipper has reasonable
fallback logic already,
this CL just enables it.

[f(malita), 2016-06-30 20:43:25]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2016-06-30 20:43:57]

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, 2016-06-30 23:04:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-30 23:04:32]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 23:07:09]

Description was changed from

==========
Reinstate BoxPainter's unrederable background rrect logic

http://crrev.com/2008913002 replaced the unrenderable case with an
assert, on the assumption that higher-level logic should ensure a
renderable rrect.

But in certain cases we compute the background rrect by shrinking the
border rrect - and this can yield degenerate/unrenderable results when
some radii are not large enough to accomodate the requested inset.

E.g.

 width: 100
 left radius: 0
 right radius: 100
 border-width: 10

We attempt to inset and shrink all radii by 5 (half border-width), but
we can only reduce the right radius since the left one is already zero.
So we end up with

 width: 90
 left radius: 0
 right radius: 95

which is not renderable.

This CL restores the clip-based fallback for unrenderable rrects.

BUG=614825
R=chrishtr@chromium.org
==========

to

==========
Reinstate BoxPainter's unrederable background rrect logic

http://crrev.com/2008913002 replaced the unrenderable case with an
assert, on the assumption that higher-level logic should ensure a
renderable rrect.

But in certain cases we compute the background rrect by shrinking the
border rrect - and this can yield degenerate/unrenderable results when
some radii are not large enough to accomodate the requested inset.

E.g.

 width: 100
 left radius: 0
 right radius: 100
 border-width: 10

We attempt to inset and shrink all radii by 5 (half border-width), but
we can only reduce the right radius since the left one is already zero.
So we end up with

 width: 90
 left radius: 0
 right radius: 95

which is not renderable.

This CL restores the clip-based fallback for unrenderable rrects.

BUG=614825
R=chrishtr@chromium.org

Committed: https://crrev.com/804f289c635c1ccf84e8079d4a5c78d4d8786516
Cr-Commit-Position: refs/heads/master@{#403321}
==========

[commit-bot: I haz the power, 2016-06-30 23:07:10]

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