Bug 189579

Summary: Build error in ImageBufferCG when compiled with IOSurfacePool
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: Tools / TestsAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, lforschler, mdelaney7, mjs, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 188043    
Attachments:
Description Flags
Quick hack to work around the issue
none
Patch
none
Patch (move definition to Platform.h) none

Frédéric Wang (:fredw)
Reported 2018-09-13 03:27:34 PDT
Applying attachment 349536 [details], I get the errors below on macOS and iOS ports, the unified file is: #include "platform/graphics/cg/IOSurfacePool.cpp" #include "platform/graphics/cg/ImageBufferCG.cpp" #include "platform/graphics/cg/ImageBufferDataCG.cpp" #include "platform/graphics/cg/ImageDecoderCG.cpp" #include "platform/graphics/cg/IntPointCG.cpp" #include "platform/graphics/cg/IntRectCG.cpp" #include "platform/graphics/cg/IntSizeCG.cpp" #include "platform/graphics/cg/NativeImageCG.cpp" IOSurfacePool includes IOSurface.h a first time without defining IOSURFACE_CANVAS_BACKING_STORE ImageBufferCG includes IOSurface.h again but defining IOSURFACE_CANVAS_BACKING_STORE (this second include is ignored because of the #pragma once directive) This is causing IOSurface::createFromImageBuffer to become undefined: In file included from /Users/fred/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource329.cpp:2: ./platform/graphics/cg/ImageBufferCG.cpp:305:48: error: no member named 'createFromImageBuffer' in 'WebCore::IOSurface' return IOSurface::sinkIntoImage(IOSurface::createFromImageBuffer(WTF... ~~~~~~~~~~~^ ./platform/graphics/cg/ImageBufferCG.cpp:348:71: error: no member named 'createFromImageBuffer' in 'WebCore::IOSurface' RetainPtr<CGImageRef> image = IOSurface::sinkIntoImage(IOSurface::create... ~~~~~~~~~~~^ 2 errors generated.
Attachments
Quick hack to work around the issue (1.55 KB, patch)
2018-09-13 06:25 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (6.77 KB, patch)
2018-09-13 08:56 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch (move definition to Platform.h) (4.38 KB, patch)
2018-09-14 05:35 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2018-09-13 06:25:14 PDT
Created attachment 349660 [details] Quick hack to work around the issue
Frédéric Wang (:fredw)
Comment 2 2018-09-13 08:56:07 PDT
Created attachment 349673 [details] Patch A proposed solution. Maybe an alternative is to define USE_IOSURFACE_CANVAS_BACKING_STORE in Source/WTF/wtf/Platform.h?
Maciej Stachowiak
Comment 3 2018-09-13 09:47:28 PDT
Comment on attachment 349673 [details] Patch Instead of this change, I think the right fix is to make sure all files have the same value for IOSURFACE_CANVAS_BACKING_STORE. If headers depend on an #ifdef and different files have it set differently, that is a potential problem even without unified builds. Not in this case since the only difference is a static member function, but if the define affected data members, then it would be real bad for different files to have different values.
Frédéric Wang (:fredw)
Comment 4 2018-09-13 12:50:54 PDT
(In reply to Maciej Stachowiak from comment #3) > Comment on attachment 349673 [details] > Patch > > Instead of this change, I think the right fix is to make sure all files have > the same value for IOSURFACE_CANVAS_BACKING_STORE. If headers depend on an > #ifdef and different files have it set differently, that is a potential > problem even without unified builds. Not in this case since the only > difference is a static member function, but if the define affected data > members, then it would be real bad for different files to have different > values. Yeah, I was wondering that too. Does the alternative I propose in comment 2 make sense?
Frédéric Wang (:fredw)
Comment 5 2018-09-14 05:35:52 PDT
Created attachment 349757 [details] Patch (move definition to Platform.h)
Frédéric Wang (:fredw)
Comment 6 2018-09-16 23:13:16 PDT
cc'ing Simon and Matthew since it looks like the definition was added in bug 58084.
Tim Horton
Comment 7 2018-09-17 10:55:10 PDT
Comment on attachment 349757 [details] Patch (move definition to Platform.h) View in context: https://bugs.webkit.org/attachment.cgi?id=349757&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.h:43 > +#if USE(IOSURFACE_CANVAS_BACKING_STORE) No real need for the #ifs here but they're fine too.
WebKit Commit Bot
Comment 8 2018-09-17 11:24:18 PDT
Comment on attachment 349757 [details] Patch (move definition to Platform.h) Clearing flags on attachment: 349757 Committed r236074: <https://trac.webkit.org/changeset/236074>
WebKit Commit Bot
Comment 9 2018-09-17 11:24:20 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 10 2018-09-17 11:25:26 PDT
<rdar://problem/44527313>
Note You need to log in before you can comment on or make changes to this bug.