Bug 133198

Summary: SVG TextRuns do not always get RenderingContexts
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dino, enrica, hyatt, jonlee, mitz, rniwa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 133338    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 none

Description Myles C. Maxfield 2014-05-22 19:40:09 PDT
SVG TextRuns do not always get RenderingContexts
Comment 1 Myles C. Maxfield 2014-05-22 19:41:16 PDT
Created attachment 231931 [details]
Patch
Comment 2 Myles C. Maxfield 2014-05-22 19:53:30 PDT
<rdar://problem/16433531>
Comment 3 Darin Adler 2014-05-26 09:18:39 PDT
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.
Comment 4 Myles C. Maxfield 2014-05-27 13:13:49 PDT
Created attachment 232149 [details]
Patch
Comment 5 Simon Fraser (smfr) 2014-05-27 13:15:59 PDT
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.
Comment 6 Myles C. Maxfield 2014-05-27 14:29:22 PDT
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.
Comment 7 Myles C. Maxfield 2014-05-27 14:35:55 PDT
http://trac.webkit.org/changeset/169400
Comment 8 Myles C. Maxfield 2014-05-27 14:39:51 PDT
Addressed Simon's comment in http://trac.webkit.org/changeset/169401
Comment 9 WebKit Commit Bot 2014-05-27 22:05:25 PDT
Re-opened since this is blocked by bug 133338
Comment 10 Myles C. Maxfield 2014-06-03 17:39:17 PDT
Created attachment 232453 [details]
Patch
Comment 11 Myles C. Maxfield 2014-06-03 17:40:53 PDT
Simon already reviewed most of this. The new pieces that need a review are the following files:

PlatformLocale.h
LocalizedDateCache.*
LocaleMac.h
RenderThemeIOS.mm
Comment 12 Myles C. Maxfield 2014-06-03 17:41:20 PDT
dino: You would probably be a good person to review this.
Comment 13 Myles C. Maxfield 2014-06-03 17:42:49 PDT
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)
Comment 14 Myles C. Maxfield 2014-06-03 18:52:10 PDT
Created attachment 232458 [details]
Patch
Comment 15 Build Bot 2014-06-03 19:58:42 PDT
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
Comment 16 Build Bot 2014-06-03 19:58:45 PDT
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
Comment 17 Simon Fraser (smfr) 2014-06-03 21:37:58 PDT
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?
Comment 18 Myles C. Maxfield 2014-06-04 13:20:14 PDT
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.
Comment 19 Myles C. Maxfield 2014-06-04 13:23:17 PDT
http://trac.webkit.org/changeset/169591