Fix repaint on :hover for empty elements with a border.

Fix repaint on :hover for empty elements with a border.

If the color changes on :hover, then the border of the element may need to
be repainted. Currently, we only repaint if an immediate child has a
border, but not if the element itself has a border. This causes a missing
repaint in some cases for bordered elements with whitespace-only contents.

Fixed by repainting when the element itself has a border.

BUG=275044
TEST=automated

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

[rune, 2014-01-29 19:55:49]

https://codereview.chromium.org/140673006/diff/1/LayoutTests/fast/repaint/hov...
File LayoutTests/fast/repaint/hover-pseudo-borders-whitespace.html (right):

https://codereview.chromium.org/140673006/diff/1/LayoutTests/fast/repaint/hov...
LayoutTests/fast/repaint/hover-pseudo-borders-whitespace.html:13: <script
src="resources/repaint.js"></script>

Could you perhaps use text-based-repaint.js instead to avoid the png?

https://codereview.chromium.org/140673006/diff/1/LayoutTests/fast/repaint/hov...
LayoutTests/fast/repaint/hover-pseudo-borders-whitespace.html:20:
eventSender.mouseUp();

There's something funny with the indentation here.

Also, shouldn't mouseMoveTo be enough for :hover?

https://codereview.chromium.org/140673006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/140673006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1910: 

This will trigger an unnecessary repaint if you have a border-color different
from currentColor, right?

[kinuko, 2014-01-30 04:55:25]

Sorry, I don't think I'm a good reviewer for this area-- deferring to pdr@

(Also the upload seems to have been incomplete, I can't open the side-by-side
view for Source/core/rendering/ files)

[andersr, 2014-01-30 09:36:13]

https://codereview.chromium.org/140673006/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/140673006/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1910: 
> This will trigger an unnecessary repaint if you have a border-color different
> from currentColor, right?

Yes, correct.

I did not optimize that case because
hasImmediateNonWhitespaceTextChildOrPropertiesDependentOnColor() also does not
optimize that case. I was unsure if there was a reason for that (other than "not
worth it"). (Which pdr@ should know).

Perhaps it's worthwhile to add something like
RenderStyle::hasBorderWithCurrentColor?

[pdr., 2014-02-03 03:57:25]

Sorry about the delayed review--we had a Chrome team offsite last week.

I think this is close but not quite right. As you noted in the change
description, the bug is just that we check our children but not ourselves for
border and outline changes.
Instead of adding a new function, can you just add the following to
hasImmediateNonWhitespaceTextChildOrPropertiesDependentOnColor:
if (style()->hasBorder() || style()->hasOutline())
  return true;

We will also need to update the test to check for outline changes. This is
probably best done using two rects, one that turns a border green and a second
that turns an outline green.

[pdr., 2014-02-06 19:25:32]

Excellent! LGTM assuming the bots are happy.

I'll go ahead and send this to the CQ to be committed.

[pdr., 2014-02-06 19:25:36]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-02-06 19:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andersr@opera.com/140673006/60001

[commit-bot: I haz the power, 2014-02-06 20:42:26]

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

[commit-bot: I haz the power, 2014-02-06 20:42:27]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[pdr., 2014-02-06 21:08:41]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-02-06 21:08:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andersr@opera.com/140673006/60001

[commit-bot: I haz the power, 2014-02-06 22:06:13]

Change committed as 166654