Bug 133198 – SVG TextRuns do not always get RenderingContexts

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

SVG TextRuns do not always get RenderingContexts

[Myles C. Maxfield]

Created attachment 231931 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/16433531>

[Darin Adler]

Comment on attachment 231931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231931&action=review

> Source/WebCore/ChangeLog:24
> +        RenderListMarker and RenderMenuList are subclasses of RenderBox, but the
> +        function that properly creates a TextRun is in RenderBlock. Rather than
> +        call functions in a sibling class, this patch moves this construction
> +        function from RenderBlock to RenderBox, the shared base class. It also
> +        updates all the call sites of this function. This patch then modifies
> +        RenderListMarker and RenderMenuList to use this newly-moved construction
> +        function.

These are static member functions; it doesn’t matter what class they are in. I suggest separating the “move functions to RenderBox” from the rest of this patch; it makes the patch huge and is entirely mechanical. And in fact is unnecessary, but an OK thing to do either before or after the work.

[Myles C. Maxfield]

Created attachment 232149 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 232149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232149&action=review

> Source/WebCore/ChangeLog:3
> +        SVG TextRuns do not always get RenderingContexts

Would be better for the title to describe the user-visible impact of the bug.

[Myles C. Maxfield]

Comment on attachment 232149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232149&action=review

> Source/WebCore/rendering/RenderListMarker.cpp:-1850
> -            int itemWidth = font.width(m_text);

Whoops, stupid mistake.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/169400

[Myles C. Maxfield]

Addressed Simon's comment in http://trac.webkit.org/changeset/169401

[WebKit Commit Bot]

Re-opened since this is blocked by bug 133338

[Myles C. Maxfield]

Created attachment 232453 [details]
Patch

[Myles C. Maxfield]

Simon already reviewed most of this. The new pieces that need a review are the following files:

PlatformLocale.h
LocalizedDateCache.*
LocaleMac.h
RenderThemeIOS.mm

[Myles C. Maxfield]

dino: You would probably be a good person to review this.

[Myles C. Maxfield]

Also note that I don't have expected test results because the iOS testing infrastructure isn't operational right now. I thought I would post the patch anyway. I will add expected iOS test results once I am able. (I have verified that the fix works by stepping through code in a debugger)

[Myles C. Maxfield]

Created attachment 232458 [details]
Patch

[Build Bot]

Comment on attachment 232458 [details]
Patch

Attachment 232458 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5643460241522688

New failing tests:
media/W3C/video/src/src_reflects_attribute_not_source_elements.html

[Build Bot]

Created attachment 232461 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Comment on attachment 232458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232458&action=review

> Source/WebCore/rendering/RenderThemeIOS.mm:573
> +    float maximumWidth = font.isSVGFont() ? 0 : inputElement->locale().maximumWidthForDateType(dateType, font);

This deserves a FIXME comment referencing the bug you'll use to make things work with SVG Fonts.

> LayoutTests/ChangeLog:12
> +        * platform/ios-sim/fonts/resources/graffiti.svg: Added. SVG font for above test.

Don't we already have an SVG font somewhere in LayoutTests?

[Myles C. Maxfield]

Comment on attachment 232458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232458&action=review

>> Source/WebCore/rendering/RenderThemeIOS.mm:573
>> +    float maximumWidth = font.isSVGFont() ? 0 : inputElement->locale().maximumWidthForDateType(dateType, font);
> 
> This deserves a FIXME comment referencing the bug you'll use to make things work with SVG Fonts.

Done.

>> LayoutTests/ChangeLog:12
>> +        * platform/ios-sim/fonts/resources/graffiti.svg: Added. SVG font for above test.
> 
> Don't we already have an SVG font somewhere in LayoutTests?

We do, but it's in LayoutTests/svg/custom/resources/graffiti.svg, far away from platform/ios-sim.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/169591