Comment on attachment 75408[details]
Part 1: CSS support
r=me, but can you go ahead and add a RenderStyle::diff hint as well? I think it's going to end up being a layout hint (because of possible changes to overflow propagation, e.g., since emphasis marks can go above Rubies).
Comment on attachment 75408[details]
Part 1: CSS support
View in context: https://bugs.webkit.org/attachment.cgi?id=75408&action=review> WebCore/css/CSSComputedStyleDeclaration.cpp:1237
> + }
> + }
I think it would be slightly better to explicitly list all the cases so we get a warning if we ever add a new case. If you want to put the default code outside the switch and use "break" we’d get everything except for the runtime check for a bad enum value, which I think is a good tradeoff.
> WebCore/css/CSSParser.cpp:5147
> + if (fill && shape) {
> + RefPtr<CSSValueList> parsedValues = CSSValueList::createSpaceSeparated();
> + parsedValues->append(fill.release());
> + parsedValues->append(shape.release());
> + addProperty(CSSPropertyWebkitTextEmphasisStyle, parsedValues.release(), important);
> + } else if (fill)
> + addProperty(CSSPropertyWebkitTextEmphasisStyle, fill.release(), important);
> + else if (shape)
> + addProperty(CSSPropertyWebkitTextEmphasisStyle, shape.release(), important);
> + else
> + return false;
> +
> + return true;
I think this would read better if the return true was inside each if and we thus did not need all those elses.
Created attachment 76439[details]
Part 2: Platform, layout and rendering (WIP, do not review)
Trying to appease Qt, correcting emphasis mark positioning in vertical fonts.
Comment on attachment 76712[details]
Part 3: Platform support
View in context: https://bugs.webkit.org/attachment.cgi?id=76712&action=review> WebCore/platform/graphics/FontFastPath.cpp:200
> +// FIXME: This method is not complex-text aware.
Anthropomorphizing the function and saying it’s not aware of complex text doesn’t make clear to me what the problem is here. Is it incorrect in some way? For what cases? I think you can probably write a slightly different comment that will be less ambiguous.
Also, I’d prefer you use the C++ term function rather than the term method.
> WebCore/platform/graphics/SimpleFontData.cpp:200
> + m_derivedFontData = new DerivedFontData(isCustomFont());
Can I talk you into future-proofing this code by calling adoptPtr in all the call sites like this one? In a related question, should we use the create function idiom for this struct?
> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:85
> + return new SimpleFontData(
> + FontPlatformData(cairo_scaled_font_get_font_face(m_platformData.scaledFont()),
I would have made that first argument be on the first line because of my distaste for breaking right after an open paren. I think the line length ends up being not too horrible.
> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:96
> + m_derivedFontData->smallCaps = scaledFontData(fontDescription, .7);
We should really name that 0.7 constant at some point. I know it wasn’t named before.
> WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:106
> + m_derivedFontData->emphasisMark = scaledFontData(fontDescription, .5);
And this 0.5 constant.
Comment on attachment 76852[details]
Part 4: Layout and rendering
View in context: https://bugs.webkit.org/attachment.cgi?id=76852&action=review
r=me!
> WebCore/rendering/InlineFlowBox.cpp:754
> + if (style->textEmphasisMark() != TextEmphasisMarkNone) {
> + int emphasisMarkHeight = style->font().emphasisMarkHeight(style->textEmphasisMarkString());
> + if (style->textEmphasisPosition() == TextEmphasisPositionOver)
> + topGlyphOverflow = min(topGlyphOverflow, -emphasisMarkHeight);
> + else
> + bottomGlyphOverflow = max(bottomGlyphOverflow, emphasisMarkHeight);
> + }
It would be great to have a repaint test that tests the overflow repainting.
2010-12-02 12:58 PST, mitz
2010-12-12 13:30 PST, mitz
2010-12-12 14:04 PST, mitz
2010-12-12 15:06 PST, mitz
2010-12-12 15:08 PST, mitz
2010-12-12 18:56 PST, mitz
2010-12-13 14:25 PST, mitz
2010-12-13 18:09 PST, mitz
2010-12-13 18:38 PST, mitz
2010-12-14 19:21 PST, mitz
2010-12-14 19:28 PST, mitz
2010-12-15 16:57 PST, mitz
2010-12-16 13:50 PST, mitz
2010-12-17 00:11 PST, mitz