|
Description
Dave Hyatt
2010-10-28 11:47:53 PDT
Created attachment 75408 [details]
Part 1: CSS support
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).
Actually only changing the color is just a repaint hint I suppose, so just do the right thing with each property. :) 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. Comment on attachment 75408 [details] Part 1: CSS support <http://trac.webkit.org/projects/webkit/changeset/73219> Created attachment 76333 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Uploading this primarily for the EWS to look at.
Attachment 76333 [details] did not build on qt: Build output: http://queues.webkit.org/results/6912088 Attachment 76333 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6898081 Created attachment 76334 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Attachment 76333 [details] did not build on win: Build output: http://queues.webkit.org/results/6982088 Attachment 76334 [details] did not build on qt: Build output: http://queues.webkit.org/results/6934089 Attachment 76334 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6967083 Attachment 76334 [details] did not build on win: Build output: http://queues.webkit.org/results/6990072 Created attachment 76336 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Created attachment 76337 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
I can probably move the implementation of smallCapsFontData() and emphasisMarkFontData() to cross-platform code. Attachment 76336 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6909083 Attachment 76336 [details] did not build on qt: Build output: http://queues.webkit.org/results/6958074 Attachment 76337 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7031034 Attachment 76336 [details] did not build on win: Build output: http://queues.webkit.org/results/6996081 Attachment 76337 [details] did not build on qt: Build output: http://queues.webkit.org/results/7032036 Attachment 76337 [details] did not build on win: Build output: http://queues.webkit.org/results/7019077 Attachment 76337 [details] did not build on win: Build output: http://queues.webkit.org/results/6907085 Created attachment 76345 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Attachment 76345 [details] did not build on qt: Build output: http://queues.webkit.org/results/6969087 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.
Attachment 76439 [details] did not build on qt: Build output: http://queues.webkit.org/results/6963114 Created attachment 76477 [details]
Part 2: Platform, layout and rendering (WIP, do not review)
Created attachment 76481 [details]
Platform, layout and rendering (WIP, do not review)
Created attachment 76617 [details]
Part 2: Additional RenderStyle support
Another small piece, ready for review.
Created attachment 76619 [details]
Part 2: Additional RenderStyle support
Comment on attachment 76619 [details] Part 2: Additional RenderStyle support <http://trac.webkit.org/changeset/74095> Created attachment 76712 [details]
Part 3: Platform support
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 76712 [details] Part 3: Platform support <http://trac.webkit.org/projects/webkit/changeset/74169> Created attachment 76813 [details]
Part 4: Layout and rendering
I am going to file a follow-up bug for the interaction with ruby
Comment on attachment 76813 [details]
Part 4: Layout and rendering
Hyatt suggested that I try do this the right way instead :)
http://trac.webkit.org/changeset/74169 might have broken Leopard Intel Debug (Tests) Created attachment 76852 [details]
Part 4: Layout and rendering
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. Fixed in <http://trac.webkit.org/changeset/74281> Are platform-specific tests required on Windows? http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html (In reply to comment #43) > Are platform-specific tests required on Windows? > > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html Yes (In reply to comment #44) > (In reply to comment #43) > > Are platform-specific tests required on Windows? > > > > http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74283%20(7526)/results.html > > Yes Are you adding them or should I? Landed Windows-specific results in http://trac.webkit.org/changeset/74290 |