Add support for multiple text decorations with same line positioning

Add support for multiple text decorations with same line positioning

Add support for multiple propagated text decorations with the same line
positioning, as defined in http://www.w3.org/TR/css-text-decor-3/.
Before this, elements with multiple text decorations but the same line
positioning (e.g. two underlines) would only paint the 'topmost' one.
With this patch, both lines are painted.

This is important for CSS Text Decorations Module 3, where two text
decorations can have the same line position but different styles (e.g.
wavy and solid), and they should be painted on top of eachother.

This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted.

Also cleaned up the existing logic for text decorations to only inherit
through StyleAdjuster, removing the need to traverse up the tree to
resolve colors for inherited text decorations.

BUG=462142

[sashab, 2015-09-17 01:21:25]

sashab@chromium.org changed reviewers:
+ timloh@chromium.org

[sashab, 2015-09-17 01:21:25]

I'm WIP fixing the failing tests... How do you feel about the overall
implementation so far? The challenge comes from the fact that we don't just
resolve the color at style time.

I'm considering landing some patches to fix this -- moving color resolution to
style time (as suggested in crbug.com/462142) and getting rid of
LayoutObject::getTextDecorations() to do everything in StyleAdjuster, for
example -- which would make implementing this patch and debugging these failing
tests much easier.

Sasha

[Timothy Loh, 2015-09-17 01:24:52]

On 2015/09/17 01:21:25, sashab wrote:
> I'm WIP fixing the failing tests... How do you feel about the overall
> implementation so far? The challenge comes from the fact that we don't just
> resolve the color at style time.
> 
> I'm considering landing some patches to fix this -- moving color resolution to
> style time (as suggested in crbug.com/462142) and getting rid of
> LayoutObject::getTextDecorations() to do everything in StyleAdjuster, for
> example -- which would make implementing this patch and debugging these
failing
> tests much easier.
> 
> Sasha

If this makes it easier, then do it first. samahto starting this a few months
ago (https://codereview.chromium.org/950623002/) but it was never finished.

[sashab, 2015-09-21 02:09:40]

Ok tim, I think it's ready for you to take another look.

Hopefully it's clear from the code, but here's the logic I'm using to calculate
the text decoration color:
1. The color when the decoration was set, otherwise if this was currentColor:
2. The color of the closest <a> or <font> tag, otherwise:
3. The currentColor when the decoration was set
Don't think this is actually specified in the spec but is the current logic that
Chrome uses.

Some notes:
- I aimed to removed LayoutObject::getTextDecorations which traverses up the
tree to get colors. Now everything is in StyleAdjuster.
- I've had to add 4 colors to rareInheritedData... I think this is the minimum
(colors of the propagated color font/a tag, and colors of a simple underline
when that optimization is used). Not sure if I need to store both the
visited/non-visited colors, maybe you have a better idea of what they mean so
can advise

The trybots are failing but the diffs are nonsensical. The tests need
rebaselining -- I think the main difference is SVGs now paint multiple times for
multiple inherited text decorations, for example, which is the correct behavior.

I also accidentally fixed the inherited multiple decorations bug by cleaning
this code up, so I went back and added the tests it had originally which all
pass.

Thanks,

Sasha

[Timothy Loh, 2015-09-21 07:58:19]

On 2015/09/21 02:09:40, sashab wrote:
> Ok tim, I think it's ready for you to take another look.
> 
> Hopefully it's clear from the code, but here's the logic I'm using to
calculate
> the text decoration color:
> 1. The color when the decoration was set, otherwise if this was currentColor:
> 2. The color of the closest <a> or <font> tag, otherwise:
> 3. The currentColor when the decoration was set
> Don't think this is actually specified in the spec but is the current logic
that
> Chrome uses.
> 
> Some notes:
> - I aimed to removed LayoutObject::getTextDecorations which traverses up the
> tree to get colors. Now everything is in StyleAdjuster.
> - I've had to add 4 colors to rareInheritedData... I think this is the minimum
> (colors of the propagated color font/a tag, and colors of a simple underline
> when that optimization is used). Not sure if I need to store both the
> visited/non-visited colors, maybe you have a better idea of what they mean so
> can advise
> 
> The trybots are failing but the diffs are nonsensical. The tests need
> rebaselining -- I think the main difference is SVGs now paint multiple times
for
> multiple inherited text decorations, for example, which is the correct
behavior.
> 
> I also accidentally fixed the inherited multiple decorations bug by cleaning
> this code up, so I went back and added the tests it had originally which all
> pass.
> 
> Thanks,
> 
> Sasha

I think you're misreading the code. It looks like the current behaviour is to
use the color (with currentColor resolved) where a decoration is set, where
<a>/<font> elements will override this as it propagates down.

[sashab, 2015-09-22 03:01:34]

It's not that I was misreading the code... But you're right, we can actually get
away with not storing the font/a colors (see latest patch). We still need to
store the simple underline color though, since we need to know its original
color (and we need to change its original color if we pass a font/a element). Ie
we need to handle both of these cases:

<u><span style="color: red;">Text</span></u>
(red text, black underline)

<u><font color="red">Text</font></u>
(red text, red underline)

I'll also go back through and update the tests to ref html tests using absolute
positioning, which we discussed over lunch and I thought was a clever idea :)

