Bug 49388 – Share code between Mac (CA) and Windows (CACF) GraphicsLayer implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Marrin]

Currently GraphicsLayerCA and GraphicsLayerCACF are very similar and much of the code is duplicated. The big difference is the underlying ObjectiveC CALayer (Mac) and the C CACFLayer Windows)). We already have a wrapper around CACFLayer called WKCACFLayer. If we make a common API for these two classes, we can use that API in GraphicsLayer and share most of the code. 

The API will be the C++ class PlatformCALayer and it will have PlatformCALayerMac.mm and PlatformLayerWin.cpp implementations. There will also be PlatformCALayer.cpp for any common code. We will also need a common PlatformCAAnimation class to wrap the functionality of CAAnimation and CACFAnimation.

As a first step I will rename the current GraphicsLayerCA.* files to GraphicsLayerMac.*. That will allow me to create a new GraphicsLayerCA object which will be the common class using PlatformCALayer and PlatformCAAnimation. Eventually GraphicsLayerMac.* will go away, as will GraphicsLayerCACF on Windows.

The new file will go in the new folder platform/graphics/ca.

[Vangelis Kokkevis]

Maybe it's a good time to think about how to factor the code such that common pieces can be used by non-CA platforms. Right now platform/graphics/chromium duplicates a lot of the CACF code as well.

[Chris Marrin]

Created attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*

[Simon Fraser (smfr)]

Comment on attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*

The GraphicsLayerCA class needs to renamed too (hint: Edit All in Scope).

[Simon Fraser (smfr)]

Comment on attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*

I was confused by SVN.

[Chris Marrin]

Landed rename part in http://trac.webkit.org/changeset/71848

[Chris Marrin]

(In reply to comment #1)
> Maybe it's a good time to think about how to factor the code such that common pieces can be used by non-CA platforms. Right now platform/graphics/chromium duplicates a lot of the CACF code as well.

I think it's reasonable to make the work I'm doing have a wider scope than "CA based implementations". But I'd like to get the convergence between CA and CACF finished first. Then I think it will be simpler for you to understand how your implementation fits in and how we can change the new "ca" based API to have that wider scope.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71848 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
fast/workers/storage/interrupt-database-sync.html

[Eric Seidel (no email)]

Comment on attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*

Cleared Simon Fraser's review+ from obsolete attachment 73632 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Chris Marrin]

Created attachment 75382 [details]
Patch with Mac Implementation

[Chris Marrin]

Created attachment 75394 [details]
Fix merge issue with last patch

[Early Warning System Bot]

Attachment 75394 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6801015

[WebKit Review Bot]

Attachment 75394 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6826009

[Simon Fraser (smfr)]

Comment on attachment 75394 [details]
Fix merge issue with last patch

> WebCore/platform/graphics/GraphicsLayer.h:51
>  typedef CALayer PlatformLayer;

I think we should do this as:
typedef CALayer* PlatformLayerRef
to avoid confusion about whether it's a pointer or not.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:258
> +PassOwnPtr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerClient* client)
> +{
> +    return new GraphicsLayerCA(client);
> +}
> +

This could be in the header.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:291
> +    // Release the clone layers inside the exception-handling block.
> +    removeCloneLayers();

