Bug 136769 – Allow targetting the SVG->OTF font converter with ENABLE(SVG_OTF_CONVERTER)

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

[Myles C. Maxfield]

Allow targetting the SVG->OTF font converter with ENABLE(SVG_OTF_CONVERTER)

[Myles C. Maxfield]

Created attachment 238008 [details]
Patch

[Myles C. Maxfield]

Created attachment 238634 [details]
Patch

[Chris Dumez]

Comment on attachment 238634 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:177
> +                SVGFontElement* fontElement = downcast<SVGFontElement>(m_svgFontFaceElement->parentNode());

nit: I have been porting the code to use this style now:
SVGFontElement& fontElement = downcast<SVGFontElement>(*m_svgFontFaceElement->parentNode());

> Source/WebCore/loader/cache/CachedFont.cpp:99
> +bool CachedFont::ensureCustomFontData(bool svgFont, String svgURI)

nit: const String& ?

> Source/WebCore/loader/cache/CachedFont.cpp:110
> +            auto externalSVGDocument = SVGDocument::create(nullptr, URL());

I am new to using "auto" but are we sure this will actually be a RefPtr here? (and not a PassRefPtr)

[Build Bot]

Comment on attachment 238634 [details]
Patch

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

New failing tests:
svg/W3C-SVG-1.1-SE/color-prop-05-t.svg
svg/W3C-SVG-1.1-SE/coords-dom-01-f.svg
svg/W3C-SVG-1.1-SE/styling-pres-02-f.svg
svg/W3C-SVG-1.1-SE/interact-pointer-03-t.svg
svg/W3C-SVG-1.1/fonts-elem-05-t.svg
svg/W3C-SVG-1.1/fonts-elem-06-t.svg
svg/W3C-SVG-1.1-SE/styling-css-04-f.svg
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
svg/W3C-SVG-1.1/fonts-kern-01-t.svg
svg/W3C-SVG-1.1/filters-turb-02-f.svg
svg/W3C-SVG-1.1-SE/struct-use-14-f.svg
svg/W3C-SVG-1.1/fonts-glyph-04-t.svg
svg/W3C-SVG-1.1-SE/pservers-pattern-04-f.svg
svg/W3C-SVG-1.1/filters-light-04-f.svg
svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
svg/W3C-SVG-1.1/fonts-glyph-02-t.svg
svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg
svg/W3C-SVG-1.1/fonts-desc-02-t.svg
svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-04-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-03-f.svg
svg/W3C-SVG-1.1-SE/filters-image-03-f.svg
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
fast/css/ex-unit-with-no-x-height.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg
svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg
svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg

[Build Bot]

Created attachment 238654 [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

[Build Bot]

Comment on attachment 238634 [details]
Patch

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

New failing tests:
svg/custom/altglyph.svg
svg/W3C-SVG-1.1-SE/coords-dom-01-f.svg
svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg
svg/W3C-SVG-1.1-SE/interact-pointer-03-t.svg
svg/W3C-SVG-1.1/fonts-elem-05-t.svg
svg/W3C-SVG-1.1/fonts-elem-06-t.svg
svg/W3C-SVG-1.1-SE/styling-css-04-f.svg
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
svg/W3C-SVG-1.1/filters-turb-02-f.svg
svg/W3C-SVG-1.1-SE/struct-use-14-f.svg
svg/W3C-SVG-1.1-SE/text-intro-02-b.svg
svg/W3C-SVG-1.1-SE/pservers-pattern-04-f.svg
svg/W3C-SVG-1.1/filters-light-04-f.svg
svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
svg/W3C-SVG-1.1-SE/text-intro-05-t.svg
svg/W3C-SVG-1.1-SE/color-prop-05-t.svg
svg/W3C-SVG-1.1-SE/styling-pres-02-f.svg
svg/W3C-SVG-1.1/fonts-desc-02-t.svg
svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-04-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-03-f.svg
svg/W3C-SVG-1.1-SE/filters-image-03-f.svg
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/W3C-SVG-1.1-SE/text-intro-09-b.svg
fast/css/ex-unit-with-no-x-height.html
svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg
svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg
svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg

[Build Bot]

Created attachment 238690 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Myles C. Maxfield]

Comment on attachment 238634 [details]
Patch

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

>> Source/WebCore/css/CSSFontFaceSource.cpp:177
>> +                SVGFontElement* fontElement = downcast<SVGFontElement>(m_svgFontFaceElement->parentNode());
> 
> nit: I have been porting the code to use this style now:
> SVGFontElement& fontElement = downcast<SVGFontElement>(*m_svgFontFaceElement->parentNode());

