Improve shadow support in EllipsisBox.

Improve shadow support in EllipsisBox.

Previously, the ellipsis generated by text-overflow: ellipsis would not
render correctly if multiple text-shadows were specified, or if the text
color was not opaque (but there was a shadow).

This brings the behaviour of EllipsisBox more in line with InlineTextBox (as far as text-shadow is concerned).

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

[jbroman, 2013-05-15 14:54:49]

I intend to refactor some of the logic for rendering these shadows in a
following CL, but it was suggested to me that I capture the correct behaviour in
code and layout tests before I do that.

[jbroman, 2013-05-16 15:22:02]

Ping (still awaiting review).

[alokp, 2013-05-16 16:32:58]

text-bounds lgtm after you address my comment.

I defer to people more familiar with text painting for other changes.

https://codereview.chromium.org/15001034/diff/8001/Source/core/rendering/Elli...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15001034/diff/8001/Source/core/rendering/Elli...
Source/core/rendering/EllipsisBox.cpp:64: textRunPaintInfo.bounds =
FloatRect(boxOrigin, FloatSize(logicalWidth(), logicalHeight()));
Use boxRect here.

[jbroman, 2013-05-16 16:43:17]

https://codereview.chromium.org/15001034/diff/8001/Source/core/rendering/Elli...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15001034/diff/8001/Source/core/rendering/Elli...
Source/core/rendering/EllipsisBox.cpp:64: textRunPaintInfo.bounds =
FloatRect(boxOrigin, FloatSize(logicalWidth(), logicalHeight()));
On 2013/05/16 16:32:58, Alok Priyadarshi wrote:
> Use boxRect here.

Done.

[Stephen Chennney, 2013-05-16 17:03:14]

Just some code clarity and robustness issues.

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:41: Color textColor =
style->visitedDependentColor(CSSPropertyWebkitTextFillColor);
Rename this "defaultTextColor".

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:45: Color actualTextColor = textColor;
Then name this "textColor".

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:76: if (shadow->next() || !opaque) {
What the abstract meaning of this? I would prefer a variable "needToDrawAgain"
or something, describing why you need to take a different path. It also avoids
duplicating the test further down, which is a recipe for bugs if someone ever
makes changes. You can even use it as the while condition.

[jbroman, 2013-05-16 17:22:47]

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:41: Color textColor =
style->visitedDependentColor(CSSPropertyWebkitTextFillColor);
On 2013/05/16 17:03:14, Stephen Chenney wrote:
> Rename this "defaultTextColor".

Changed to "styleTextColor" to be clear that this arises from the style, not
some global default.

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:45: Color actualTextColor = textColor;
On 2013/05/16 17:03:14, Stephen Chenney wrote:
> Then name this "textColor".

Done.

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:76: if (shadow->next() || !opaque) {
On 2013/05/16 17:03:14, Stephen Chenney wrote:
> What the abstract meaning of this? I would prefer a variable "needToDrawAgain"
> or something, describing why you need to take a different path. It also avoids
> duplicating the test further down, which is a recipe for bugs if someone ever
> makes changes. You can even use it as the while condition.

I've tried to mirror paintTextWithShadows from InlineTextBox.cpp as closely as
possible in order to capture the current behaviour there here. I'm not a huge
fan of the mysterious condition either.

The reason I didn't clean this up here (or in InlineTextBox) is because I have
another CL following this one that does a more dramatic refactor of this code
which would eliminate this condition anyhow.

https://codereview.chromium.org/15137009/diff/18001/Source/core/rendering/Ell...

Would you prefer I come up with some abstract name for this condition in the
meantime, or leave it as-is?

[Stephen Chennney, 2013-05-16 17:34:55]

LGTM.

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
File Source/core/rendering/EllipsisBox.cpp (right):

https://codereview.chromium.org/15001034/diff/14001/Source/core/rendering/Ell...
Source/core/rendering/EllipsisBox.cpp:76: if (shadow->next() || !opaque) {
On 2013/05/16 17:22:47, jbroman wrote:
> On 2013/05/16 17:03:14, Stephen Chenney wrote:
> > What the abstract meaning of this? I would prefer a variable
"needToDrawAgain"
> > or something, describing why you need to take a different path. It also
avoids
> > duplicating the test further down, which is a recipe for bugs if someone
ever
> > makes changes. You can even use it as the while condition.
> 
> I've tried to mirror paintTextWithShadows from InlineTextBox.cpp as closely as
> possible in order to capture the current behaviour there here. I'm not a huge
> fan of the mysterious condition either.
> 
> The reason I didn't clean this up here (or in InlineTextBox) is because I have
> another CL following this one that does a more dramatic refactor of this code
> which would eliminate this condition anyhow.
> 
>
https://codereview.chromium.org/15137009/diff/18001/Source/core/rendering/Ell...
> 
> Would you prefer I come up with some abstract name for this condition in the
> meantime, or leave it as-is?

Now that I've seen the other patch, it's fine to leave this as is.

[commit-bot: I haz the power, 2013-05-16 18:24:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/15001034/17001

[commit-bot: I haz the power, 2013-05-16 19:38:33]

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

[commit-bot: I haz the power, 2013-05-16 19:43:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbroman@chromium.org/15001034/17001

[commit-bot: I haz the power, 2013-05-16 20:20:41]

Change committed as 150534