The comment is no longer applicable.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1459
> +static void copyAnimationProperties(const PlatformCAAnimation* from, PlatformCAAnimation* to)
> +{
> +    to->setBeginTime(from->beginTime());
> +    to->setDuration(from->duration());
> +    to->setRepeatCount(from->repeatCount());
> +    to->setAutoreverses(from->autoreverses());
> +    to->setFillMode(from->fillMode());
> +    to->setRemovedOnCompletion(from->isRemovedOnCompletion());
> +    to->setAdditive(from->isAdditive());
> +    to->copyTimingFunctionFrom(from);
> +
> +#if HAVE_MODERN_QUARTZCORE

I think we could have a copy constructor on PlatformCAAnimation instead, here.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1524
> +void GraphicsLayerCA::setContentsToCanvas(PlatformLayer* canvasLayer)
> +{
> +    if (m_contentsLayer && canvasLayer == m_contentsLayer->platformLayer())
> +        return;
> +    
> +    // Create the PlatformCALayer to wrap the incoming layer
> +    m_contentsLayer = canvasLayer ? PlatformCALayer::create(canvasLayer, this) : 0;
> +    
> +    m_contentsLayerPurpose = canvasLayer ? ContentsLayerForCanvas : NoContentsLayer;
> +
> +    noteSublayersChanged();
> +    noteLayerPropertyChanged(ContentsCanvasLayerChanged);
> +}

Please move this so it's next to the other "setContentsTo" methods.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1762
> +
> +
> +
> +
> +        if (isTransformTypeNumber(transformOpType)) {

Extra blank lines here.

> WebCore/platform/graphics/ca/PlatformCAAnimation.h:63
> +    static PassRefPtr<PlatformCAAnimation> create(void* animation);

CAPropertyAnimation rather than void*? Or maybe you have to have a typedef that maps to CAPropertyAnimation on Mac, and CACFAnimationRef on Windows?

> WebCore/platform/graphics/ca/PlatformCAAnimation.h:69
> +    void* platformAnimation() const;

Why not return a CAPropertyAnimation?

> WebCore/platform/graphics/ca/PlatformCALayer.h:50
> +    // FIXME: This object represents all Layer types used in GraphicsLayer, and those used by video and canvas.

Why is that a FIXME?

> WebCore/platform/graphics/ca/PlatformCALayer.h:65
> +    // FIXME: turn off implicit animation in ctor

Either do that, or remove the FIXME.
 

r=me with those comments addressed.

[Build Bot]

Attachment 75394 [details] did not build on win:
Build output: http://queues.webkit.org/results/6839009

[Eric Seidel (no email)]

Attachment 75394 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6754009

[Chris Marrin]

Created attachment 75422 [details]
Patch fixing issues from the review and add 2 files

[Chris Marrin]

Comment on attachment 75422 [details]
Patch fixing issues from the review and add 2 files

The last patch was missing the PlatformCALayerMac.mm and PlatformCAAnimationMac.mm implementation files. I also added some initial settings to in TiledLayer creation. Then I implemented all of Simon's recommendations.

[Chris Marrin]

This latest patch also fixes build errors in Qt, Win, and Chrome builds

[Simon Fraser (smfr)]

Comment on attachment 75422 [details]
Patch fixing issues from the review and add 2 files

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

> WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:234
> +bool PlatformCAAnimation::nonZeroBeginTimeFlag() const

I think this should be hasNonZeroBeginTimeFlag()

> WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:242
> +    [m_animation.get() setValue:[NSNumber numberWithBool:value] forKey:WKNonZeroBeginTimeFlag];
> +

Extra space.

> WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:182
> +                layerClass = NSClassFromString(@"CALayer");

[CALayer class] would be fine here, and faster.

[Simon Fraser (smfr)]

This was rolled out because of leopard build issues. I tried to fix them, but fixes were non-obvious.

[Eric Seidel (no email)]

Attachment 75422 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6851007

[Chris Marrin]

Created attachment 75522 [details]
Please do not review. Just submitting to see the errors on the bots

[Chris Marrin]

Created attachment 75530 [details]
Fixed merge conflicts

[Chris Marrin]

Created attachment 75575 [details]
Patch

[Chris Marrin]

Comment on attachment 75575 [details]
Patch

This patch landed with http://trac.webkit.org/changeset/73380

[Chris Marrin]

Created attachment 75842 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 75842 [details]
Patch

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

New files are missing from the patch.

> JavaScriptCore/wtf/Platform.h:535
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(WIN)
> +#define WTF_PLATFORM_CA 1
> +#endif

Are you sure this won't affect other ports on Windows?

[Chris Marrin]

Created attachment 75844 [details]
Patch

[Chris Marrin]

Created attachment 75846 [details]
Patch with some housekeeping changes

[Simon Fraser (smfr)]

Comment on attachment 75846 [details]
Patch with some housekeeping changes

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

Don't forget to add the new files to the vcproj.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:-45
> -#include <wtf/UnusedParam.h>

Are you sure it's unused on all platforms?

[Chris Marrin]

(In reply to comment #30)
> (From update of attachment 75846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75846&action=review
> 
> Don't forget to add the new files to the vcproj.

I will do that when I do the Windows side, which I haven't yet.

> 
> > WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:-45
> > -#include <wtf/UnusedParam.h>
> 
> Are you sure it's unused on all platforms?

The macro isn't used anywhere in GraphicsLayerCA.cpp and there aren't any relevant ifdefs

[WebKit Review Bot]

Attachment 75842 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75844 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75846 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Marrin]

Created attachment 75943 [details]
Resubmission of last patch with build fixes

[Chris Marrin]

relanded housekeeping patch in http://trac.webkit.org/changeset/73540

[Chris Marrin]

Created attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag

[Chris Marrin]

Comment on attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag

Windows doesn't need this flag, so it is just an implementation detail. This buries it in the Mac implementation.

[Simon Fraser (smfr)]

Comment on attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag

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

> WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:252
> +    // The flag is needed because later beginTIme will get changed

beginTime.

[Chris Marrin]

Comment on attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag

Landed in http://trac.webkit.org/changeset/73731

[Chris Marrin]

Created attachment 76223 [details]
Patch adding CACFContextGetLastCommitTime to WKSI

[Chris Marrin]

Created attachment 76225 [details]
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file)

[Adam Roben (:aroben)]

Comment on attachment 76225 [details]
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file)

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

> WebKitLibraries/ChangeLog:7
> +        Also got rid of WebKitSystemInterface_debug.lib as recommended by
> +        Adam Roben.

You should mention that this hasn't been used since r72327.

[Chris Marrin]

Comment on attachment 76225 [details]
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file)

Landed in http://trac.webkit.org/changeset/73790

[Eric Seidel (no email)]

Comment on attachment 75575 [details]
Patch

Cleared Simon Fraser's review+ from obsolete attachment 75575 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 75846 [details]
Patch with some housekeeping changes

