Summary: | SVG TextRuns do not always get RenderingContexts | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||
Component: | New Bugs | Assignee: | 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
Myles C. Maxfield
2014-05-22 19:40:09 PDT
Created attachment 231931 [details]
Patch
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. Created attachment 232149 [details]
Patch
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 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. Addressed Simon's comment in http://trac.webkit.org/changeset/169401 Re-opened since this is blocked by bug 133338 Created attachment 232453 [details]
Patch
Simon already reviewed most of this. The new pieces that need a review are the following files: PlatformLocale.h LocalizedDateCache.* LocaleMac.h RenderThemeIOS.mm dino: You would probably be a good person to review this. 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) Created attachment 232458 [details]
Patch
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 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 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 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. |