[Timothy Loh, 2015-09-22 05:00:23]

I don't get the isAffectByPropagatedColors thing. Also I think we shouldn't add
more colors to the ComputedStyle data, we can just resolve the simple underline
into AppliedTextDecorations if an element's color is different to its parent's.

https://codereview.chromium.org/1328283005/diff/210001/Source/core/paint/Inli...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1328283005/diff/210001/Source/core/paint/Inli...
Source/core/paint/InlineTextBoxPainter.cpp:717: void
InlineTextBoxPainter::paintDecoration(const PaintInfo& paintInfo, const
LayoutPoint& boxOrigin, TextDecoration deco)
Does this patch remove our usage of TextDecoration as an enum bitset?

[Timothy Loh, 2015-09-22 07:19:28]

FYI the <font> behaviour of overriding text decoration colors is in the HTML
spec, while the <a> behaviour is non-standard.

https://html.spec.whatwg.org/multipage/rendering.html#phrasing-content-3

[sashab, 2015-09-24 01:19:27]

I've removed isAffectByPropagatedColors(), but have a look at
text-decoration-style-inherit-links.html.

<div style="text-decoration: underline wavy green;">
  <a href="">Text</a>
</div>

This now appears with a wavy *purple* underline, because <a> tags override
the text decoration colors. With isAffectByPropagatedColors(), we can prevent
overriding for text decorations that have the color explicitly specified.

Up to you; this isn't spec-driven behavior so maybe we should leave it
as-is. Won't match firefox though, but then again neither does any of
this font color stuff (they differentiate between colors set with
<font color='foo'> and <font style='color:foo'> - when using color='foo',
it matches our behavior).

Still todo: update the tests to use expected.htmls.

https://codereview.chromium.org/1328283005/diff/210001/Source/core/paint/Inli...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1328283005/diff/210001/Source/core/paint/Inli...
Source/core/paint/InlineTextBoxPainter.cpp:717: void
InlineTextBoxPainter::paintDecoration(const PaintInfo& paintInfo, const
LayoutPoint& boxOrigin, TextDecoration deco)
On 2015/09/22 05:00:23, Timothy Loh wrote:
> Does this patch remove our usage of TextDecoration as an enum bitset?

No, SVGInlineTextBoxPainter and InlineTextBoxPainter still check
decoration.lines() & TextDecorationUnderline etc to see if the line is included
in the decoration. Maybe I don't understand your question. Are you asking me to
remove all callsites that use bit operations to check the applied decoration
lines? Maybe use decoration.hasUnderline() etc instead?

[Timothy Loh, 2015-09-24 01:38:16]

I think I don't exactly get the <a>/<font> thing, could you clarify *precisely*
what our behaviour is now, and what changes this patch makes to it?

