Summary: | [chromium] Convert WebTransformOperations into pure virtual | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ali Juma <ajuma> | ||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, cc-bugs, dglazkov, fishd, jamesr, peter+ews, tkent+wkapi, vollick, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Ali Juma
2012-12-20 10:44:09 PST
Created attachment 180366 [details]
Patch
This needs to land after https://codereview.chromium.org/11644004 lands in chromium. Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 180366 [details] Patch Attachment 180366 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15455098 Comment on attachment 180366 [details] Patch Attachment 180366 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15449151 Comment on attachment 180366 [details]
Patch
I think this is the wrong direction, see chromium code review.
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/ 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. Created attachment 182575 [details]
Patch
(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. Comment on attachment 182575 [details] Patch Attachment 182575 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15874464 Comment on attachment 182575 [details] Patch Attachment 182575 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15868523 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 Created attachment 183018 [details]
Patch
(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. Comment on attachment 183018 [details] Patch Attachment 183018 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15907481 Comment on attachment 183018 [details] Patch Attachment 183018 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15913425 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>()" Created attachment 183252 [details]
Patch for landing
Comment on attachment 183252 [details] Patch for landing This depends on chromium revision r177290, which has already rolled into Source/WebKit/chromium/DEPS. Comment on attachment 183252 [details] Patch for landing Clearing flags on attachment: 183252 Committed r140184: <http://trac.webkit.org/changeset/140184> All reviewed patches have been landed. Closing bug. |