Bug 105553 – [chromium] Convert WebTransformOperations into pure virtual

• Component: New Bugs
• Status: RESOLVED FIXED

[Ali Juma]

[chromium] Convert WebTransformOperations into pure virtual

[Ali Juma]

Created attachment 180366 [details]
Patch

[Ali Juma]

This needs to land after https://codereview.chromium.org/11644004 lands in chromium.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 180366 [details]
Patch

Attachment 180366 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15455098

[Peter Beverloo (cr-android ews)]

Comment on attachment 180366 [details]
Patch

Attachment 180366 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15449151

[James Robinson]

Comment on attachment 180366 [details]
Patch

I think this is the wrong direction, see chromium code review.

[Ali Juma]

Created attachment 181504 [details]
Not for review: combined patch

Posting this patch for discussion. This is combined result of the two WebKit patches described in the plan at https://codereview.chromium.org/11745018/

[James Robinson]

I think you'll want something very much like this patch, but with some tweaks and as a second step in the transition.  See my comments on the chromium codereview.

[Ali Juma]

Created attachment 182575 [details]
Patch

[Ali Juma]

(In reply to comment #9)
> Created an attachment (id=182575) [details]
> Patch

This patch follows the approach discussed in the chromium codereview (https://chromiumcodereview.appspot.com/11745018). It requires https://chromiumcodereview.appspot.com/11876016 to land first.

[WebKit Review Bot]

Comment on attachment 182575 [details]
Patch

Attachment 182575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15874464

[Peter Beverloo (cr-android ews)]

Comment on attachment 182575 [details]
Patch

Attachment 182575 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15868523

[James Robinson]

Comment on attachment 182575 [details]
Patch

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

Since this has a chromium-side dep, make sure that lands and is rolled into Source/WebKit/chromium/DEPS and leave a comment on this bug indicating what the dependency is so folks know.  This patch has some minor issues, but is looking close.

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:60
> +WebTransformOperations* toWebTransformOperations(const TransformOperations& transformOperations, const FloatSize& boxSize)

since the caller is going to put this in an OwnPtr<>, you should make the return type of this function PassOwnPtr<> and put the adoptPtr() call in the initialization of the webTransformOperations local (which you should make OwnPtr<>).  whenever using smart pointers you want to put it into the smart pointer as close to the allocation point as you can so less code is dealing with raw pointers

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:63
> +    WebTransformOperations* webTransformOperations = Platform::current()->compositorSupport()->createTransformOperations();

you should check if this call returns 0 and, if so, return 0 from this function before proceeding. you can't promise here that the platform's compositingSupport will give you something

[Ali Juma]

Created attachment 183018 [details]
Patch

[Ali Juma]

(In reply to comment #13)
> (From update of attachment 182575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182575&action=review
> 
> Since this has a chromium-side dep, make sure that lands and is rolled into Source/WebKit/chromium/DEPS and leave a comment on this bug indicating what the dependency is so folks know.  This patch has some minor issues, but is looking close.

Will do once the chromium-side dep (https://chromiumcodereview.appspot.com/11876016) lands in chromium.

> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:60
> > +WebTransformOperations* toWebTransformOperations(const TransformOperations& transformOperations, const FloatSize& boxSize)
> 
> since the caller is going to put this in an OwnPtr<>, you should make the return type of this function PassOwnPtr<> and put the adoptPtr() call in the initialization of the webTransformOperations local (which you should make OwnPtr<>).  whenever using smart pointers you want to put it into the smart pointer as close to the allocation point as you can so less code is dealing with raw pointers

Done.

> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:63
> > +    WebTransformOperations* webTransformOperations = Platform::current()->compositorSupport()->createTransformOperations();
> 
> you should check if this call returns 0 and, if so, return 0 from this function before proceeding. you can't promise here that the platform's compositingSupport will give you something

Done.

[WebKit Review Bot]

Comment on attachment 183018 [details]
Patch

Attachment 183018 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15907481

[Peter Beverloo (cr-android ews)]

Comment on attachment 183018 [details]
Patch

Attachment 183018 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15913425

[James Robinson]

Comment on attachment 183018 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:65
> +        return PassOwnPtr<WebTransformOperations>();

you can say "nullptr" instead of "PassOwnPtr<Type>()"

[Ali Juma]

Created attachment 183252 [details]
Patch for landing

[Ali Juma]

Comment on attachment 183252 [details]
Patch for landing

This depends on chromium revision r177290, which has already rolled into Source/WebKit/chromium/DEPS.

[WebKit Review Bot]

Comment on attachment 183252 [details]
Patch for landing

Clearing flags on attachment: 183252

Committed r140184: <http://trac.webkit.org/changeset/140184>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.