Allow targetting the SVG->OTF font converter with ENABLE(SVG_OTF_CONVERTER)
Created attachment 238008 [details] Patch
Created attachment 238634 [details] Patch
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)
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
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
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
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
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.
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
Created attachment 238691 [details] Patch
Created attachment 241398 [details] Patch
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
Created attachment 241401 [details] Patch
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.
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
Created attachment 241804 [details] Patch
Created attachment 243407 [details] Patch
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.
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
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.
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
Created attachment 243576 [details] Patch
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.
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.
With this factoring the function is ownership transferring and can take null so RefPtr<SharedBuffer>&& would likely be the right choice.
(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.
Committed r178292: <http://trac.webkit.org/changeset/178292>
> 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.
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.
Oh, you don't need to explicitly state the template parameter. Good idea.
(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.