Compute correct repaint rect for decorated RenderSVGRoots

Compute correct repaint rect for decorated RenderSVGRoots

The current implementation of clippedOverflowRectForRepaint() uses the
generic repaint-rect calculations in SVGRenderSupport. Those in turn make
use of repaintRectInLocalCoordinates(), which for RenderSVGRoot is the
union of the painted children (w/ some expansion). If there're no children,
or they do not fill the entire content box, then a repaint would not
repaint the correct parts.
Fix by calculating the union of the border-box and the SVG content
when the SVG root is decorated (has background/border/etc.)

BUG=354305
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170890

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170960

[fs, 2014-03-25 14:00:29]

So this ended up being a fun one...

Since this changes what absoluteClippedOverflowRect() returns for RenderSVGRoot,
it requires rebaselining of quite a few text-based tests. It also feels relevant
to ask if the current implementation is more interesting for DRT, or if the
new/fixed one is (i.e. should the DRT code be adjusted to keep dumping what it
dumps prior to this fix.)

(Didn't start any tryjobs because of the above.)

[Stephen Chennney, 2014-03-25 14:22:49]

On 2014/03/25 14:00:29, fs wrote:
> So this ended up being a fun one...
> 
> Since this changes what absoluteClippedOverflowRect() returns for
RenderSVGRoot,
> it requires rebaselining of quite a few text-based tests. It also feels
relevant
> to ask if the current implementation is more interesting for DRT, or if the
> new/fixed one is (i.e. should the DRT code be adjusted to keep dumping what it
> dumps prior to this fix.)
> 
> (Didn't start any tryjobs because of the above.)

I think we just pay the cost of fixing it and rebasing the tests. I don't see
any reason to change the DRT code.

Is the new rect always bigger than the old one? It seems like it to me because
the SVG cannot escape its RenderReplaced paint area. Do you agree with that
analysis?

[fs, 2014-03-25 14:37:47]

On 2014/03/25 14:22:49, Stephen Chenney wrote:
> On 2014/03/25 14:00:29, fs wrote:
> > So this ended up being a fun one...
> > 
> > Since this changes what absoluteClippedOverflowRect() returns for
> RenderSVGRoot,
> > it requires rebaselining of quite a few text-based tests. It also feels
> relevant
> > to ask if the current implementation is more interesting for DRT, or if the
> > new/fixed one is (i.e. should the DRT code be adjusted to keep dumping what
it
> > dumps prior to this fix.)
> > 
> > (Didn't start any tryjobs because of the above.)
> 
> I think we just pay the cost of fixing it and rebasing the tests. I don't see
> any reason to change the DRT code.
> 
> Is the new rect always bigger than the old one? It seems like it to me because
> the SVG cannot escape its RenderReplaced paint area. Do you agree with that
> analysis?

Yes, until we support "overflow: visible" on the SVG root, that should be the
behavior. (And consequently such an implementation/fix would likely end up
re-introducing clippedOverflowRectForRepaint...)

I guess I should get cracking on the TestExpectations then... (Will be huge...)

[fs, 2014-03-26 15:31:14]

On 2014/03/25 14:37:47, fs wrote:
> On 2014/03/25 14:22:49, Stephen Chenney wrote:
> > On 2014/03/25 14:00:29, fs wrote:
> > > So this ended up being a fun one...
> > > 
> > > Since this changes what absoluteClippedOverflowRect() returns for
> > RenderSVGRoot,
> > > it requires rebaselining of quite a few text-based tests.
...
> > I think we just pay the cost of fixing it and rebasing the tests. I don't
see
> > any reason to change the DRT code.
> > 
...
> I guess I should get cracking on the TestExpectations then... (Will be
huge...)

Ok, added all ~700 to TestExpectations... It's currently failing presubmit
because of errors like the following:

LayoutTests/TestExpectations:NNN More specific entry for foo/bar/baz.html on
line LayoutTests/TestExpectations:NNN overrides line
LayoutTests/TestExpectations:MMM. foo/bar/baz.html

Is there any way around that? (AFAICT I still want to rebaseline those... even
for the excepted platforms/configurations)

[f(malita), 2014-03-26 15:38:38]

On 2014/03/26 15:31:14, fs wrote:
> On 2014/03/25 14:37:47, fs wrote:
> > On 2014/03/25 14:22:49, Stephen Chenney wrote:
> > > On 2014/03/25 14:00:29, fs wrote:
> > > > So this ended up being a fun one...
> > > > 
> > > > Since this changes what absoluteClippedOverflowRect() returns for
> > > RenderSVGRoot,
> > > > it requires rebaselining of quite a few text-based tests.
> ...
> > > I think we just pay the cost of fixing it and rebasing the tests. I don't
> see
> > > any reason to change the DRT code.
> > > 
> ...
> > I guess I should get cracking on the TestExpectations then... (Will be
> huge...)
> 
> Ok, added all ~700 to TestExpectations... It's currently failing presubmit
> because of errors like the following:
> 
> LayoutTests/TestExpectations:NNN More specific entry for foo/bar/baz.html on
> line LayoutTests/TestExpectations:NNN overrides line
> LayoutTests/TestExpectations:MMM. foo/bar/baz.html
> 
> Is there any way around that? (AFAICT I still want to rebaseline those... even
> for the excepted platforms/configurations)

The way I usually handle that is by commenting out the original expectation + a
note that it should be re-enabled after rebaselining. This may not scale for 700
tests though.

[fs, 2014-03-26 15:58:32]

On 2014/03/26 15:38:38, Florin Malita wrote:
> On 2014/03/26 15:31:14, fs wrote:
> > On 2014/03/25 14:37:47, fs wrote:
> > > On 2014/03/25 14:22:49, Stephen Chenney wrote:
> > > > On 2014/03/25 14:00:29, fs wrote:
> > > > > So this ended up being a fun one...
> > > > > 
> > > > > Since this changes what absoluteClippedOverflowRect() returns for
> > > > RenderSVGRoot,
> > > > > it requires rebaselining of quite a few text-based tests.
> > ...
> > > > I think we just pay the cost of fixing it and rebasing the tests. I
don't
> > see
> > > > any reason to change the DRT code.
> > > > 
> > ...
> > > I guess I should get cracking on the TestExpectations then... (Will be
> > huge...)
> > 
> > Ok, added all ~700 to TestExpectations... It's currently failing presubmit
> > because of errors like the following:
> > 
> > LayoutTests/TestExpectations:NNN More specific entry for foo/bar/baz.html on
> > line LayoutTests/TestExpectations:NNN overrides line
> > LayoutTests/TestExpectations:MMM. foo/bar/baz.html
> > 
> > Is there any way around that? (AFAICT I still want to rebaseline those...
even
> > for the excepted platforms/configurations)
> 
> The way I usually handle that is by commenting out the original expectation +
a
> note that it should be re-enabled after rebaselining. This may not scale for
700
> tests though.

I think this was only around 10 that triggered that warning though, so that
strategy could work I suppose.

[fs, 2014-03-26 16:43:31]

I hope the TestExpectations are sorted now, so PTAL (at the actual change too!
Yes, it's hiding in there somewhere... ;-)).

[fs, 2014-03-28 12:00:03]

On 2014/03/26 16:43:31, fs wrote:
> I hope the TestExpectations are sorted now, so PTAL (at the actual change too!
> Yes, it's hiding in there somewhere... ;-)).

Bots looking green now (win_blink_rel looks like a flake), so PTAL.

[pdr., 2014-03-31 16:51:31]

I don't think this change is right, or I need more convincing.

This change increases the repaint rects by a lot for what seems to be a minor
edge case. I think this change will hurt performance overall. Can we do better
here?

https://codereview.chromium.org/211193002/diff/200001/LayoutTests/svg/custom/...
File LayoutTests/svg/custom/change-background-color.html (right):

https://codereview.chromium.org/211193002/diff/200001/LayoutTests/svg/custom/...
LayoutTests/svg/custom/change-background-color.html:13: }, 0);
Is this setTimeout needed? I think the rAF should be enough.

Nit: indent (window.testrunner)

[fs, 2014-04-01 12:33:07]

On 2014/03/31 16:51:31, pdr wrote:
> I don't think this change is right, or I need more convincing.

"Pessimistically correct" is the term that comes to mind...
 
> This change increases the repaint rects by a lot for what seems to be a minor
> edge case. I think this change will hurt performance overall. Can we do better
> here?

It ought to only increase the repaint rect when repainting the root itself
though - which shouldn't be _that_ common.
I think we might be able to optimize for (what I think is) the common case
though - only include the border-box in the repaint rect if the box actually has
decorations (background, borders, etc.). Doing that fixes the TC (haven't run
that version through any wider testing yet.) With
https://codereview.chromium.org/220853002/ in mind though I'm trying to make
that integrate with the other mechanisms in a "good way" (i.e. it can't be "one
or the other" but rather the union.)
 
>
https://codereview.chromium.org/211193002/diff/200001/LayoutTests/svg/custom/...
> File LayoutTests/svg/custom/change-background-color.html (right):
> 
>
https://codereview.chromium.org/211193002/diff/200001/LayoutTests/svg/custom/...
> LayoutTests/svg/custom/change-background-color.html:13: }, 0);
> Is this setTimeout needed? I think the rAF should be enough.

Let's see if I remember this... The timeout is to avoid processing on 'load' (if
background is set on load there won't be a repaint triggered), and rAF can't be
used directly on load either, because then it'll all just end up in the first
frame rendered.

[fs, 2014-04-01 14:38:03]

On 2014/04/01 12:33:07, fs wrote:
> On 2014/03/31 16:51:31, pdr wrote:
> > I don't think this change is right, or I need more convincing.
> 
> "Pessimistically correct" is the term that comes to mind...
>  
> > This change increases the repaint rects by a lot for what seems to be a
minor
> > edge case. I think this change will hurt performance overall. Can we do
better
> > here?
> 
> It ought to only increase the repaint rect when repainting the root itself
> though - which shouldn't be _that_ common.
> I think we might be able to optimize for (what I think is) the common case
> though - only include the border-box in the repaint rect if the box actually
has
> decorations (background, borders, etc.). Doing that fixes the TC (haven't run
> that version through any wider testing yet.) With
> https://codereview.chromium.org/220853002/ in mind though I'm trying to make
> that integrate with the other mechanisms in a "good way" (i.e. it can't be
"one
> or the other" but rather the union.)

Uploaded a version that attempts to keep the old repaint behavior unless the box
has decorations - TestExpectations/rebaselines down to 9. pdr PTAL.

[pdr., 2014-04-01 18:55:47]

I think the results of this patch look great--just a handful of tests need
updating and I think that's correct.

I'm still a little concerned about the implementation though.

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:365: if (object.isRoot())
Should this be an assert instead of a conditional?

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:366: return object.hasBackground()
|| object.style()->hasBorder();
I suspect there are additional accoutrements we're leaving off here. Outline is
one example. I'd prefer to not reinvent these checks if they exist in CSS/HTML.

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:373: // This is an open-coded
aggregate of SVGRenderSupport::clippedOverflowRectForRepaint,
This is a lot of code but it's sort of expected for the svg<->html boundary. Why
don't we have shouldRepaintBorderBox-esque functions in html-land? How does a
<div> handle this, for example?

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:391: if
(shouldRepaintBorderBox(*this)) {
I see how this can work when changing or gaining a background or border, but
what about when the background or border goes away: won't your repaint rect be
too small?

[pdr., 2014-04-02 06:15:31]

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:373: // This is an open-coded
aggregate of SVGRenderSupport::clippedOverflowRectForRepaint,
On 2014/04/01 18:55:48, pdr wrote:
> This is a lot of code but it's sort of expected for the svg<->html boundary.
Why
> don't we have shouldRepaintBorderBox-esque functions in html-land? How does a
> <div> handle this, for example?

Looking at this again, I see falling back to regular
RenderReplaced::clippedOverflowRectForRepaint was your original approach. After
looking into how a RenderBox handles this, I think this function is fine after
all. I'm still not sure this patch correctly handles about the case where we
lose an outline/border/background though.

[fs, 2014-04-02 08:01:54]

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:365: if (object.isRoot())
On 2014/04/01 18:55:48, pdr wrote:
> Should this be an assert instead of a conditional?

You mean:

ASSERT(object.isRoot());

(or the negated)?

No, I don't see why it should. There's two cases to care about: isRoot() -
standalone SVG's where the <svg> element is the document root; !isRoot() -
inline SVG where <html> (99.99999999999% at least) is the document root.

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:366: return object.hasBackground()
|| object.style()->hasBorder();
On 2014/04/01 18:55:48, pdr wrote:
> I suspect there are additional accoutrements we're leaving off here. Outline
is
> one example. I'd prefer to not reinvent these checks if they exist in
CSS/HTML.

The "original" check/predicate (from RenderBoxModelObject::updateFromStyle) was:

hasBackground() || styleToUse->hasBorder() || styleToUse->hasAppearance() ||
styleToUse->boxShadow()

(which is then overridden for isRoot()). Of that I dropped the last two (because
I couldn't see how appearance would apply to the root, and box-shadows weren't).
I suppose could move that into a method and re-use here (appearance/box-shadow
is probably unlikely enough.)

Outline should probably be handled in some way...

https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:373: // This is an open-coded
aggregate of SVGRenderSupport::clippedOverflowRectForRepaint,
On 2014/04/02 06:15:31, pdr wrote:
> On 2014/04/01 18:55:48, pdr wrote:
> > This is a lot of code but it's sort of expected for the svg<->html boundary.
> Why
> > don't we have shouldRepaintBorderBox-esque functions in html-land? How does
a
> > <div> handle this, for example?
> 
> Looking at this again, I see falling back to regular
> RenderReplaced::clippedOverflowRectForRepaint was your original approach.
After
> looking into how a RenderBox handles this, I think this function is fine after
> all. I'm still not sure this patch correctly handles about the case where we
> lose an outline/border/background though.

I look into 'outline', and the mentioned transitions.

[fs, 2014-04-02 14:04:55]

Fixed repaint w/ outline (not sure that worked previously, hopefully better now)
and some other cases. Added a bunch of new tests.

[pdr., 2014-04-02 19:50:39]

On 2014/04/02 14:04:55, fs wrote:
> Fixed repaint w/ outline (not sure that worked previously, hopefully better
now)
> and some other cases. Added a bunch of new tests.

I like this patch, thank you for sticking with it. LGTM.

@schenney, @fmalita, did you have further thoughts?

[pdr., 2014-04-02 19:52:36]

On 2014/04/02 08:01:54, fs wrote:
>
https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
> File Source/core/rendering/svg/RenderSVGRoot.cpp (right):
> 
>
https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
> Source/core/rendering/svg/RenderSVGRoot.cpp:365: if (object.isRoot())
> On 2014/04/01 18:55:48, pdr wrote:
> > Should this be an assert instead of a conditional?
> 
> You mean:
> 
> ASSERT(object.isRoot());
> 
> (or the negated)?
> 
> No, I don't see why it should. There's two cases to care about: isRoot() -
> standalone SVG's where the <svg> element is the document root; !isRoot() -
> inline SVG where <html> (99.99999999999% at least) is the document root.

You were right here. I was being dumb and conflating isRoot with isSVGRoot.

[Stephen Chennney, 2014-04-02 19:59:20]

On 2014/04/02 19:52:36, pdr wrote:
> On 2014/04/02 08:01:54, fs wrote:
> >
>
https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
> > File Source/core/rendering/svg/RenderSVGRoot.cpp (right):
> > 
> >
>
https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
> > Source/core/rendering/svg/RenderSVGRoot.cpp:365: if (object.isRoot())
> > On 2014/04/01 18:55:48, pdr wrote:
> > > Should this be an assert instead of a conditional?
> > 
> > You mean:
> > 
> > ASSERT(object.isRoot());
> > 
> > (or the negated)?
> > 
> > No, I don't see why it should. There's two cases to care about: isRoot() -
> > standalone SVG's where the <svg> element is the document root; !isRoot() -
> > inline SVG where <html> (99.99999999999% at least) is the document root.
> 
> You were right here. I was being dumb and conflating isRoot with isSVGRoot.

I have only one nit, which is to avoid the double invocation of style() by
passing the style to calculateHasBoxDecorations(). You could then probably
inline the method. Really, though, don't let that stop you if others disagree
with my pedantic performance suggestions.

[f(malita), 2014-04-02 20:45:47]

lgtm

https://codereview.chromium.org/211193002/diff/260001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/211193002/diff/260001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:225: m_overflow.clear();
Not the fault of this CL I think, but poking at this in subclasses smells.

[fs, 2014-04-03 07:58:38]

On 2014/04/02 19:59:20, Stephen Chenney wrote:
> On 2014/04/02 19:52:36, pdr wrote:
> > On 2014/04/02 08:01:54, fs wrote:
> > >
> >
>
https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
> > > File Source/core/rendering/svg/RenderSVGRoot.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/211193002/diff/220001/Source/core/rendering/s...
> > > Source/core/rendering/svg/RenderSVGRoot.cpp:365: if (object.isRoot())
> > > On 2014/04/01 18:55:48, pdr wrote:
> > > > Should this be an assert instead of a conditional?
> > > 
> > > You mean:
> > > 
> > > ASSERT(object.isRoot());
> > > 
> > > (or the negated)?
> > > 
> > > No, I don't see why it should. There's two cases to care about: isRoot() -
> > > standalone SVG's where the <svg> element is the document root; !isRoot() -
> > > inline SVG where <html> (99.99999999999% at least) is the document root.
> > 
> > You were right here. I was being dumb and conflating isRoot with isSVGRoot.
> 
> I have only one nit, which is to avoid the double invocation of style() by
> passing the style to calculateHasBoxDecorations(). You could then probably
> inline the method. Really, though, don't let that stop you if others disagree
> with my pedantic performance suggestions.

This method actually ends up growing quite a lot (it's ~250 bytes on my setup),
so I almost hope it wouldn't inline that easily =) - but admittedly I'm a bit of
a codesize buff...

[fs, 2014-04-03 07:58:58]

https://codereview.chromium.org/211193002/diff/260001/Source/core/rendering/s...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/211193002/diff/260001/Source/core/rendering/s...
Source/core/rendering/svg/RenderSVGRoot.cpp:225: m_overflow.clear();
On 2014/04/02 20:45:48, Florin Malita wrote:
> Not the fault of this CL I think, but poking at this in subclasses smells.

Yes, this pattern seemed to be used in a few places (clear()+addVisual...).

[fs, 2014-04-03 09:20:20]

At a whim I decided to change all tests to (text-based-)repaint tests before
landing - seemed appropriate.

(Additionally I forgot to note earlier that the add-outline case does not PASS,
but that seemed to be the case for pretty much anything I tested with [<div>,
<img>, ...]. It's easily noticable with the repaint test output though. So I
smell a generic bug in that area...)

[fs, 2014-04-03 09:20:57]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-03 09:21:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/211193002/280001

[commit-bot: I haz the power, 2014-04-03 09:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 09:55:55]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[pdr., 2014-04-03 17:02:55]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-04-03 17:03:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/211193002/300001

[fs, 2014-04-03 17:36:47]

While trying to come up with a workaround for the 1px difference between the
various bots for some TCs, I noticed another bug stemming from the
hasBoxDecorations bit not being stable enough for this use. I made a change to
track said state explicitly instead (since the state of that bit can change
without a layout I added recomputing of it on certain style-changes too). This
may warrant a new look, so PTAL.

[pdr., 2014-04-03 20:07:21]

On 2014/04/03 17:36:47, fs wrote:
> While trying to come up with a workaround for the 1px difference between the
> various bots for some TCs, I noticed another bug stemming from the
> hasBoxDecorations bit not being stable enough for this use. I made a change to
> track said state explicitly instead (since the state of that bit can change
> without a layout I added recomputing of it on certain style-changes too). This
> may warrant a new look, so PTAL.

I think we need to ask some repaint after layout folks to review this.

+cc dsinclair and jchaffraix.

[dsinclair, 2014-04-04 16:14:46]

On 2014/04/03 20:07:21, pdr wrote:
> On 2014/04/03 17:36:47, fs wrote:
> > While trying to come up with a workaround for the 1px difference between the
> > various bots for some TCs, I noticed another bug stemming from the
> > hasBoxDecorations bit not being stable enough for this use. I made a change
to
> > track said state explicitly instead (since the state of that bit can change
> > without a layout I added recomputing of it on certain style-changes too).
This
> > may warrant a new look, so PTAL.
> 
> I think we need to ask some repaint after layout folks to review this.
> 
> +cc dsinclair and jchaffraix.

This LGTM. I know jchaffraix has been doing a lot of work around overflow's so
he may have other comments.

[pdr., 2014-04-04 16:28:00]

LGTM as well.

[fs, 2014-04-04 16:53:19]

On 2014/04/04 16:14:46, dsinclair wrote:
> On 2014/04/03 20:07:21, pdr wrote:
> > On 2014/04/03 17:36:47, fs wrote:
> > > While trying to come up with a workaround for the 1px difference between
the
> > > various bots for some TCs, I noticed another bug stemming from the
> > > hasBoxDecorations bit not being stable enough for this use. I made a
change
> to
> > > track said state explicitly instead (since the state of that bit can
change
> > > without a layout I added recomputing of it on certain style-changes too).
> This
> > > may warrant a new look, so PTAL.
> > 
> > I think we need to ask some repaint after layout folks to review this.
> > 
> > +cc dsinclair and jchaffraix.
> 
> This LGTM. I know jchaffraix has been doing a lot of work around overflow's so
> he may have other comments.

I can let this linger for a while, in case there's additional comments - say
Tuesday morning (CEST)?

[pdr., 2014-04-04 17:08:05]

On 2014/04/04 16:53:19, fs wrote:
> On 2014/04/04 16:14:46, dsinclair wrote:
> > On 2014/04/03 20:07:21, pdr wrote:
> > > On 2014/04/03 17:36:47, fs wrote:
> > > > While trying to come up with a workaround for the 1px difference between
> the
> > > > various bots for some TCs, I noticed another bug stemming from the
> > > > hasBoxDecorations bit not being stable enough for this use. I made a
> change
> > to
> > > > track said state explicitly instead (since the state of that bit can
> change
> > > > without a layout I added recomputing of it on certain style-changes
too).
> > This
> > > > may warrant a new look, so PTAL.
> > > 
> > > I think we need to ask some repaint after layout folks to review this.
> > > 
> > > +cc dsinclair and jchaffraix.
> > 
> > This LGTM. I know jchaffraix has been doing a lot of work around overflow's
so
> > he may have other comments.
> 
> I can let this linger for a while, in case there's additional comments - say
> Tuesday morning (CEST)?

I think we're good to go now if you want. Jchaffraix's calendar looks pretty
full today and, at 1.5 weeks in for this review, I think we can address
additional concerns in a followup :)

[fs, 2014-04-04 17:12:24]

On 2014/04/04 17:08:05, pdr wrote:
> On 2014/04/04 16:53:19, fs wrote:
> > On 2014/04/04 16:14:46, dsinclair wrote:
> > > On 2014/04/03 20:07:21, pdr wrote:
> > > > On 2014/04/03 17:36:47, fs wrote:
> > > > > While trying to come up with a workaround for the 1px difference
between
> > the
> > > > > various bots for some TCs, I noticed another bug stemming from the
> > > > > hasBoxDecorations bit not being stable enough for this use. I made a
> > change
> > > to
> > > > > track said state explicitly instead (since the state of that bit can
> > change
> > > > > without a layout I added recomputing of it on certain style-changes
> too).
> > > This
> > > > > may warrant a new look, so PTAL.
> > > > 
> > > > I think we need to ask some repaint after layout folks to review this.
> > > > 
> > > > +cc dsinclair and jchaffraix.
> > > 
> > > This LGTM. I know jchaffraix has been doing a lot of work around
overflow's
> so
> > > he may have other comments.
> > 
> > I can let this linger for a while, in case there's additional comments - say
> > Tuesday morning (CEST)?
> 
> I think we're good to go now if you want. Jchaffraix's calendar looks pretty
> full today and, at 1.5 weeks in for this review, I think we can address
> additional concerns in a followup :)

Fine by me - I'll tick the box now and hope for the best =) Have a nice weekend!

[fs, 2014-04-04 17:12:32]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-04 17:12:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/211193002/320001

[commit-bot: I haz the power, 2014-04-04 18:20:50]

Change committed as 170890

[fs, 2014-04-07 10:25:39]

...and what were the odds of this landing at the same time as RAL got disalbed
(by default) again...

Updated the *-border-* tests to be less sensitive to the order of repaint rects.
Relanding with that change.

[fs, 2014-04-07 10:25:50]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-07 10:25:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/211193002/330001

[fs, 2014-04-07 10:26:06]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-07 12:54:06]

Change committed as 170960