Bug 49388 - Share code between Mac (CA) and Windows (CACF) GraphicsLayer implementations
Summary: Share code between Mac (CA) and Windows (CACF) GraphicsLayer implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-11 10:02 PST by Chris Marrin
Modified: 2011-01-10 15:13 PST (History)
10 users (show)

See Also:


Attachments
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.* (266.15 KB, patch)
2010-11-11 11:52 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch with Mac Implementation (246.24 KB, patch)
2010-12-02 08:59 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Fix merge issue with last patch (247.11 KB, patch)
2010-12-02 10:52 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch fixing issues from the review and add 2 files (285.24 KB, patch)
2010-12-02 15:43 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Please do not review. Just submitting to see the errors on the bots (287.93 KB, patch)
2010-12-03 11:57 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Fixed merge conflicts (287.88 KB, patch)
2010-12-03 13:01 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (291.23 KB, patch)
2010-12-03 16:13 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (16.16 KB, patch)
2010-12-07 14:09 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (19.04 KB, patch)
2010-12-07 14:29 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch with some housekeeping changes (19.17 KB, patch)
2010-12-07 14:57 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Resubmission of last patch with build fixes (21.29 KB, patch)
2010-12-08 12:12 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch getting rid of NonZeroBeginTimeFlag (4.58 KB, patch)
2010-12-09 17:36 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch adding CACFContextGetLastCommitTime to WKSI (623.73 KB, patch)
2010-12-10 11:30 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file) (623.51 KB, patch)
2010-12-10 11:33 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (38.19 KB, patch)
2011-01-06 13:36 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (119.33 KB, patch)
2011-01-06 18:12 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (108.11 KB, patch)
2011-01-06 18:28 PST, Chris Marrin
no flags Details | Formatted Diff | Diff
Final Patch (122.38 KB, patch)
2011-01-06 18:33 PST, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff
Final Patch (152.70 KB, patch)
2011-01-07 11:35 PST, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2010-11-11 10:02:52 PST
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.
Comment 1 Vangelis Kokkevis 2010-11-11 10:12:39 PST
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.
Comment 2 Chris Marrin 2010-11-11 11:52:09 PST
Created attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*
Comment 3 Simon Fraser (smfr) 2010-11-11 11:54:25 PST
Comment on attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*

The GraphicsLayerCA class needs to renamed too (hint: Edit All in Scope).
Comment 4 Simon Fraser (smfr) 2010-11-11 13:05:11 PST
Comment on attachment 73632 [details]
Patch changing name of GraphicsLayerCA.* to GraphicsLayerMac.*