Done.

>> Source/WebCore/loader/cache/CachedFont.cpp:99
>> +bool CachedFont::ensureCustomFontData(bool svgFont, String svgURI)
> 
> nit: const String& ?

I made this a pointer so I could get rid of the boolean.

Done.

>> Source/WebCore/loader/cache/CachedFont.cpp:110
>> +            auto externalSVGDocument = SVGDocument::create(nullptr, URL());
> 
> I am new to using "auto" but are we sure this will actually be a RefPtr here? (and not a PassRefPtr)

I believe this will be a PassRefPtr. In this case I think it's benign but I'll change it nevertheless so it's more obvious what's going on.

Done.

[Myles C. Maxfield]

I didn't mean for this patch to actually turn the converter ::on::, just to provide a flag with which can can turn it on later

[Myles C. Maxfield]

Created attachment 238691 [details]
Patch

[Myles C. Maxfield]

Created attachment 241398 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 241398 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:301
> +FEATURE_DEFINES = $(ENABLE_3D_RENDERING) $(ENABLE_ACCELERATED_2D_CANVAS) $(ENABLE_ACCELERATED_OVERFLOW_SCROLLING) $(ENABLE_AVF_CAPTIONS) $(ENABLE_CACHE_PARTITIONING) $(ENABLE_CANVAS_PATH) $(ENABLE_CANVAS_PROXY) $(ENABLE_CHANNEL_MESSAGING) $(ENABLE_CONTENT_FILTERING) $(ENABLE_CSP_NEXT) $(ENABLE_CSS_BOX_DECORATION_BREAK) $(ENABLE_CSS_COMPOSITING) $(ENABLE_CSS_DEVICE_ADAPTATION) $(ENABLE_CSS_GRID_LAYOUT) $(ENABLE_CSS_IMAGE_ORIENTATION) $(ENABLE_CSS_IMAGE_RESOLUTION) $(ENABLE_CSS_REGIONS) $(ENABLE_CSS_SELECTORS_LEVEL4) $(ENABLE_CSS_SHAPES) $(ENABLE_CSS3_TEXT) $(ENABLE_CSS3_TEXT_LINE_BREAK) $(ENABLE_CURSOR_VISIBILITY) $(ENABLE_CUSTOM_SCHEME_HANDLER) $(ENABLE_DASHBOARD_SUPPORT) $(ENABLE_DATALIST_ELEMENT) $(ENABLE_DATA_TRANSFER_ITEMS) $(ENABLE_DETAILS_ELEMENT) $(ENABLE_DEVICE_ORIENTATION) $(ENABLE_DOM4_EVENTS_CONSTRUCTOR) $(ENABLE_ENCRYPTED_MEDIA) $(ENABLE_ENCRYPTED_MEDIA_V2) $(ENABLE_FILTERS_LEVEL_2) $(ENABLE_FONT_LOAD_EVENTS) $(ENABLE_FULLSCREEN_API) $(ENABLE_GAMEPAD) $(ENABLE_GAMEPAD_DEPRECATED) $(ENABLE_GEOLOCATION) $(ENABLE_HIDDEN_PAGE_DOM_TIMER_THROTTLING) $(ENABLE_ICONDATABASE) $(ENABLE_SERVICE_CONTROLS) $(ENABLE_INDEXED_DATABASE) $(ENABLE_INDEXED_DATABASE_IN_WORKERS) $(ENABLE_INDIE_UI) $(ENABLE_INPUT_TYPE_COLOR) $(ENABLE_INPUT_TYPE_COLOR_POPOVER) $(ENABLE_INPUT_TYPE_DATE) $(ENABLE_INPUT_TYPE_DATETIME_INCOMPLETE) $(ENABLE_INPUT_TYPE_DATETIMELOCAL) $(ENABLE_INPUT_TYPE_MONTH) $(ENABLE_INPUT_TYPE_TIME) $(ENABLE_INPUT_TYPE_WEEK) $(ENABLE_INSPECTOR) $(ENABLE_IOS_AIRPLAY) $(ENABLE_IOS_GESTURE_EVENTS) $(ENABLE_IOS_TEXT_AUTOSIZING) $(ENABLE_IOS_TOUCH_EVENTS) $(ENABLE_LEGACY_CSS_VENDOR_PREFIXES) $(ENABLE_LEGACY_NOTIFICATIONS) $(ENABLE_LEGACY_VENDOR_PREFIXES) $(ENABLE_LEGACY_WEB_AUDIO) $(ENABLE_LETTERPRESS) $(ENABLE_LINK_PREFETCH) $(ENABLE_MATHML) $(ENABLE_MEDIA_CONTROLS_SCRIPT) $(ENABLE_MEDIA_SOURCE) $(ENABLE_MEDIA_STATISTICS) $(ENABLE_METER_ELEMENT) $(ENABLE_MHTML) $(ENABLE_MOUSE_CURSOR_SCALE) $(ENABLE_NAVIGATOR_CONTENT_UTILS) $(ENABLE_NAVIGATOR_HWCONCURRENCY) $(ENABLE_NOTIFICATIONS) $(ENABLE_PDFKIT_PLUGIN) $(ENABLE_POINTER_LOCK) $(ENABLE_PROMISES) $(ENABLE_PROXIMITY_EVENTS) $(ENABLE_PUBLIC_SUFFIX_LIST) $(ENABLE_QUOTA) $(ENABLE_REQUEST_ANIMATION_FRAME) $(ENABLE_REQUEST_AUTOCOMPLETE) $(ENABLE_REMOTE_INSPECTOR) $(ENABLE_RESOLUTION_MEDIA_QUERY) $(ENABLE_RUBBER_BANDING) $(ENABLE_SHARED_WORKERS) $(ENABLE_CSS_SCROLL_SNAP) $(ENABLE_SPEECH_SYNTHESIS) $(ENABLE_SQL_DATABASE) $(ENABLE_SUBTLE_CRYPTO) $(ENABLE_SVG_FONTS) $(ENABLE_SVG_OTF_CONVERTER) R$(ENABLE_TELEPHONE_NUMBER_DETECTION) $(ENABLE_TEMPLATE_ELEMENT) $(ENABLE_TEXT_AUTOSIZING) $(ENABLE_TOUCH_EVENTS) $(ENABLE_TOUCH_ICON_LOADING) $(ENABLE_USERSELECT_ALL) $(ENABLE_VIDEO) $(ENABLE_VIDEO_TRACK) $(ENABLE_DATACUE_VALUE) $(ENABLE_VIEW_MODE_CSS_MEDIA) $(ENABLE_WEBGL) $(ENABLE_WEB_AUDIO) $(ENABLE_WEB_REPLAY) $(ENABLE_WEB_SOCKETS) $(ENABLE_PICTURE_SIZES) $(ENABLE_WEB_TIMING) $(ENABLE_WEBVTT_REGIONS) $(ENABLE_XHR_TIMEOUT) $(ENABLE_XSLT) $(ENABLE_FTL_JIT) $(ENABLE_LLINT_C_LOOP) $(ENABLE_SATURATED_LAYOUT_ARITHMETIC) $(FEATURE_DEFINES_$(PLATFORM_NAME));