Cleared Simon Fraser's review+ from obsolete attachment 75846 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag

Cleared Simon Fraser's review+ from obsolete attachment 76144 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 76225 [details]
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file)

Cleared Adam Roben's review+ from obsolete attachment 76225 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Chris Marrin]

Created attachment 78154 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78154 [details]
Patch

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

This all looks fine, but reflections aren't working because layerDidDisplay isn't being called on Windows, as we discussed.

> WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:916
> -        for (size_t i = 0; i < newSublayers.size(); ++i)
> +        for (int i = newSublayers.size() - 1; i >= 0; --i)
>              newSublayers[i]->removeFromSuperlayer();

Why the order reversal?

> WebCore/platform/graphics/ca/GraphicsLayerCA.h:67
> +    // PlatformCALayerClient overrides
> +    virtual void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*) { }
> +    virtual bool platformCALayerRespondsToLayoutChanges() const { return false; }
> +
> +    virtual void platformCALayerAnimationStarted(CFTimeInterval beginTime);
> +    virtual CompositingCoordinatesOrientation platformCALayerContentsOrientation() const { return contentsOrientation(); }
> +    virtual void platformCALayerPaintContents(GraphicsContext& context, const IntRect& clip) { paintGraphicsLayerContents(context, clip); }
> +    virtual bool platformCALayerShowDebugBorders() const { return showDebugBorders(); }
> +    virtual bool platformCALayerShowRepaintCounter() const { return showRepaintCounter(); }
> +    virtual int platformCALayerIncrementRepaintCount() { return incrementRepaintCount(); }
> +
> +    virtual bool platformCALayerContentsOpaque() const { return contentsOpaque(); }
> +    virtual bool platformCALayerDrawsContent() const { return drawsContent(); }
> +    virtual void platformCALayerLayerDidDisplay(PlatformLayer* layer) { return layerDidDisplay(layer); }

Please make these private.

> WebCore/platform/graphics/ca/PlatformCALayerClient.h:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2011!

[Simon Fraser (smfr)]

Comment on attachment 78154 [details]
Patch

Ah, this is Mac code. r=me

[Build Bot]

Attachment 78154 [details] did not build on win:
Build output: http://queues.webkit.org/results/7402011

[Chris Marrin]

Committed r75199: <http://trac.webkit.org/changeset/75199>

[Chris Marrin]

Not quite done yet. Hopefully the next patch will be the last. It will have the Windows side of all this

[WebKit Review Bot]

http://trac.webkit.org/changeset/75199 might have broken Windows Release (Build) and Windows Debug (Build)

[Chris Marrin]

Created attachment 78196 [details]
Patch

[Chris Marrin]

Created attachment 78197 [details]
Patch

[Chris Marrin]

Created attachment 78200 [details]
Final Patch

[Simon Fraser (smfr)]