I was confused by SVN.
Comment 5 Chris Marrin 2010-11-11 13:11:00 PST
Landed rename part in http://trac.webkit.org/changeset/71848
Comment 6 Chris Marrin 2010-11-11 13:37:38 PST
(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.
Comment 7 WebKit Review Bot 2010-11-11 14:03:19 PST
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
Comment 8 Eric Seidel (no email) 2010-11-11 16:24:07 PST
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.
Comment 9 Chris Marrin 2010-12-02 08:59:58 PST
Created attachment 75382 [details]
Patch with Mac Implementation
Comment 10 Chris Marrin 2010-12-02 10:52:40 PST
Created attachment 75394 [details]
Fix merge issue with last patch
Comment 11 Early Warning System Bot 2010-12-02 11:07:19 PST
Attachment 75394 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6801015
Comment 12 WebKit Review Bot 2010-12-02 11:13:26 PST
Attachment 75394 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6826009
Comment 13 Simon Fraser (smfr) 2010-12-02 11:21:03 PST
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.
Comment 14 Build Bot 2010-12-02 11:41:05 PST
Attachment 75394 [details] did not build on win:
Build output: http://queues.webkit.org/results/6839009
Comment 15 Eric Seidel (no email) 2010-12-02 12:28:19 PST
Attachment 75394 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6754009
Comment 16 Chris Marrin 2010-12-02 15:43:03 PST
Created attachment 75422 [details]
Patch fixing issues from the review and add 2 files
Comment 17 Chris Marrin 2010-12-02 15:45:10 PST
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.
Comment 18 Chris Marrin 2010-12-02 15:46:21 PST
This latest patch also fixes build errors in Qt, Win, and Chrome builds
Comment 19 Simon Fraser (smfr) 2010-12-02 15:52:34 PST
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.
Comment 20 Simon Fraser (smfr) 2010-12-02 21:02:24 PST
This was rolled out because of leopard build issues. I tried to fix them, but fixes were non-obvious.
Comment 21 Eric Seidel (no email) 2010-12-03 00:24:05 PST
Attachment 75422 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6851007
Comment 22 Chris Marrin 2010-12-03 11:57:20 PST
Created attachment 75522 [details]
Please do not review. Just submitting to see the errors on the bots
Comment 23 Chris Marrin 2010-12-03 13:01:29 PST
Created attachment 75530 [details]
Fixed merge conflicts
Comment 24 Chris Marrin 2010-12-03 16:13:11 PST
Created attachment 75575 [details]
Patch
Comment 25 Chris Marrin 2010-12-06 16:06:07 PST
Comment on attachment 75575 [details]
Patch

This patch landed with http://trac.webkit.org/changeset/73380
Comment 26 Chris Marrin 2010-12-07 14:09:27 PST
Created attachment 75842 [details]
Patch
Comment 27 Simon Fraser (smfr) 2010-12-07 14:13:25 PST
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?
Comment 28 Chris Marrin 2010-12-07 14:29:51 PST
Created attachment 75844 [details]
Patch
Comment 29 Chris Marrin 2010-12-07 14:57:10 PST
Created attachment 75846 [details]
Patch with some housekeeping changes
Comment 30 Simon Fraser (smfr) 2010-12-07 15:15:09 PST
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?
Comment 31 Chris Marrin 2010-12-07 15:51:32 PST
(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
Comment 32 WebKit Review Bot 2010-12-07 21:53:54 PST
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.
Comment 33 WebKit Review Bot 2010-12-07 21:54:53 PST
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.
Comment 34 WebKit Review Bot 2010-12-07 21:55:54 PST
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.
Comment 35 Chris Marrin 2010-12-08 12:12:58 PST
Created attachment 75943 [details]
Resubmission of last patch with build fixes
Comment 36 Chris Marrin 2010-12-08 12:56:25 PST
relanded housekeeping patch in http://trac.webkit.org/changeset/73540
Comment 37 Chris Marrin 2010-12-09 17:36:34 PST
Created attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag
Comment 38 Chris Marrin 2010-12-09 17:38:49 PST
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.
Comment 39 Simon Fraser (smfr) 2010-12-09 17:43:07 PST
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.
Comment 40 Chris Marrin 2010-12-10 11:28:55 PST
Comment on attachment 76144 [details]
Patch getting rid of NonZeroBeginTimeFlag

Landed in http://trac.webkit.org/changeset/73731
Comment 41 Chris Marrin 2010-12-10 11:30:35 PST
Created attachment 76223 [details]
Patch adding CACFContextGetLastCommitTime to WKSI
Comment 42 Chris Marrin 2010-12-10 11:33:08 PST
Created attachment 76225 [details]
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file)
Comment 43 Adam Roben (:aroben) 2010-12-10 11:52:39 PST
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.
Comment 44 Chris Marrin 2010-12-10 13:06:49 PST
Comment on attachment 76225 [details]
Patch adding CACFContextGetLastCommitTime to WKSI (gets rid of bogus .lib file)

Landed in http://trac.webkit.org/changeset/73790
Comment 45 Eric Seidel (no email) 2010-12-14 01:27:32 PST
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.
Comment 46 Eric Seidel (no email) 2010-12-14 01:27:40 PST
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.
Comment 47 Eric Seidel (no email) 2010-12-14 01:27:45 PST
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.
Comment 48 Eric Seidel (no email) 2010-12-14 01:27:56 PST
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.
Comment 49 Chris Marrin 2011-01-06 13:36:02 PST
Created attachment 78154 [details]
Patch
Comment 50 Simon Fraser (smfr) 2011-01-06 14:04:08 PST
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!
Comment 51 Simon Fraser (smfr) 2011-01-06 14:07:28 PST
Comment on attachment 78154 [details]
Patch

Ah, this is Mac code. r=me
Comment 52 Build Bot 2011-01-06 14:18:07 PST
Attachment 78154 [details] did not build on win:
Build output: http://queues.webkit.org/results/7402011
Comment 53 Chris Marrin 2011-01-06 14:27:00 PST
Committed r75199: <http://trac.webkit.org/changeset/75199>
Comment 54 Chris Marrin 2011-01-06 14:30:57 PST
Not quite done yet. Hopefully the next patch will be the last. It will have the Windows side of all this
Comment 55 WebKit Review Bot 2011-01-06 14:49:34 PST
http://trac.webkit.org/changeset/75199 might have broken Windows Release (Build) and Windows Debug (Build)
Comment 56 Chris Marrin 2011-01-06 18:12:23 PST
Created attachment 78196 [details]
Patch
Comment 57 Chris Marrin 2011-01-06 18:28:44 PST
Created attachment 78197 [details]
Patch
Comment 58 Chris Marrin 2011-01-06 18:33:05 PST
Created attachment 78200 [details]
Final Patch
Comment 59 Simon Fraser (smfr) 2011-01-06 18:37:09 PST
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?
Comment 60 Simon Fraser (smfr) 2011-01-06 18:37:50 PST
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?
Comment 61 Darin Adler 2011-01-06 20:27:19 PST
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
Comment 62 Darin Adler 2011-01-06 20:27:36 PST
I wanted to fix this, but I could not figure out exactly how.
Comment 63 Chris Marrin 2011-01-06 20:32:58 PST
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...
Comment 64 Chris Marrin 2011-01-06 20:54:51 PST
Backed out latest change in http://trac.webkit.org/changeset/75227. Will fix the Windows side and then resubmit.
Comment 65 Adam Roben (:aroben) 2011-01-07 05:28:31 PST
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.
Comment 66 Chris Marrin 2011-01-07 11:35:24 PST
Created attachment 78251 [details]
Final Patch
Comment 67 Simon Fraser (smfr) 2011-01-07 11:40:24 PST
Comment on attachment 78251 [details]
Final Patch

rs=me
Comment 68 Chris Marrin 2011-01-07 12:01:09 PST
Final patch landed in http://trac.webkit.org/changeset/75262
Comment 69 Dimitri Glazkov (Google) 2011-01-07 12:46:09 PST
(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
Comment 70 WebKit Review Bot 2011-01-07 13:36:03 PST
http://trac.webkit.org/changeset/75262 might have broken Windows Release (Build), Windows Debug (Build), and Chromium Linux Release
Comment 71 Chris Marrin 2011-01-10 15:13:08 PST
I no longer see any build breakage related to these patches. Closing