https://codereview.chromium.org/1328283005/diff/210001/Source/core/paint/Inli...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1328283005/diff/210001/Source/core/paint/Inli...
Source/core/paint/InlineTextBoxPainter.cpp:717: void
InlineTextBoxPainter::paintDecoration(const PaintInfo& paintInfo, const
LayoutPoint& boxOrigin, TextDecoration deco)
On 2015/09/24 01:19:27, sashab wrote:
> On 2015/09/22 05:00:23, Timothy Loh wrote:
> > Does this patch remove our usage of TextDecoration as an enum bitset?
> 
> No, SVGInlineTextBoxPainter and InlineTextBoxPainter still check
> decoration.lines() & TextDecorationUnderline etc to see if the line is
included
> in the decoration. Maybe I don't understand your question. Are you asking me
to
> remove all callsites that use bit operations to check the applied decoration
> lines? Maybe use decoration.hasUnderline() etc instead?

Are the TextDecoration argument here and the textDecorationsInEffect methods no
longer needed?

[sashab, 2015-09-24 02:49:03]

To answer your first question: the current behavior is that <a> and <font> tags
override the color of all *existing* text decorations (text decorations
specified by a parent element) to be the color of the <a>/<font> tag, even if
that text decoration was specified with a certain color. For example, <span
style="text-decoration: underline; color: green"><font
color="orange">Test</font></span> will appear as orange text with an orange
underline. Similarly, "text-decoration: green underline" with the experimental
flag enabled will also appear as orange text with an orange underline.

Our behavior with <font color="..."> matches the behavior in Firefox. However,
Firefox does not override colors when specified with <font style="color: ...">,
nor do they override colors for <a> tags.

As for your second question, ComputedStyle::textDecorationsInEffect() is still
used in places (e.g. ComputedStyleCSSValueMapping for
CSSPropertyWebkitTextDecorationsInEffect), and in LayoutTextCombine::updateFont.
If you like, I could try and remove all callsites by ensuring that decorations
are only added to appliedTextDecorations if their style is not
TextDecorationNone. That way, you could use appliedTextDecorations.size() to
check if any text decorations are visible, and we could get rid of
textDecorationsInEffect().

[sashab, 2015-09-29 02:52:53]

Ping tim

[sashab, 2015-09-30 01:50:54]

So it turns out that using expected.html files for these tests is actually
harder than it sounds. For single-line text its fine, but on some small displays
(other OSs?) when the text wraps there's no way to mimic the overlay behavior of
text decorations with divs (first line decorations go underneath the second
line, etc). Also getting the text to only appear once is hard.

Suggestions:
1) Use pixel tests
2) Take all the text out of the tests (put them in HTML comments instead?) and
just apply the decorations to non-breaking spaces, which will never wrap more
than 1 line
3) Just add expected html files, and if the text wraps multiple lines, update
that test to have a shorter line

[Timothy Loh, 2015-10-01 01:51:44]

