Bug 154104

Summary: Fix the !(ENABLE(SVG_FONTS) || ENABLE(SVG_OTF_CONVERTER)) build after r196322
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, darin, koivisto, mmaxfield, ossy
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=154035
Bug Depends on:    
Bug Blocks: 153414    
Attachments:
Description Flags
Patch none

Csaba Osztrogonác
Reported 2016-02-11 03:26:28 PST
http://trac.webkit.org/changeset/196322 broke the !(ENABLE(SVG_FONTS) || ENABLE(SVG_OTF_CONVERTER)) build, because CSSFontFaceSource::CSSFontFaceSource:m_svgFontFaceElement is declared inside ifdef guards, but initialized uncionditionally in the init list of the constructor.
Attachments
Patch (1.48 KB, patch)
2016-02-11 03:33 PST, Csaba Osztrogonác 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2016-02-11 03:33:20 PST
Created attachment 271055 [details] Patch
Csaba Osztrogonác
Comment 2 2016-02-11 03:43:40 PST
Or should we declare m_svgFontFaceElement unconditionally? ( similar to bug154035 )
Alex Christensen
Comment 3 2016-02-11 08:57:40 PST
(In reply to comment #2) > Or should we declare m_svgFontFaceElement unconditionally? > ( similar to bug154035 ) I think that would be cleaner.
Darin Adler
Comment 4 2016-02-11 09:30:52 PST
(In reply to comment #3) > (In reply to comment #2) > > Or should we declare m_svgFontFaceElement unconditionally? > > ( similar to bug154035 ) > I think that would be cleaner. I agree.
Myles C. Maxfield
Comment 5 2016-02-11 13:31:28 PST
EWS was all green on http://trac.webkit.org/changeset/196322 :(
Csaba Osztrogonác
Comment 6 2016-02-12 02:02:53 PST
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Or should we declare m_svgFontFaceElement unconditionally? > > > ( similar to bug154035 ) > > I think that would be cleaner. > > I agree. I checked it in details, it's impossible. m_svgFontFaceElement has RefPtr<SVGFontFaceElement> type and SVGFontFaceElement class is defined inside ENABLE(SVG_FONTS) guard. To be able to use RefPtr<SVGFontFaceElement> type, we have to remove ENABLE(SVG_FONTS) guard from SVGFontFaceElement.h and I think transitionally all ENABLE(SVG_FONTS) guards everywhere. The question is if we want to support !ENABLE(SVG_FONTS) build or not. If yes, we should land my previously proposed patch. If not, we should remove all ENABLE(SVG_FONTS) guards.
Csaba Osztrogonác
Comment 7 2016-02-12 02:03:39 PST
Comment on attachment 271055 [details] Patch r? again, maybe we would like to keep !ENABLE(SVG_FONTS) build configuration work.
Myles C. Maxfield
Comment 8 2016-02-12 10:44:36 PST
(In reply to comment #6) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > Or should we declare m_svgFontFaceElement unconditionally? > > > > ( similar to bug154035 ) > > > I think that would be cleaner. > > > > I agree. > > I checked it in details, it's impossible. > > m_svgFontFaceElement has RefPtr<SVGFontFaceElement> type and > SVGFontFaceElement class is defined inside ENABLE(SVG_FONTS) guard. > > To be able to use RefPtr<SVGFontFaceElement> type, we have to > remove ENABLE(SVG_FONTS) guard from SVGFontFaceElement.h and > I think transitionally all ENABLE(SVG_FONTS) guards everywhere. > > The question is if we want to support !ENABLE(SVG_FONTS) build or not. > If yes, we should land my previously proposed patch. If not, we should > remove all ENABLE(SVG_FONTS) guards. FWIW, I am very close to enabling the SVG font converter on Windows, and I hope the remaining ports will be quick to follow. I hope to remove all the SVG font code (in favor of the converter) in a few weeks.
WebKit Commit Bot
Comment 9 2016-02-15 04:28:20 PST
Comment on attachment 271055 [details] Patch Clearing flags on attachment: 271055 Committed r196576: <http://trac.webkit.org/changeset/196576>
WebKit Commit Bot
Comment 10 2016-02-15 04:28:25 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.