Woah woah woah woah woah

[Myles C. Maxfield]

Created attachment 241401 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 241398 [details]
Patch

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

There's a lot of messing #ifdeffing and conditionals in CachedFont and CSSFontFaceSource. Can we use subclasses or somehow package this up in a cleaner way?

> Source/WebCore/css/CSSFontFaceSource.cpp:180
> +                auto otfFont = convertSVGToOTFFont(fontElement);

Bad use of auto; I've no idea if it's copying by value, or is a unque_ptr or what.

> Source/WebCore/css/CSSFontFaceSource.cpp:182
> +                if (m_generatedOTFBuffer) {

Maybe if (!m_generatedOTFBuffer) return nullptr here.

> Source/WebCore/css/CSSFontFaceSource.cpp:183
> +                    auto customPlatformData = createFontCustomPlatformData(*m_generatedOTFBuffer);

Not keen on auto here too.

> Source/WebCore/loader/cache/CachedFont.cpp:98
> +bool CachedFont::ensureCustomFontData(const AtomicString* svgURI)

Not a fan of this optional argument. Maybe CachedFont should expose ensureCustomFontData() and ensureCustomSVGFontData(svgURI)?

Also, if it's an svgURI why isn't it a URL? And should be it an AtomicString?

It seems this is really a fragment identifier?

> Source/WebCore/loader/cache/CachedFont.cpp:114
> +                const auto* externalSVGFontElement = getSVGFontById(fragmentIdentifier, externalSVGDocument.get());

auto ick.

> Source/WebCore/loader/cache/CachedFont.cpp:118
> +                    auto otf = convertSVGToOTFFont(*externalSVGFontElement);

auto ick.

[Myles C. Maxfield]

Comment on attachment 241398 [details]
Patch

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

http://trac.webkit.org/changeset/176276 cleans up a lot of the #ifdefing

>> Source/WebCore/css/CSSFontFaceSource.cpp:180
>> +                auto otfFont = convertSVGToOTFFont(fontElement);
> 
> Bad use of auto; I've no idea if it's copying by value, or is a unque_ptr or what.

Done.

>> Source/WebCore/css/CSSFontFaceSource.cpp:182
>> +                if (m_generatedOTFBuffer) {
> 
> Maybe if (!m_generatedOTFBuffer) return nullptr here.

Done.

>> Source/WebCore/css/CSSFontFaceSource.cpp:183
>> +                    auto customPlatformData = createFontCustomPlatformData(*m_generatedOTFBuffer);
> 
> Not keen on auto here too.

Done.

>> Source/WebCore/loader/cache/CachedFont.cpp:98
>> +bool CachedFont::ensureCustomFontData(const AtomicString* svgURI)
> 
> Not a fan of this optional argument. Maybe CachedFont should expose ensureCustomFontData() and ensureCustomSVGFontData(svgURI)?
> 
> Also, if it's an svgURI why isn't it a URL? And should be it an AtomicString?
> 
> It seems this is really a fragment identifier?

This has been refactored

>> Source/WebCore/loader/cache/CachedFont.cpp:114
>> +                const auto* externalSVGFontElement = getSVGFontById(fragmentIdentifier, externalSVGDocument.get());
> 
> auto ick.

This has been refactored

>> Source/WebCore/loader/cache/CachedFont.cpp:118
>> +                    auto otf = convertSVGToOTFFont(*externalSVGFontElement);
> 
> auto ick.

This has been refactored

[Myles C. Maxfield]

Created attachment 241804 [details]
Patch

[Myles C. Maxfield]

Created attachment 243407 [details]
Patch

[Build Bot]

Comment on attachment 243407 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 243417 [details]
Archive of layout-test-results from ews103 for mac-mountainlion

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

[Build Bot]

Comment on attachment 243407 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 243418 [details]
Archive of layout-test-results from ews106 for mac-mountainlion-wk2

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

[Myles C. Maxfield]

Created attachment 243576 [details]
Patch

[Antti Koivisto]

Comment on attachment 243576 [details]
Patch

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

r=me

> Source/WebCore/loader/cache/CachedFont.h:60
> +    bool ensureCustomFontData(PassRefPtr<SharedBuffer> data);

Please just use RefPtr. We are tying to kill PassRefPtr as it is unnecessary with C++11 move semantics.

[Darin Adler]

Comment on attachment 243576 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedFont.h:60
>> +    bool ensureCustomFontData(PassRefPtr<SharedBuffer> data);
> 
> Please just use RefPtr. We are tying to kill PassRefPtr as it is unnecessary with C++11 move semantics.

The argument should be either SharedBuffer*, SharedBuffer&, RefPtr<SharedBuffer>&&, or Ref<SharedBuffer>&& depending on the situation. If the caller is handing off ownership but it can be null, then the correct type would be RefPtr<SharedBuffer>&&. I am really unclear in this case what the semantics of the data argument is, so I can’t tell you which should be used.

[Antti Koivisto]

With this factoring the function is ownership transferring and can take null so RefPtr<SharedBuffer>&& would likely be the right choice.

[Myles C. Maxfield]

(In reply to comment #25)
> With this factoring the function is ownership transferring and can take null
> so RefPtr<SharedBuffer>&& would likely be the right choice.

There are two callers. One wishes to transfer ownership, and the other doesn't. Therefore, I've elected to use RefPtr<SharedBuffer>&& with one of the callers explicitly creating a copy of the RefPtr.

[Myles C. Maxfield]

Committed r178292: <http://trac.webkit.org/changeset/178292>

[Antti Koivisto]

> There are two callers. One wishes to transfer ownership, and the other
> doesn't. Therefore, I've elected to use RefPtr<SharedBuffer>&& with one of
> the callers explicitly creating a copy of the RefPtr.

There is RefPtr<T>::copyRef() for that.

[Myles C. Maxfield]

Does RefPtr<T>::copyRef() get us anything that the constructor doesn't? If not, I'd say that we should get rid of the function because there's no need to have two ways of doing the same thing.

[Myles C. Maxfield]

Oh, you don't need to explicitly state the template parameter. Good idea.

[Darin Adler]

(In reply to comment #29)
> Does RefPtr<T>::copyRef() get us anything that the constructor doesn't? If
> not, I'd say that we should get rid of the function because there's no need
> to have two ways of doing the same thing.

Long term, we are going to be getting rid of the constructor and keeping the function.