Please update the patch description to actually explain what's changing in the
patch. Also we already support multiple text decorations, it should be more
clear that the patch is about multiple text decorations of the positioning.

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/LayoutTests/TestExpectations:2080: crbug.com/462142
svg/batik/text/textDecoration2.svg [ NeedsRebaseline ]
Please upload at least one new baseline for each of these so we can see what
actually changed in these.

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:115: ||
isHTMLRTElement(e);
Is this a separate bug fix? If so, put it in a separate patch...

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:753: for (const
auto& decoration : decorations) {
let's write out the type here and everywhere else so it's easier to read

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1395: void
ComputedStyle::applyTextDecorations(const ComputedStyle& parentStyle, bool
overrideExistingColors)
just pass in the parent style's decoration color

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1402: if
(inherited_flags.m_hasSimpleUnderline &&
visitedDependentColor(CSSPropertyTextDecorationColor) != parentDecorationColor)
{
can we not copy-paste visitedDependentColor(CSSPropertyTextDecorationColor) so
many times?

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1422: // Clear the
existing optimization if we have it.
The logic here is a bit complicated, can we just only clear the flag once at the
start of the function?

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
File third_party/WebKit/Source/core/style/StyleRareInheritedData.h (left):

https://chromiumcodereview.appspot.com/1328283005/diff/250001/third_party/Web...
third_party/WebKit/Source/core/style/StyleRareInheritedData.h:136: unsigned
m_textUnderlinePosition : 1; // TextUnderlinePosition
Why did this change?

[sashab, 2015-10-02 02:45:42]

Thanks tim; responded to all comments.

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2080: crbug.com/462142
svg/batik/text/textDecoration2.svg [ NeedsRebaseline ]
On 2015/10/01 01:51:43, Timothy Loh wrote:
> Please upload at least one new baseline for each of these so we can see what
> actually changed in these.

Done.

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:115: ||
isHTMLRTElement(e);
On 2015/10/01 01:51:43, Timothy Loh wrote:
> Is this a separate bug fix? If so, put it in a separate patch...

No? Both code paths now use this function, so it needs to stop text decorations
from propagating through RT elements... This logic was previously in
LayoutObject::getTextDecorations...

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:753: for (const
auto& decoration : decorations) {
On 2015/10/01 01:51:43, Timothy Loh wrote:
> let's write out the type here and everywhere else so it's easier to read

Done.

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1395: void
ComputedStyle::applyTextDecorations(const ComputedStyle& parentStyle, bool
overrideExistingColors)
On 2015/10/01 01:51:43, Timothy Loh wrote:
> just pass in the parent style's decoration color

Done.

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1402: if
(inherited_flags.m_hasSimpleUnderline &&
visitedDependentColor(CSSPropertyTextDecorationColor) != parentDecorationColor)
{
On 2015/10/01 01:51:43, Timothy Loh wrote:
> can we not copy-paste visitedDependentColor(CSSPropertyTextDecorationColor) so
> many times?

Done.

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1422: // Clear the
existing optimization if we have it.
On 2015/10/01 01:51:43, Timothy Loh wrote:
> The logic here is a bit complicated, can we just only clear the flag once at
the
> start of the function?

Simplified it a bit.

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/StyleRareInheritedData.h (left):

https://codereview.chromium.org/1328283005/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/StyleRareInheritedData.h:136: unsigned
m_textUnderlinePosition : 1; // TextUnderlinePosition
On 2015/10/01 01:51:43, Timothy Loh wrote:
> Why did this change?

Mistake, sorry

[Timothy Loh, 2015-10-02 05:40:21]

What's the change on the tests that need rebaseline and why did they change? I
can't tell from the images.

[sashab, 2015-10-02 06:03:57]

The changes in the images are *rreeally* subtle (I've uploaded an image for
every test that needs rebaselining). They barely changed at all. The only reason
they need rebaselining is because they all have text decorations that were
declared twice (and are now being drawn twice) so they appear slightly harder/in
a slightly darker color than before. If you pixel diff them you'll see the small
changes, but they are basically not different at all.

[Timothy Loh, 2015-10-12 00:11:16]

Some comments on the patch notes:
- Be clearer on the test changes, "This changes some existing text decoration
behavior" isn't very useful.
- I'm not sure why the description says the behaviour is behind a flag, isn't
this visible in the default chrome configuration?
- Please be explicit about the behaviour changes! We already support having
multiple text decorations on an element, the patch is about multiple decorations
with the same line positioning.

Do we need to restrict the underline optimization to where text-decoration-color
is set to currentColor? I think it should work without checking the color.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:118: // Certain
elements (<a>, <font>) bind strongly to text decoration colors.
This sentence doesn't make sense to me.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1248: if
(!inherited_flags.m_hasSimpleUnderline &&
!rareInheritedData->appliedTextDecorations)
can we move this if statement below the other one so we only have a single
check?

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1383: void
ComputedStyle::updateAppliedTextDecorations(Color propagatedColor)
"update" is really vague. How about overrideTextDecorationColors?

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1397: // If we have no
decorations, return early.
This comment doesn't add value.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1408: // If we have a
propagated color, update any other affected decorations.
Don't think this comment helps (or makes sense)

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1426: Color
resolvedDecorationColor = decorationColor.resolve(currentTextDecorationColor);
isn't this exactly the same as currentTextDecorationColor?

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:1386: 
unrelated blank line?

[sashab, 2015-10-12 23:11:02]

Thanks Tim, had a shot at rewriting the patch description. PTAL.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:118: // Certain
elements (<a>, <font>) bind strongly to text decoration colors.
On 2015/10/12 00:11:16, Timothy Loh wrote:
> This sentence doesn't make sense to me.

I was trying to explain the weird behavior in a clever way. Turns out I was too
clever by half ;) Changed to say 'override', this probably makes more sense now
but isn't super informative. Suggestions welcome or if it's still not great I'll
keep thinking of a way to describe it.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1248: if
(!inherited_flags.m_hasSimpleUnderline &&
!rareInheritedData->appliedTextDecorations)
On 2015/10/12 00:11:16, Timothy Loh wrote:
> can we move this if statement below the other one so we only have a single
> check?

Yeahhh.. It's a bit weird because applideTextDecorations() below uses a similar
pattern, and I feel they should match. OK, changed both.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1383: void
ComputedStyle::updateAppliedTextDecorations(Color propagatedColor)
On 2015/10/12 00:11:16, Timothy Loh wrote:
> "update" is really vague. How about overrideTextDecorationColors?

Hey, good one. Yup, this name is an artifact from an older patch. Way better
now.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1397: // If we have no
decorations, return early.
On 2015/10/12 00:11:16, Timothy Loh wrote:
> This comment doesn't add value.

Removed!

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1408: // If we have a
propagated color, update any other affected decorations.
On 2015/10/12 00:11:16, Timothy Loh wrote:
> Don't think this comment helps (or makes sense)

Yup, 'propagated' doesn't belong here. Removed!

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1426: Color
resolvedDecorationColor = decorationColor.resolve(currentTextDecorationColor);
On 2015/10/12 00:11:16, Timothy Loh wrote:
> isn't this exactly the same as currentTextDecorationColor?

Oh yeah, you're right :) Done.

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1328283005/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:1386: 
On 2015/10/12 00:11:16, Timothy Loh wrote:
> unrelated blank line?

Whoops, how embarrassing ^_^; I swear I checked this whole patch...

[sashab, 2015-10-18 22:54:13]

Ping tim, should be OK now

[sashab, 2015-11-03 03:28:07]

Timloh pls review

[Timothy Loh, 2015-11-03 06:38:59]

lgtm with some nits

https://codereview.chromium.org/1328283005/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1328283005/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1418: for (size_t i = 0;
i < rareInheritedData->appliedTextDecorations->size(); ++i)
rareInheritedData->appliedTextDecorations should just be list

https://codereview.chromium.org/1328283005/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1445: if
(!rareInheritedData->appliedTextDecorations &&
!inherited_flags.m_hasSimpleUnderline && isSimpleUnderline) {
m_hasSimpleUnderline is always false here

[sashab, 2015-11-03 22:51:26]

https://codereview.chromium.org/1328283005/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1328283005/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1418: for (size_t i = 0;
i < rareInheritedData->appliedTextDecorations->size(); ++i)
On 2015/11/03 06:38:59, Timothy Loh wrote:
> rareInheritedData->appliedTextDecorations should just be list

Done.

https://codereview.chromium.org/1328283005/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:1445: if
(!rareInheritedData->appliedTextDecorations &&
!inherited_flags.m_hasSimpleUnderline && isSimpleUnderline) {
On 2015/11/03 06:38:59, Timothy Loh wrote:
> m_hasSimpleUnderline is always false here

Done.

[sashab, 2015-11-03 22:52:04]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-11-03 22:52:04]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1328283005/#ps390001
(title: "Review feedback")

[commit-bot: I haz the power, 2015-11-03 22:52:43]

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

[commit-bot: I haz the power, 2015-11-04 00:57:35]

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

[commit-bot: I haz the power, 2015-11-04 00:57:36]

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

[Timothy Loh, 2016-01-04 06:45:38]

timloh@chromium.org changed reviewers:
+ rune@opera.com

[Timothy Loh, 2016-01-04 06:45:39]

BTW we might eventually need to move propagation out of the StyleAdjuster
because it doesn't correctly handle anonymous boxes, but I don't think this will
make that harder to fix. More context at
https://bugzilla.mozilla.org/show_bug.cgi?id=1222703

[drott, 2016-11-14 15:47:03]

As agreed with Sasha, I'm taking this one. Rebased and adapted in
https://codereview.chromium.org/2497963002