Comment on attachment 78196 [details]
Patch

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

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\win\tools\vsprops\FeatureDefines.vsprops;$(WebKitVSPropsRedirectionDir)..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;$(WebKitVSPropsRedirectionDir)..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebCoreCommon.vsprops;.\WebCoreCG.vsprops;.\WebCoreCFNetwork.vsprops;.\WebCorePthreads.vsprops;.\WebCoreMediaQT.vsprops;.\WebCoreQuartzCore.vsprops"
> +			InheritedPropertySheets="..\..\WebKitLibraries\win\tools\vsprops\FeatureDefines.vsprops;..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebCoreCommon.vsprops;.\WebCoreCG.vsprops;.\WebCoreCFNetwork.vsprops;.\WebCorePthreads.vsprops;.\WebCoreMediaQT.vsprops;.\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="&quot;$(ProjectDir)..&quot;;&quot;$(ProjectDir)..\accessibility&quot;;&quot;$(ProjectDir)..\accessibility\win&quot;;&quot;$(ProjectDir)..\bridge&quot;;&quot;$(ProjectDir)..\bridge\c&quot;;&quot;$(ProjectDir)..\bridge\jsc&quot;;&quot;$(ProjectDir)..\css&quot;;&quot;$(ProjectDir)..\editing&quot;;&quot;$(ProjectDir)..\fileapi&quot;;&quot;$(ProjectDir)..\rendering&quot;;&quot;$(ProjectDir)..\rendering\style&quot;;&quot;$(ProjectDir)..\rendering\svg&quot;;&quot;$(ProjectDir)..\bindings&quot;;&quot;$(ProjectDir)..\bindings\generic&quot;;&quot;$(ProjectDir)..\bindings\js&quot;;&quot;$(ProjectDir)..\bindings\js\specialization&quot;;&quot;$(ProjectDir)..\dom&quot;;&quot;$(ProjectDir)..\dom\default&quot;;&quot;$(ProjectDir)..\history&quot;;&quot;$(ProjectDir)..\html&quot;;&quot;$(ProjectDir)..\html\canvas&quot;;&quot;$(ProjectDir)..\html\parser&quot;;&quot;$(ProjectDir)..\html\shadow&quot;;&quot;$(ProjectDir)..\inspector&quot;;&quot;$(ProjectDir)..\loader&quot;;&quot;$(ProjectDir)..\loader\appcache&quot;;&quot;$(ProjectDir)..\loader\archive&quot;;&quot;$(ProjectDir)..\loader\archive\cf&quot;;&quot;$(ProjectDir)..\loader\cache&quot;;&quot;$(ProjectDir)..\loader\icon&quot;;&quot;$(ProjectDir)..\mathml&quot;;&quot;$(ProjectDir)..\notifications&quot;;&quot;$(ProjectDir)..\page&quot;;&quot;$(ProjectDir)..\page\animation&quot;;&quot;$(ProjectDir)..\page\win&quot;;&quot;$(ProjectDir)..\platform&quot;;&quot;$(ProjectDir)..\platform\animation&quot;;&quot;$(ProjectDir)..\platform\mock&quot;;&quot;$(ProjectDir)..\platform\sql&quot;;&quot;$(ProjectDir)..\platform\win&quot;;&quot;$(ProjectDir)..\platform\network&quot;;&quot;$(ProjectDir)..\platform\network\win&quot;;&quot;$(ProjectDir)..\platform\cf&quot;;&quot;$(ProjectDir)..\platform\graphics&quot;;&quot;$(ProjectDir)..\platform\graphics\filters&quot;;&quot;$(ProjectDir)..\platform\graphics\opentype&quot;;&quot;$(ProjectDir)..\platform\graphics\transforms&quot;;&quot;$(ProjectDir)..\platform\text&quot;;&quot;$(ProjectDir)..\platform\text\transcoder&quot;;&quot;$(ProjectDir)..\platform\graphics\win&quot;;&quot;$(ProjectDir)..\xml&quot;;&quot;$(ConfigurationBuildDir)\obj\WebCore\DerivedSources&quot;;&quot;$(ProjectDir)..\plugins&quot;;&quot;$(ProjectDir)..\plugins\win&quot;;&quot;$(ProjectDir)..\svg\animation&quot;;&quot;$(ProjectDir)..\svg\graphics&quot;;&quot;$(ProjectDir)..\svg\properties&quot;;&quot;$(ProjectDir)..\svg\graphics\filters&quot;;&quot;$(ProjectDir)..\svg&quot;;&quot;$(ProjectDir)..\wml&quot;;&quot;$(ProjectDir)..\storage&quot;;&quot;$(ProjectDir)..\websockets&quot;;&quot;$(ProjectDir)..\workers&quot;;&quot;$(ConfigurationBuildDir)\include&quot;;&quot;$(ConfigurationBuildDir)\include\private&quot;;&quot;$(ConfigurationBuildDir)\include\JavaScriptCore&quot;;&quot;$(ConfigurationBuildDir)\include\private\JavaScriptCore&quot;;&quot;$(ProjectDir)..\ForwardingHeaders&quot;;&quot;$(WebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include\private\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\pthreads&quot;;&quot;$(WebKitLibrariesDir)\include\sqlite&quot;;&quot;$(WebKitLibrariesDir)\include\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\zlib&quot;"
> +		AdditionalIncludeDirectories="&quot;$(ProjectDir)..&quot;;&quot;$(ProjectDir)..\accessibility&quot;;&quot;$(ProjectDir)..\accessibility\win&quot;;&quot;$(ProjectDir)..\bridge&quot;;&quot;$(ProjectDir)..\bridge\c&quot;;&quot;$(ProjectDir)..\bridge\jsc&quot;;&quot;$(ProjectDir)..\css&quot;;&quot;$(ProjectDir)..\editing&quot;;&quot;$(ProjectDir)..\fileapi&quot;;&quot;$(ProjectDir)..\rendering&quot;;&quot;$(ProjectDir)..\rendering\style&quot;;&quot;$(ProjectDir)..\rendering\svg&quot;;&quot;$(ProjectDir)..\bindings&quot;;&quot;$(ProjectDir)..\bindings\generic&quot;;&quot;$(ProjectDir)..\bindings\js&quot;;&quot;$(ProjectDir)..\bindings\js\specialization&quot;;&quot;$(ProjectDir)..\dom&quot;;&quot;$(ProjectDir)..\dom\default&quot;;&quot;$(ProjectDir)..\history&quot;;&quot;$(ProjectDir)..\html&quot;;&quot;$(ProjectDir)..\html\canvas&quot;;&quot;$(ProjectDir)..\html\parser&quot;;&quot;$(ProjectDir)..\html\shadow&quot;;&quot;$(ProjectDir)..\inspector&quot;;&quot;$(ProjectDir)..\loader&quot;;&quot;$(ProjectDir)..\loader\appcache&quot;;&quot;$(ProjectDir)..\loader\archive&quot;;&quot;$(ProjectDir)..\loader\archive\cf&quot;;&quot;$(ProjectDir)..\loader\cache&quot;;&quot;$(ProjectDir)..\loader\icon&quot;;&quot;$(ProjectDir)..\mathml&quot;;&quot;$(ProjectDir)..\notifications&quot;;&quot;$(ProjectDir)..\page&quot;;&quot;$(ProjectDir)..\page\animation&quot;;&quot;$(ProjectDir)..\page\win&quot;;&quot;$(ProjectDir)..\platform&quot;;&quot;$(ProjectDir)..\platform\animation&quot;;&quot;$(ProjectDir)..\platform\mock&quot;;&quot;$(ProjectDir)..\platform\sql&quot;;&quot;$(ProjectDir)..\platform\win&quot;;&quot;$(ProjectDir)..\platform\network&quot;;&quot;$(ProjectDir)..\platform\network\win&quot;;&quot;$(ProjectDir)..\platform\cf&quot;;&quot;$(ProjectDir)..\platform\graphics&quot;;&quot;$(ProjectDir)..\platform\graphics\ca&quot;;&quot;$(ProjectDir)..\platform\graphics\filters&quot;;&quot;$(ProjectDir)..\platform\graphics\opentype&quot;;&quot;$(ProjectDir)..\platform\graphics\transforms&quot;;&quot;$(ProjectDir)..\platform\text&quot;;&quot;$(ProjectDir)..\platform\text\transcoder&quot;;&quot;$(ProjectDir)..\platform\graphics\win&quot;;&quot;$(ProjectDir)..\xml&quot;;&quot;$(ConfigurationBuildDir)\obj\WebCore\DerivedSources&quot;;&quot;$(ProjectDir)..\plugins&quot;;&quot;$(ProjectDir)..\plugins\win&quot;;&quot;$(ProjectDir)..\svg\animation&quot;;&quot;$(ProjectDir)..\svg\graphics&quot;;&quot;$(ProjectDir)..\svg\properties&quot;;&quot;$(ProjectDir)..\svg\graphics\filters&quot;;&quot;$(ProjectDir)..\svg&quot;;&quot;$(ProjectDir)..\wml&quot;;&quot;$(ProjectDir)..\storage&quot;;&quot;$(ProjectDir)..\websockets&quot;;&quot;$(ProjectDir)..\workers&quot;;&quot;$(ConfigurationBuildDir)\include&quot;;&quot;$(ConfigurationBuildDir)\include\private&quot;;&quot;$(ConfigurationBuildDir)\include\JavaScriptCore&quot;;&quot;$(ConfigurationBuildDir)\include\private\JavaScriptCore&quot;;&quot;$(ProjectDir)..\ForwardingHeaders&quot;;&quot;$(WebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include\private\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\pthreads&quot;;&quot;$(WebKitLibrariesDir)\include\sqlite&quot;;&quot;$(WebKitLibrariesDir)\include\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\zlib&quot;"
>  		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?

[Simon Fraser (smfr)]

Comment on attachment 78200 [details]
Final Patch

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\win\tools\vsprops\FeatureDefines.vsprops;$(WebKitVSPropsRedirectionDir)..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;$(WebKitVSPropsRedirectionDir)..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebCoreCommon.vsprops;.\WebCoreCG.vsprops;.\WebCoreCFNetwork.vsprops;.\WebCorePthreads.vsprops;.\WebCoreMediaQT.vsprops;.\WebCoreQuartzCore.vsprops"
> +			InheritedPropertySheets="..\..\WebKitLibraries\win\tools\vsprops\FeatureDefines.vsprops;..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebCoreCommon.vsprops;.\WebCoreCG.vsprops;.\WebCoreCFNetwork.vsprops;.\WebCorePthreads.vsprops;.\WebCoreMediaQT.vsprops;.\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="&quot;$(ProjectDir)..&quot;;&quot;$(ProjectDir)..\accessibility&quot;;&quot;$(ProjectDir)..\accessibility\win&quot;;&quot;$(ProjectDir)..\bridge&quot;;&quot;$(ProjectDir)..\bridge\c&quot;;&quot;$(ProjectDir)..\bridge\jsc&quot;;&quot;$(ProjectDir)..\css&quot;;&quot;$(ProjectDir)..\editing&quot;;&quot;$(ProjectDir)..\fileapi&quot;;&quot;$(ProjectDir)..\rendering&quot;;&quot;$(ProjectDir)..\rendering\style&quot;;&quot;$(ProjectDir)..\rendering\svg&quot;;&quot;$(ProjectDir)..\bindings&quot;;&quot;$(ProjectDir)..\bindings\generic&quot;;&quot;$(ProjectDir)..\bindings\js&quot;;&quot;$(ProjectDir)..\bindings\js\specialization&quot;;&quot;$(ProjectDir)..\dom&quot;;&quot;$(ProjectDir)..\dom\default&quot;;&quot;$(ProjectDir)..\history&quot;;&quot;$(ProjectDir)..\html&quot;;&quot;$(ProjectDir)..\html\canvas&quot;;&quot;$(ProjectDir)..\html\parser&quot;;&quot;$(ProjectDir)..\html\shadow&quot;;&quot;$(ProjectDir)..\inspector&quot;;&quot;$(ProjectDir)..\loader&quot;;&quot;$(ProjectDir)..\loader\appcache&quot;;&quot;$(ProjectDir)..\loader\archive&quot;;&quot;$(ProjectDir)..\loader\archive\cf&quot;;&quot;$(ProjectDir)..\loader\cache&quot;;&quot;$(ProjectDir)..\loader\icon&quot;;&quot;$(ProjectDir)..\mathml&quot;;&quot;$(ProjectDir)..\notifications&quot;;&quot;$(ProjectDir)..\page&quot;;&quot;$(ProjectDir)..\page\animation&quot;;&quot;$(ProjectDir)..\page\win&quot;;&quot;$(ProjectDir)..\platform&quot;;&quot;$(ProjectDir)..\platform\animation&quot;;&quot;$(ProjectDir)..\platform\mock&quot;;&quot;$(ProjectDir)..\platform\sql&quot;;&quot;$(ProjectDir)..\platform\win&quot;;&quot;$(ProjectDir)..\platform\network&quot;;&quot;$(ProjectDir)..\platform\network\win&quot;;&quot;$(ProjectDir)..\platform\cf&quot;;&quot;$(ProjectDir)..\platform\graphics&quot;;&quot;$(ProjectDir)..\platform\graphics\filters&quot;;&quot;$(ProjectDir)..\platform\graphics\opentype&quot;;&quot;$(ProjectDir)..\platform\graphics\transforms&quot;;&quot;$(ProjectDir)..\platform\text&quot;;&quot;$(ProjectDir)..\platform\text\transcoder&quot;;&quot;$(ProjectDir)..\platform\graphics\win&quot;;&quot;$(ProjectDir)..\xml&quot;;&quot;$(ConfigurationBuildDir)\obj\WebCore\DerivedSources&quot;;&quot;$(ProjectDir)..\plugins&quot;;&quot;$(ProjectDir)..\plugins\win&quot;;&quot;$(ProjectDir)..\svg\animation&quot;;&quot;$(ProjectDir)..\svg\graphics&quot;;&quot;$(ProjectDir)..\svg\properties&quot;;&quot;$(ProjectDir)..\svg\graphics\filters&quot;;&quot;$(ProjectDir)..\svg&quot;;&quot;$(ProjectDir)..\wml&quot;;&quot;$(ProjectDir)..\storage&quot;;&quot;$(ProjectDir)..\websockets&quot;;&quot;$(ProjectDir)..\workers&quot;;&quot;$(ConfigurationBuildDir)\include&quot;;&quot;$(ConfigurationBuildDir)\include\private&quot;;&quot;$(ConfigurationBuildDir)\include\JavaScriptCore&quot;;&quot;$(ConfigurationBuildDir)\include\private\JavaScriptCore&quot;;&quot;$(ProjectDir)..\ForwardingHeaders&quot;;&quot;$(WebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include\private\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\pthreads&quot;;&quot;$(WebKitLibrariesDir)\include\sqlite&quot;;&quot;$(WebKitLibrariesDir)\include\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\zlib&quot;"
> +		AdditionalIncludeDirectories="&quot;$(ProjectDir)..&quot;;&quot;$(ProjectDir)..\accessibility&quot;;&quot;$(ProjectDir)..\accessibility\win&quot;;&quot;$(ProjectDir)..\bridge&quot;;&quot;$(ProjectDir)..\bridge\c&quot;;&quot;$(ProjectDir)..\bridge\jsc&quot;;&quot;$(ProjectDir)..\css&quot;;&quot;$(ProjectDir)..\editing&quot;;&quot;$(ProjectDir)..\fileapi&quot;;&quot;$(ProjectDir)..\rendering&quot;;&quot;$(ProjectDir)..\rendering\style&quot;;&quot;$(ProjectDir)..\rendering\svg&quot;;&quot;$(ProjectDir)..\bindings&quot;;&quot;$(ProjectDir)..\bindings\generic&quot;;&quot;$(ProjectDir)..\bindings\js&quot;;&quot;$(ProjectDir)..\bindings\js\specialization&quot;;&quot;$(ProjectDir)..\dom&quot;;&quot;$(ProjectDir)..\dom\default&quot;;&quot;$(ProjectDir)..\history&quot;;&quot;$(ProjectDir)..\html&quot;;&quot;$(ProjectDir)..\html\canvas&quot;;&quot;$(ProjectDir)..\html\parser&quot;;&quot;$(ProjectDir)..\html\shadow&quot;;&quot;$(ProjectDir)..\inspector&quot;;&quot;$(ProjectDir)..\loader&quot;;&quot;$(ProjectDir)..\loader\appcache&quot;;&quot;$(ProjectDir)..\loader\archive&quot;;&quot;$(ProjectDir)..\loader\archive\cf&quot;;&quot;$(ProjectDir)..\loader\cache&quot;;&quot;$(ProjectDir)..\loader\icon&quot;;&quot;$(ProjectDir)..\mathml&quot;;&quot;$(ProjectDir)..\notifications&quot;;&quot;$(ProjectDir)..\page&quot;;&quot;$(ProjectDir)..\page\animation&quot;;&quot;$(ProjectDir)..\page\win&quot;;&quot;$(ProjectDir)..\platform&quot;;&quot;$(ProjectDir)..\platform\animation&quot;;&quot;$(ProjectDir)..\platform\mock&quot;;&quot;$(ProjectDir)..\platform\sql&quot;;&quot;$(ProjectDir)..\platform\win&quot;;&quot;$(ProjectDir)..\platform\network&quot;;&quot;$(ProjectDir)..\platform\network\win&quot;;&quot;$(ProjectDir)..\platform\cf&quot;;&quot;$(ProjectDir)..\platform\graphics&quot;;&quot;$(ProjectDir)..\platform\graphics\ca&quot;;&quot;$(ProjectDir)..\platform\graphics\filters&quot;;&quot;$(ProjectDir)..\platform\graphics\opentype&quot;;&quot;$(ProjectDir)..\platform\graphics\transforms&quot;;&quot;$(ProjectDir)..\platform\text&quot;;&quot;$(ProjectDir)..\platform\text\transcoder&quot;;&quot;$(ProjectDir)..\platform\graphics\win&quot;;&quot;$(ProjectDir)..\xml&quot;;&quot;$(ConfigurationBuildDir)\obj\WebCore\DerivedSources&quot;;&quot;$(ProjectDir)..\plugins&quot;;&quot;$(ProjectDir)..\plugins\win&quot;;&quot;$(ProjectDir)..\svg\animation&quot;;&quot;$(ProjectDir)..\svg\graphics&quot;;&quot;$(ProjectDir)..\svg\properties&quot;;&quot;$(ProjectDir)..\svg\graphics\filters&quot;;&quot;$(ProjectDir)..\svg&quot;;&quot;$(ProjectDir)..\wml&quot;;&quot;$(ProjectDir)..\storage&quot;;&quot;$(ProjectDir)..\websockets&quot;;&quot;$(ProjectDir)..\workers&quot;;&quot;$(ConfigurationBuildDir)\include&quot;;&quot;$(ConfigurationBuildDir)\include\private&quot;;&quot;$(ConfigurationBuildDir)\include\JavaScriptCore&quot;;&quot;$(ConfigurationBuildDir)\include\private\JavaScriptCore&quot;;&quot;$(ProjectDir)..\ForwardingHeaders&quot;;&quot;$(WebKitLibrariesDir)\include&quot;;&quot;$(WebKitLibrariesDir)\include\private&quot;;&quot;$(WebKitLibrariesDir)\include\private\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\pthreads&quot;;&quot;$(WebKitLibrariesDir)\include\sqlite&quot;;&quot;$(WebKitLibrariesDir)\include\JavaScriptCore&quot;;&quot;$(WebKitLibrariesDir)\include\zlib&quot;"
>  		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?

[Darin Adler]

The Windows build has been failing since this was checked in.

On example <http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/10209/steps/compile-webkit/logs/stdio>.


MediaPlayerPrivateQuickTimeVisualContext.cpp
..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(1193) : error C2027: use of undefined type '_CACFLayer'
        c:\cygwin\home\buildbot\slave\win-release\build\WebCore\platform\graphics\GraphicsLayer.h(56) : see declaration of '_CACFLayer'
..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(1193) : error C2227: left of '->setLayoutClient' must point to class/struct/union/generic type
..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(1212) : error C2027: use of undefined type '_CACFLayer'
        c:\cygwin\home\buildbot\slave\win-release\build\WebCore\platform\graphics\GraphicsLayer.h(56) : see declaration of '_CACFLayer'
..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(1212) : error C2227: left of '->addSublayer' must point to class/struct/union/generic type
..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(1213) : error C2027: use of undefined type '_CACFLayer'
        c:\cygwin\home\buildbot\slave\win-release\build\WebCore\platform\graphics\GraphicsLayer.h(56) : see declaration of '_CACFLayer'
..\platform\graphics\win\MediaPlayerPrivateQuickTimeVisualContext.cpp(1213) : error C2227: left of '->setNeedsLayout' must point to class/struct/union/generic type
MediaPlayerPrivateQuickTimeWin.cpp
..\platform\graphics\win\MediaPlayerPrivateQuickTimeWin.cpp(763) : error C2027: use of undefined type '_CACFLayer'
        c:\cygwin\home\buildbot\slave\win-release\build\WebCore\platform\graphics\GraphicsLayer.h(56) : see declaration of '_CACFLayer'
..\platform\graphics\win\MediaPlayerPrivateQuickTimeWin.cpp(763) : error C2227: left of '->setNeedsDisplay' must point to class/struct/union/generic type
GraphicsLayerCACF.cpp
..\platform\graphics\win\GraphicsLayerCACF.cpp(375) : error C2679: binary '=' : no operator found which takes a right-hand operand of type 'PlatformLayer *' (or there is no acceptable conversion)
        c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\private\javascriptcore\RefPtr.h(73): could be 'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(const WTF::RefPtr<T> &)'
        with
        [
            T=WebCore::WKCACFLayer
        ]
        c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\private\javascriptcore\RefPtr.h(74): or 'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(T *)'
        with
        [
            T=WebCore::WKCACFLayer
        ]
        c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\private\javascriptcore\RefPtr.h(75): or 'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(const WTF::PassRefPtr<T> &)'
        with
        [
            T=WebCore::WKCACFLayer
        ]
        c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\private\javascriptcore\RefPtr.h(76): or 'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(const WTF::NonNullPassRefPtr<T> &)'
        with
        [
            T=WebCore::WKCACFLayer
        ]
        c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\private\javascriptcore\RefPtr.h(78): or 'WTF::RefPtr<T> &WTF::RefPtr<T>::operator =(std::nullptr_t)'
        with
        [
            T=WebCore::WKCACFLayer
        ]
        while trying to match the argument list '(WTF::RefPtr<T>, PlatformLayer *)'
        with
        [
            T=WebCore::WKCACFLayer
        ]
..\platform\graphics\win\GraphicsLayerCACF.cpp(385) : error C2556: 'PlatformLayer *WebCore::GraphicsLayerCACF::hostLayerForSublayers(void) const' : overloaded function differs only by return type from 'WebCore::WKCACFLayer *WebCore::GraphicsLayerCACF::hostLayerForSublayers(void) const'
        c:\cygwin\home\buildbot\slave\win-release\build\webcore\platform\graphics\win\GraphicsLayerCACF.h(93) : see declaration of 'WebCore::GraphicsLayerCACF::hostLayerForSublayers'
..\platform\graphics\win\GraphicsLayerCACF.cpp(385) : error C2371: 'WebCore::GraphicsLayerCACF::hostLayerForSublayers' : redefinition; different basic types
        c:\cygwin\home\buildbot\slave\win-release\build\webcore\platform\graphics\win\GraphicsLayerCACF.h(93) : see declaration of 'WebCore::GraphicsLayerCACF::hostLayerForSublayers'
..\platform\graphics\win\GraphicsLayerCACF.cpp(390) : error C2556: 'PlatformLayer *WebCore::GraphicsLayerCACF::layerForSuperlayer(void) const' : overloaded function differs only by return type from 'WebCore::WKCACFLayer *WebCore::GraphicsLayerCACF::layerForSuperlayer(void) const'
        c:\cygwin\home\buildbot\slave\win-release\build\webcore\platform\graphics\win\GraphicsLayerCACF.h(94) : see declaration of 'WebCore::GraphicsLayerCACF::layerForSuperlayer'
..\platform\graphics\win\GraphicsLayerCACF.cpp(390) : error C2371: 'WebCore::GraphicsLayerCACF::layerForSuperlayer' : redefinition; different basic types
        c:\cygwin\home\buildbot\slave\win-release\build\webcore\platform\graphics\win\GraphicsLayerCACF.h(94) : see declaration of 'WebCore::GraphicsLayerCACF::layerForSuperlayer'
..\platform\graphics\win\GraphicsLayerCACF.cpp(396) : error C2440: 'return' : cannot convert from 'WebCore::WKCACFLayer *' to 'PlatformLayer *'
        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
..\platform\graphics\win\GraphicsLayerCACF.cpp(493) : error C2264: 'WebCore::GraphicsLayerCACF::layerForSuperlayer' : error in function definition or declaration; function not called

[Darin Adler]

I wanted to fix this, but I could not figure out exactly how.

[Chris Marrin]

Thanks for trying Darin. I've been trying to get the Windows side of this checked in for the last several hours. I am almost there, but am now stuck because DRT is crashing on every test, even those having nothing to do with compositing. I don't understand the errors being spewed so I will try again tomorrow. It probably just needs a clean rebuild.

For now I am back the change out. Sorry for the delay...

[Chris Marrin]

Backed out latest change in http://trac.webkit.org/changeset/75227. Will fix the Windows side and then resubmit.

[Adam Roben (:aroben)]

Comment on attachment 78200 [details]
Final Patch

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

You should make sure that the new CACFAnimation stuff you use is all present in WebKitSupportLibrary.

> WebCore/platform/graphics/win/WKCACFLayerRenderer.cpp:538
> +    // If timeToNextRender is not infinity, it means animations are running, so queue up to render again
> +    if (timeToNextRender != numeric_limits<CFTimeInterval>::infinity())
> +        renderSoon();

It seems wasteful always to render again immediately rather than waiting until timeToNextRender has elapsed.

[Chris Marrin]

Created attachment 78251 [details]
Final Patch

[Simon Fraser (smfr)]

Comment on attachment 78251 [details]
Final Patch

rs=me

[Chris Marrin]

Final patch landed in http://trac.webkit.org/changeset/75262

[Dimitri Glazkov (Google)]

(In reply to comment #68)
> Final patch landed in http://trac.webkit.org/changeset/75262

Broke Chromium compile (forgot an include?): http://build.webkit.org/builders/Chromium%20Linux%20Release/builds/20309/steps/compile-webkit/logs/stdio

[WebKit Review Bot]

http://trac.webkit.org/changeset/75262 might have broken Windows Release (Build), Windows Debug (Build), and Chromium Linux Release

[Chris Marrin]

I no longer see any build breakage related to these patches. Closing