[webkit-reviews] review granted: [Bug 49388] Share code between Mac (CA) and Windows (CACF) GraphicsLayer implementations : [Attachment 78200] Final Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 6 18:37:50 PST 2011
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 49388: Share code between Mac (CA) and Windows (CACF) GraphicsLayer
implementations
https://bugs.webkit.org/show_bug.cgi?id=49388
Attachment 78200: Final Patch
https://bugs.webkit.org/attachment.cgi?id=78200&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
r=me with comments, and questions about the vcproj change.
> WebCore/WebCore.vcproj/WebCore.vcproj:22
> <Configuration
> Name="Debug|Win32"
> ConfigurationType="4"
> -
InheritedPropertySheets="$(WebKitVSPropsRedirectionDir)..\..\WebKitLibraries\wi
n\tools\vsprops\FeatureDefines.vsprops;$(WebKitVSPropsRedirectionDir)..\..\WebK
itLibraries\win\tools\vsprops\common.vsprops;$(WebKitVSPropsRedirectionDir)..\.
.\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebCoreCommon.vsprops;.\Web
CoreCG.vsprops;.\WebCoreCFNetwork.vsprops;.\WebCorePthreads.vsprops;.\WebCoreMe
diaQT.vsprops;.\WebCoreQuartzCore.vsprops"
> +
InheritedPropertySheets="..\..\WebKitLibraries\win\tools\vsprops\FeatureDefines
.vsprops;..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;..\..\WebKitLib
raries\win\tools\vsprops\debug.vsprops;.\WebCoreCommon.vsprops;.\WebCoreCG.vspr
ops;.\WebCoreCFNetwork.vsprops;.\WebCorePthreads.vsprops;.\WebCoreMediaQT.vspro
ps;.\WebCoreQuartzCore.vsprops"
> CharacterSet="1"
Many of these changes look bad. You might want to hand-edit the project file.
> WebCore/WebCore.vcproj/WebCoreCommon.vsprops:11
> -
AdditionalIncludeDirectories=""$(ProjectDir)..";"$(ProjectDir)..
\accessibility";"$(ProjectDir)..\accessibility\win";"$(Proj
ectDir)..\bridge";"$(ProjectDir)..\bridge\c";"$(ProjectDir)
..\bridge\jsc";"$(ProjectDir)..\css";"$(ProjectDir)..\editi
ng";"$(ProjectDir)..\fileapi";"$(ProjectDir)..\rendering&qu
ot;;"$(ProjectDir)..\rendering\style";"$(ProjectDir)..\rendering
\svg";"$(ProjectDir)..\bindings";"$(ProjectDir)..\bindings\
generic";"$(ProjectDir)..\bindings\js";"$(ProjectDir)..\bin
dings\js\specialization";"$(ProjectDir)..\dom";"$(ProjectDi
r)..\dom\default";"$(ProjectDir)..\history";"$(ProjectDir).
.\html";"$(ProjectDir)..\html\canvas";"$(ProjectDir)..\html
\parser";"$(ProjectDir)..\html\shadow";"$(ProjectDir)..\ins
pector";"$(ProjectDir)..\loader";"$(ProjectDir)..\loader\ap
pcache";"$(ProjectDir)..\loader\archive";"$(ProjectDir)..\l
oader\archive\cf";"$(ProjectDir)..\loader\cache";"$(Project
Dir)..\loader\icon";"$(ProjectDir)..\mathml";"$(ProjectDir)
..\notifications";"$(ProjectDir)..\page";"$(ProjectDir)..\p
age\animation";"$(ProjectDir)..\page\win";"$(ProjectDir)..\
platform";"$(ProjectDir)..\platform\animation";"$(ProjectDi
r)..\platform\mock";"$(ProjectDir)..\platform\sql";"$(Proje
ctDir)..\platform\win";"$(ProjectDir)..\platform\network";"
$(ProjectDir)..\platform\network\win";"$(ProjectDir)..\platform\cf&qu
ot;;"$(ProjectDir)..\platform\graphics";"$(ProjectDir)..\platfor
m\graphics\filters";"$(ProjectDir)..\platform\graphics\opentype"
;"$(ProjectDir)..\platform\graphics\transforms";"$(ProjectDir)..
\platform\text";"$(ProjectDir)..\platform\text\transcoder";"
;$(ProjectDir)..\platform\graphics\win";"$(ProjectDir)..\xml";&q
uot;$(ConfigurationBuildDir)\obj\WebCore\DerivedSources";"$(ProjectDi
r)..\plugins";"$(ProjectDir)..\plugins\win";"$(ProjectDir).
.\svg\animation";"$(ProjectDir)..\svg\graphics";"$(ProjectD
ir)..\svg\properties";"$(ProjectDir)..\svg\graphics\filters";&qu
ot;$(ProjectDir)..\svg";"$(ProjectDir)..\wml";"$(ProjectDir
)..\storage";"$(ProjectDir)..\websockets";"$(ProjectDir)..\
workers";"$(ConfigurationBuildDir)\include";"$(Configuratio
nBuildDir)\include\private";"$(ConfigurationBuildDir)\include\JavaScr
iptCore";"$(ConfigurationBuildDir)\include\private\JavaScriptCore&quo
t;;"$(ProjectDir)..\ForwardingHeaders";"$(WebKitLibrariesDir)\in
clude";"$(WebKitLibrariesDir)\include\private";"$(WebKitLib
rariesDir)\include\private\JavaScriptCore";"$(WebKitLibrariesDir)\inc
lude\pthreads";"$(WebKitLibrariesDir)\include\sqlite";"$(We
bKitLibrariesDir)\include\JavaScriptCore";"$(WebKitLibrariesDir)\incl
ude\zlib""
> +
AdditionalIncludeDirectories=""$(ProjectDir)..";"$(ProjectDir)..
\accessibility";"$(ProjectDir)..\accessibility\win";"$(Proj
ectDir)..\bridge";"$(ProjectDir)..\bridge\c";"$(ProjectDir)
..\bridge\jsc";"$(ProjectDir)..\css";"$(ProjectDir)..\editi
ng";"$(ProjectDir)..\fileapi";"$(ProjectDir)..\rendering&qu
ot;;"$(ProjectDir)..\rendering\style";"$(ProjectDir)..\rendering
\svg";"$(ProjectDir)..\bindings";"$(ProjectDir)..\bindings\
generic";"$(ProjectDir)..\bindings\js";"$(ProjectDir)..\bin
dings\js\specialization";"$(ProjectDir)..\dom";"$(ProjectDi
r)..\dom\default";"$(ProjectDir)..\history";"$(ProjectDir).
.\html";"$(ProjectDir)..\html\canvas";"$(ProjectDir)..\html
\parser";"$(ProjectDir)..\html\shadow";"$(ProjectDir)..\ins
pector";"$(ProjectDir)..\loader";"$(ProjectDir)..\loader\ap
pcache";"$(ProjectDir)..\loader\archive";"$(ProjectDir)..\l
oader\archive\cf";"$(ProjectDir)..\loader\cache";"$(Project
Dir)..\loader\icon";"$(ProjectDir)..\mathml";"$(ProjectDir)
..\notifications";"$(ProjectDir)..\page";"$(ProjectDir)..\p
age\animation";"$(ProjectDir)..\page\win";"$(ProjectDir)..\
platform";"$(ProjectDir)..\platform\animation";"$(ProjectDi
r)..\platform\mock";"$(ProjectDir)..\platform\sql";"$(Proje
ctDir)..\platform\win";"$(ProjectDir)..\platform\network";"
$(ProjectDir)..\platform\network\win";"$(ProjectDir)..\platform\cf&qu
ot;;"$(ProjectDir)..\platform\graphics";"$(ProjectDir)..\platfor
m\graphics\ca";"$(ProjectDir)..\platform\graphics\filters";"
;$(ProjectDir)..\platform\graphics\opentype";"$(ProjectDir)..\platfor
m\graphics\transforms";"$(ProjectDir)..\platform\text";"$(P
rojectDir)..\platform\text\transcoder";"$(ProjectDir)..\platform\grap
hics\win";"$(ProjectDir)..\xml";"$(ConfigurationBuildDir)\o
bj\WebCore\DerivedSources";"$(ProjectDir)..\plugins";"$(Pro
jectDir)..\plugins\win";"$(ProjectDir)..\svg\animation";"$(
ProjectDir)..\svg\graphics";"$(ProjectDir)..\svg\properties";&qu
ot;$(ProjectDir)..\svg\graphics\filters";"$(ProjectDir)..\svg";&
quot;$(ProjectDir)..\wml";"$(ProjectDir)..\storage";"$(Proj
ectDir)..\websockets";"$(ProjectDir)..\workers";"$(Configur
ationBuildDir)\include";"$(ConfigurationBuildDir)\include\private&quo
t;;"$(ConfigurationBuildDir)\include\JavaScriptCore";"$(Configur
ationBuildDir)\include\private\JavaScriptCore";"$(ProjectDir)..\Forwa
rdingHeaders";"$(WebKitLibrariesDir)\include";"$(WebKitLibr
ariesDir)\include\private";"$(WebKitLibrariesDir)\include\private\Jav
aScriptCore";"$(WebKitLibrariesDir)\include\pthreads";"$(We
bKitLibrariesDir)\include\sqlite";"$(WebKitLibrariesDir)\include\Java
ScriptCore";"$(WebKitLibrariesDir)\include\zlib""
>
PreprocessorDefinitions="__WIN32__;DISABLE_3D_RENDERING;WEBCORE_CONTEXT_MENUS"
Do you really want this change?
> WebCore/html/canvas/CanvasRenderingContext.h:63
> - virtual PlatformLayer* platformLayer() const { return 0; }
> +// virtual PlatformLayer* platformLayer() const { return 0; }
No commenting out please!
> WebCore/platform/graphics/ca/PlatformCAAnimation.h:47
> +typedef struct _CACFAnimation *CACFAnimationRef;
The * should be on the other side here.
> WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
2011!
> WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:153
> +static void initialize(PlatformCAAnimation* animation)
> +{
> +}
Unused?
> WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:188
> + RetainPtr<CFStringRef> s(AdoptCF,
animation->keyPath().createCFString());
> + CACFAnimationSetKeyPath(m_animation.get(), s.get());
Please rename 's' to keyPath or similar.
> WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:220
> +bool PlatformCAAnimation::supportsValueFunction()
> +{
> + return true;
> +}
Method should be const, could be inline.
> WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:300
> + RetainPtr<CFStringRef> s(AdoptCF,
toCACFFillModeType(value).createCFString());
> + CACFAnimationSetFillMode(m_animation.get(), s.get());
Please rename 's'.
> WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:341
> + RetainPtr<CFStringRef> s(AdoptCF,
toCACFValueFunctionType(value).createCFString());
> + CACFAnimationSetValueFunction(m_animation.get(),
CACFValueFunctionGetFunctionWithName(s.get()));
ditto
> WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
2011!
> WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:47
> +bool PlatformCALayer::isValueFunctionSupported()
> +{
> + return true;
> +}
Should be const and inline.
> WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:303
> +PassRefPtr<PlatformCAAnimation> PlatformCALayer::animationForKey(const
String& key)
Method should be const.
> WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp:460
> + return const_cast<void*>(CACFLayerGetContents(m_layer.get()));
Is that the right cast?
> WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
2011!
> WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:63
> + if (!owner() || !owner()->owner())
owner()->owner() is weird.
> WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.h:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
2011
> WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:48
> + if (info.bmBitsPixel != 32)
> + return 0;
Why this change?
>
WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:104
> + virtual void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*);
> + virtual bool platformCALayerRespondsToLayoutChanges() const { return
true; }
> +
> + virtual void platformCALayerAnimationStarted(CFTimeInterval beginTime) {
}
> + virtual GraphicsLayer::CompositingCoordinatesOrientation
platformCALayerContentsOrientation() const { return
GraphicsLayer::CompositingCoordinatesBottomUp; }
> + virtual void platformCALayerPaintContents(GraphicsContext&, const
IntRect& inClip) { }
> + virtual bool platformCALayerShowDebugBorders() const { return false; }
> + virtual bool platformCALayerShowRepaintCounter() const { return false; }
> + virtual int platformCALayerIncrementRepaintCount() { return 0; }
> +
> + virtual bool platformCALayerContentsOpaque() const { return false; }
> + virtual bool platformCALayerDrawsContent() const { return false; }
> + virtual void platformCALayerLayerDidDisplay(PlatformLayer*) { }
Make these private if possible.
> WebCore/platform/graphics/win/WKCACFLayerRenderer.cpp:481
> + if (!wkCACFContextBeginUpdate(m_context, space, sizeof(space),
CACurrentMediaTime(), bounds, windowDirtyRects.data(),
windowDirtyRects.size()))
Share the same CACurrentMediaTime you used above?
> WebKit/win/FullscreenVideoController.cpp:193
> + virtual void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*);
> + virtual bool platformCALayerRespondsToLayoutChanges() const { return
true; }
> +
> + virtual void platformCALayerAnimationStarted(CFTimeInterval beginTime) {
}
> + virtual GraphicsLayer::CompositingCoordinatesOrientation
platformCALayerContentsOrientation() const { return
GraphicsLayer::CompositingCoordinatesBottomUp; }
> + virtual void platformCALayerPaintContents(GraphicsContext&, const
IntRect& inClip) { }
> + virtual bool platformCALayerShowDebugBorders() const { return false; }
> + virtual bool platformCALayerShowRepaintCounter() const { return false; }
> + virtual int platformCALayerIncrementRepaintCount() { return 0; }
> +
> + virtual bool platformCALayerContentsOpaque() const { return false; }
> + virtual bool platformCALayerDrawsContent() const { return false; }
> + virtual void platformCALayerLayerDidDisplay(PlatformLayer*) { }
Make private?
More information about the webkit-reviews
mailing list