[Reland] Invalidate whitespace text on text color changes

[Reland] Invalidate whitespace text on text color changes

This patch updates the optimization added in [1] to invalidate
whitespace text on color changes because whitespace text can have
decorations such as underlines which still need to repaint.

The big idea of the optimization in [1] is to not unnecessarily repaint
expensive items such as images that are in an <a> element. On hover, <a>
will change color and underline styles which should not affect images.
This patch adds a test to prove this optimization still works and
prevents it from breaking in the future.

This patch contains a fix for a performance regression[2] where text
nodes without text (i.e., !firstTextBox) would be invalidated. This is
tested by fast/repaint/inline-outline-repaint-2.html but was incorrectly
thought to be a benign change in the original patch. Additional comments
have been added both about the optimization and about the tricky
empty-text case. Additionally, hasTextBoxes has been introduced to make
this area cleaner.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=152046
[2] https://src.chromium.org/viewvc/blink?view=rev&revision=202374

BUG=529944

[pdr., 2015-09-23 05:28:03]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[pdr., 2015-09-23 05:28:11]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[commit-bot: I haz the power, 2015-09-23 05:28:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363823002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363823002/1

[pdr., 2015-09-23 06:15:35]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-23 06:16:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363823002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363823002/20001

[commit-bot: I haz the power, 2015-09-23 06:48:33]

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

[commit-bot: I haz the power, 2015-09-23 06:48:33]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[chrishtr, 2015-09-23 16:26:56]

https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:1731: || (isText() && !isBR() &&
toLayoutText(this)->firstTextBox()))
What does firstTextBox have to do with it?

[pdr., 2015-09-23 16:38:46]

On 2015/09/23 at 16:26:56, chrishtr wrote:
>
https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.cpp:1731: || (isText() && !isBR() &&
toLayoutText(this)->firstTextBox()))
> What does firstTextBox have to do with it?

firstTextBox is a check to see if you have any text box descendants. Here's an
example:
<span id="boo">    
<span>abc</span></span>

The dom tree will look like this:
body
    span
        #text < boo, which contains 4 spaces and a newline
        span
            #text < abc

The line box tree will look like this:
body
    root inline box for body
        inline text box for "abc"

The 4 spaces and a newline of #boo do not end up in the line box tree, so the
check for firstTextBox lets us skip the #boo span.

[eae, 2015-09-23 16:40:08]

eae@chromium.org changed reviewers:
+ eae@chromium.org

[eae, 2015-09-23 16:40:09]

https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:1731: || (isText() && !isBR() &&
toLayoutText(this)->firstTextBox()))
On 2015/09/23 16:26:56, chrishtr wrote:
> What does firstTextBox have to do with it?

I believe firstTextBox is used to check whether the element is empty or not,
i.e. whether it has one or more text boxes.

A hasText method or similar on LayoutText would make this more obvious.

[chrishtr, 2015-09-23 16:42:25]

On 2015/09/23 at 16:40:09, eae wrote:
>
https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1363823002/diff/20001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.cpp:1731: || (isText() && !isBR() &&
toLayoutText(this)->firstTextBox()))
> On 2015/09/23 16:26:56, chrishtr wrote:
> > What does firstTextBox have to do with it?
> 
> I believe firstTextBox is used to check whether the element is empty or not,
i.e. whether it has one or more text boxes.
> 
> A hasText method or similar on LayoutText would make this more obvious.

+1

[pdr., 2015-09-23 17:41:14]

Done. PTAL?

[chrishtr, 2015-09-23 17:45:24]

https://codereview.chromium.org/1363823002/diff/60001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1363823002/diff/60001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:941: // TODO(pdr): Shouldn't this be
hasTextBoxes and not !hasTextBoxes?
Don't think so. The else clause looks at the first text box.

[pdr., 2015-09-23 17:49:21]

https://codereview.chromium.org/1363823002/diff/60001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1363823002/diff/60001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:941: // TODO(pdr): Shouldn't this be
hasTextBoxes and not !hasTextBoxes?
On 2015/09/23 at 17:45:24, chrishtr wrote:
> Don't think so. The else clause looks at the first text box.

Ah, right, I think it should be isBR() and not !isBR(). I'll look at this in a
followup and remove the comment.

[chrishtr, 2015-09-23 17:56:33]

lgtm

[eae, 2015-09-23 18:01:57]

LGTM, thanks pdr!

[pdr., 2015-09-23 18:17:33]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-09-23 18:17:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1363823002/#ps120001
(title: "Fix rebase")

[commit-bot: I haz the power, 2015-09-23 18:17:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363823002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363823002/120001

[commit-bot: I haz the power, 2015-09-23 18:18:35]

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

[commit-bot: I haz the power, 2015-09-23 18:18:36]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2015-09-23 18:27:26]

On 2015/09/23 at 18:18:36, commit-bot wrote:
> Try jobs failed on following builders:
>   cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
>   linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

I rebased this manually but it looks like we still have some issues on the CQ.
I'll CQ later today.

[pdr., 2015-09-23 18:35:30]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-09-23 18:35:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363823002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363823002/120001

[commit-bot: I haz the power, 2015-09-23 18:37:01]

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

[commit-bot: I haz the power, 2015-09-23 18:37:02]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2015-09-24 04:20:21]

On 2015/09/23 at 18:37:02, commit-bot wrote:
> Try jobs failed on following builders:
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

This ended up landing in https://codereview.chromium.org/1363913004 with
https://crrev.com/72f85dacbbef4c117efdb8e353036873d167b989.