Bug 235311

Created attachment 449371 [details] Patch

Looks like there are issues in the CoreAnimation implementation. I'm investigating those.

I'm liking the WPT result changes!

<rdar://problem/88012700>

I think I've worked out the changes necessary for making this work with the CoreGraphics bits and I hope to have a new version up in a few days.

Created attachment 450118 [details] Patch

Created attachment 450123 [details] Patch

Created attachment 450131 [details] Patch

Comment on attachment 450131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450131&action=review > Source/WebCore/platform/graphics/GraphicsLayer.cpp:732 > + Vector<TransformOperation::OperationType> sharedPrimitives; You could reserveInitialCapacity() here > Source/WebCore/platform/graphics/GraphicsLayer.cpp:741 > + // If we have seen a non-empty list already and this list's size doesn't match, then we can't use shared primitives. > + if (sharedPrimitives.size() && sharedPrimitives.size() != operations.size()) > + return { }; Where does the "extend the list" part of https://www.w3.org/TR/css-transforms-1/#transform-interpolation-length-fixup happen? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3461 > +bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& valueList, const std::optional<Vector<TransformOperation::OperationType>>& operations, const Animation* animation, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool keyframesShouldUseAnimationWideTimingFunction) animationIndex should be unsigned

Comment on attachment 450131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450131&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:670 > + std::optional<Vector<TransformOperation::OperationType>> getSharedPrimitivesForTransformOperations(const KeyframeValueList&); We usually don't prefix things like this with get*().

Comment on attachment 450131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450131&action=review > Source/WebCore/platform/graphics/transforms/TransformOperation.cpp:84 > + static OperationType sharedPrimitives[][2] = { Can this be constexpr?

Thanks for the reviews! (In reply to Simon Fraser (smfr) from comment #9) > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:732 > > + Vector<TransformOperation::OperationType> sharedPrimitives; > > You could reserveInitialCapacity() here Okay. I do this now using the length of the first list of transforms. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:741 > > + // If we have seen a non-empty list already and this list's size doesn't match, then we can't use shared primitives. > > + if (sharedPrimitives.size() && sharedPrimitives.size() != operations.size()) > > + return { }; > > Where does the "extend the list" part of > https://www.w3.org/TR/css-transforms-1/#transform-interpolation-length-fixup > happen? This change doesn't implement support for blending with lists of different sizes. I have a followup change to this one which implements support for that as well as for blending prefixes and then falling back to matrix interpolation for the rest of the list. It requires a bit more work for the CoreAnimation backend and I figured it would be easier to review smaller patches. > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3461 > > +bool GraphicsLayerCA::appendToUncommittedAnimations(const KeyframeValueList& valueList, const std::optional<Vector<TransformOperation::OperationType>>& operations, const Animation* animation, const String& animationName, const FloatSize& boxSize, int animationIndex, Seconds timeOffset, bool keyframesShouldUseAnimationWideTimingFunction) > > animationIndex should be unsigned Fixed. (In reply to Sam Weinig from comment #10) > Comment on attachment 450131 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450131&action=review > > > Source/WebCore/platform/graphics/GraphicsLayer.h:670 > > + std::optional<Vector<TransformOperation::OperationType>> getSharedPrimitivesForTransformOperations(const KeyframeValueList&); > > We usually don't prefix things like this with get*(). Oh, good point. I've fixed this. (In reply to Sam Weinig from comment #11) > Comment on attachment 450131 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450131&action=review > > > Source/WebCore/platform/graphics/transforms/TransformOperation.cpp:84 > > + static OperationType sharedPrimitives[][2] = { > > Can this be constexpr? It can! Fixed.

Created attachment 450179 [details] Patch

Comment on attachment 450179 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450179&action=review > Source/WebCore/platform/graphics/GraphicsLayer.cpp:755 > + for (size_t operationIndex = 0; operationIndex < operations.size(); ++operationIndex) { > + const auto* operation = operations.at(operationIndex); > + if (operationIndex >= sharedPrimitives.size()) { > + ASSERT(operationIndex == sharedPrimitives.size()); > + sharedPrimitives.append(operation->primitiveType()); > + } else { > + auto newPrimitive = operation->sharedPrimitiveType(sharedPrimitives[operationIndex]); > + if (!newPrimitive.has_value()) > + return { }; > + sharedPrimitives[operationIndex] = *newPrimitive; It's hard to reason about this code that deals with a list of lists. I wonder if breaking out the inner loop into a lambda would make it easier to read.

Created attachment 450212 [details] Patch

(In reply to Simon Fraser (smfr) from comment #14) > Comment on attachment 450179 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450179&action=review > > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:755 > > + for (size_t operationIndex = 0; operationIndex < operations.size(); ++operationIndex) { > > + const auto* operation = operations.at(operationIndex); > > + if (operationIndex >= sharedPrimitives.size()) { > > + ASSERT(operationIndex == sharedPrimitives.size()); > > + sharedPrimitives.append(operation->primitiveType()); > > + } else { > > + auto newPrimitive = operation->sharedPrimitiveType(sharedPrimitives[operationIndex]); > > + if (!newPrimitive.has_value()) > > + return { }; > > + sharedPrimitives[operationIndex] = *newPrimitive; > > It's hard to reason about this code that deals with a list of lists. I > wonder if breaking out the inner loop into a lambda would make it easier to > read. This is a good idea, I've changed the name of this method to sharedPrimitivesForTransformKeyframes and made it static. The inner part of the loop is now a static function called sharedPrimitivesForTransformKeyframe. I've also added a bug link for partial list matching.

Comment on attachment 450212 [details] Patch Thinking about this more, it's a bit odd that we're implementing bits of the CSS transforms spec down in GraphicsLayer. Nothing about this sharedPrimitivesForTransformKeyframes() behavior depends on the capabilites of the platform, right? Maybe we should collapse non-matching lists of functions into matrix representations outside of the platform layer.

(In reply to Simon Fraser (smfr) from comment #17) > Comment on attachment 450212 [details] > Patch > > Thinking about this more, it's a bit odd that we're implementing bits of the > CSS transforms spec down in GraphicsLayer. Nothing about this > sharedPrimitivesForTransformKeyframes() behavior depends on the capabilites > of the platform, right? Maybe we should collapse non-matching lists of > functions into matrix representations outside of the platform layer. This functionality could be moved to the platform layer, but it is really only necessary for the CoreAnimation ports. The TextureMapper just calls TranfromOperations::blend* on every animation tick, which will happily blend between shared primitives. My plan in the next patch is to remove the call to sharedPrimitivesForTransformKeyframes from everywhere other than GraphicsLayerCA once ::blend() can blend any two TransformOperations.

Comment on attachment 450212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450212&action=review > Source/WebCore/platform/graphics/GraphicsLayer.cpp:721 > +static bool sharedPrimitivesForTransformKeyframe(const TransformOperations& operations, Vector<TransformOperation::OperationType>& sharedPrimitives) I suggest a verb phrase for the name of this function, since it does not *return* shared primitives. When a function name is a noun or adjective phrase, the name must correspond to the return value. I suggest "updateSharedPrimitives" but you may have an even better name. Not sure you need to include the words "for transform key frame", since clearly it updates based on the arguments. The comment "this function may modify the contents of sharedPrimitives" is too oblique! What kind of modification does it make, to what end? Maybe something closer to "this function updates vector elements to correspond to what is shared each time it’s called". Or maybe something better, but I think more than "may modify". I think it should also say explicitly something more like, "this returns false when it discovered that there are no primitives that can be shared", rather than the language "determine if the operations for another keyframe are compatible", which doesn’t as directly explain what the boolean means. Another possibility is to make this a lambda, so it’s clearly just part of the function below, and doesn’t have to have a name understandable outside the function. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:733 > + sharedPrimitives.reserveCapacity(operations.size()); This is not useful. The caller, sharedPrimitivesForTransformKeyframes, has already reserved capacity when the vector was empty. When the vector was non-empty, we’ve already exited if the sizes aren’t the same. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:735 > + for (size_t operationIndex = 0; operationIndex < operations.size(); ++operationIndex) { > + const auto* operation = operations.at(operationIndex); Should use a modern for loop: for (auto& operation : operations.operations()) { We can make the above cleaner by improving TransformOperations so it’s compatible with modern for loops without requiring a function cll, but even now this is better. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:745 > + if (!sharedPrimitive.has_value()) I personally would omit the unneeded "has_value()" call here, but some people think it’s easier to read. I think there are many places below where I would also omit it. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:765 > + return { }; I think std::nullopt is clearer in a case like this, although perhaps you prefer { }? > Source/WebCore/platform/graphics/GraphicsLayer.cpp:771 > + for (size_t valueListIndex = 0; valueListIndex < valueList.size(); ++valueListIndex) { > + const TransformOperations& operations = operationsAt(valueList, valueListIndex); We really need to change KeyframeValueList so one can iterate it with a modern for loop. This so so wordy and it should be much more like this: for (auto& operations : valueList) { Also if we can’t use a modern for loop, in WebKit we normally use a single letter variable name for a very small number of things, and one of those is this kind of for loop, so we would use "i" instead of "valueListIndex", which helps a bit with the wordiness. > Source/WebCore/platform/graphics/GraphicsLayer.cpp:773 > + return { }; I think std::nullopt is clearer in a case like this, although perhaps you prefer { }? > Source/WebCore/platform/graphics/GraphicsLayer.cpp:776 > + return sharedPrimitives; Given the result is a std::optional, not the Vector, we need WTFMove here to avoid *copying* the Vector into the optional<Vector>, which will be unnecessary memory allocation. > Source/WebCore/platform/graphics/GraphicsLayer.h:670 > + // Given a list of TransformAnimationValues keyframes, return a list of primitive operations which can > + // represent the transform functions of every keyframe. If the keyframes do not share compatible functions, > + // an empty optional is returned. > + static std::optional<Vector<TransformOperation::OperationType>> sharedPrimitivesForTransformKeyframes(const KeyframeValueList&); Not sure we need optional here. Could we just use an empty vector instead of nullopt to mean nothing is shared? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3465 > + TransformOperation::OperationType transformOp = isMatrixAnimation ? TransformOperation::MATRIX_3D : (*operations)[animationIndex]; Can use auto here instead of writing out that long TransformOperation::OperationType. > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:533 > + bool appendToUncommittedAnimations(const KeyframeValueList&, const std::optional<Vector<TransformOperation::OperationType>>& operations, const Animation*, const String& animationName, const FloatSize& boxSize, unsigned animationIndex, Seconds timeOffset, bool keyframesShouldUseAnimationWideTimingFunction); Does an empty vector have a different meaning than an omitted vector? If not, you might want to consider using that and not using std::optional. It’s easy to accidentally make temporary copies of vectors when using optional<Vector>. > Source/WebCore/platform/graphics/transforms/TransformOperation.cpp:94 > + return { }; Maybe std::nullopt is better? Also no blank line above this, would look better. > Source/WebCore/platform/graphics/transforms/TransformOperation.h:74 > + virtual bool isSameType(const TransformOperation& other) const { return type() == other.type(); } I don’t see any overriding of this. Why is it virtual? > Source/WebCore/platform/graphics/transforms/TransformOperation.h:76 > + virtual OperationType primitiveType() const { return m_type; } Does this really need to be a virtual function? > Source/WebCore/platform/graphics/transforms/TransformOperation.h:78 > + std::optional<OperationType> sharedPrimitiveType(OperationType otherType) const; > + std::optional<OperationType> sharedPrimitiveType(const TransformOperation* other) const; These both could be called "other", or both could be called otherType and otherOperation. But no need to be inconsistent.

(In reply to Darin Adler from comment #19) > Comment on attachment 450212 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=450212&action=review Thanks for the review! > > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:721 > > +static bool sharedPrimitivesForTransformKeyframe(const TransformOperations& operations, Vector<TransformOperation::OperationType>& sharedPrimitives) > > I suggest a verb phrase for the name of this function, since it does not > *return* shared primitives. When a function name is a noun or adjective > phrase, the name must correspond to the return value. I suggest > "updateSharedPrimitives" but you may have an even better name. Not sure you > need to include the words "for transform key frame", since clearly it > updates based on the arguments. > > The comment "this function may modify the contents of sharedPrimitives" is > too oblique! What kind of modification does it make, to what end? Maybe > something closer to "this function updates vector elements to correspond to > what is shared each time it’s called". Or maybe something better, but I > think more than "may modify". > > I think it should also say explicitly something more like, "this returns > false when it discovered that there are no primitives that can be shared", > rather than the language "determine if the operations for another keyframe > are compatible", which doesn’t as directly explain what the boolean means. > > Another possibility is to make this a lambda, so it’s clearly just part of > the function below, and doesn’t have to have a name understandable outside > the function. I've gone ahead and made this a lambda. It will only ever be used in the static function that calls it. I've preserved the comment though, with the improvements that you suggest. It isn't strictly necessary but the code is not entirely straight forward on first read. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:733 > > + sharedPrimitives.reserveCapacity(operations.size()); > > This is not useful. The caller, sharedPrimitivesForTransformKeyframes, has > already reserved capacity when the vector was empty. When the vector was > non-empty, we’ve already exited if the sizes aren’t the same. Nice catch. Fixed. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:735 > > + for (size_t operationIndex = 0; operationIndex < operations.size(); ++operationIndex) { > > + const auto* operation = operations.at(operationIndex); > > Should use a modern for loop: > > for (auto& operation : operations.operations()) { The issue here is that we need the index in order to index into sharedPrimitives. I really wish C++ had something like Python's enumerate(). If you have a better idea, I'm happy to give it a shot. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:745 > > + if (!sharedPrimitive.has_value()) > > I personally would omit the unneeded "has_value()" call here, but some > people think it’s easier to read. I think there are many places below where > I would also omit it. I've removed all of the has_value() calls from the patch. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:765 > > + return { }; > > I think std::nullopt is clearer in a case like this, although perhaps you > prefer { }? I agree that std::nullopt is clearer and have used it everywhere now. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:771 > > + for (size_t valueListIndex = 0; valueListIndex < valueList.size(); ++valueListIndex) { > > + const TransformOperations& operations = operationsAt(valueList, valueListIndex); > > We really need to change KeyframeValueList so one can iterate it with a > modern for loop. This so so wordy and it should be much more like this: > > for (auto& operations : valueList) { I'm happy to look into this for a followup patch. I've seen a few places where modern for loops would be nice to have! > Also if we can’t use a modern for loop, in WebKit we normally use a single > letter variable name for a very small number of things, and one of those is > this kind of for loop, so we would use "i" instead of "valueListIndex", > which helps a bit with the wordiness. In the original version of the patch the two loops were nested. To be honest, I switched to using wordier names because during the development of the patch, I introduced a bug by using the wrong loop iterator. This would have been fixed by the use of modern for loops in the outer loop. One of the for loops is in a lambda now, so I've switched to using one letter variable names. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:773 > > + return { }; > > I think std::nullopt is clearer in a case like this, although perhaps you > prefer { }? I switched this to std::nullopt. > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:776 > > + return sharedPrimitives; > > Given the result is a std::optional, not the Vector, we need WTFMove here to > avoid *copying* the Vector into the optional<Vector>, which will be > unnecessary memory allocation. I'm genuinely surprised the compiler can't do copy elision here! [1] If I just do a return WTFMove(sharedPrimitives); then I get an error like warning: redundant move in return statement [-Wredundant-move] I can also do return { WTFMove(sharedPrimitives) }; if that is better in some way. 1. https://stackoverflow.com/questions/51402731/why-does-returning-a-stdoptional-sometimes-move-and-sometimes-copy > > Source/WebCore/platform/graphics/GraphicsLayer.h:670 > > + // Given a list of TransformAnimationValues keyframes, return a list of primitive operations which can > > + // represent the transform functions of every keyframe. If the keyframes do not share compatible functions, > > + // an empty optional is returned. > > + static std::optional<Vector<TransformOperation::OperationType>> sharedPrimitivesForTransformKeyframes(const KeyframeValueList&); > > Not sure we need optional here. Could we just use an empty vector instead of > nullopt to mean nothing is shared? I gave this a shot, but I began to have doubts. I don't know if any of the callers ever pass a series of empty keyframes to this function and there are callers across multiple platforms (for the moment). I think it makes sense to maintain a semantic difference between "there is a mismatch" and "there are no entries." I understand that the std::optional is a little ugly here, but it seemed like an improvement over the bool out parameter. > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3465 > > + TransformOperation::OperationType transformOp = isMatrixAnimation ? TransformOperation::MATRIX_3D : (*operations)[animationIndex]; > > Can use auto here instead of writing out that long > TransformOperation::OperationType. For sure! > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:533 > > + bool appendToUncommittedAnimations(const KeyframeValueList&, const std::optional<Vector<TransformOperation::OperationType>>& operations, const Animation*, const String& animationName, const FloatSize& boxSize, unsigned animationIndex, Seconds timeOffset, bool keyframesShouldUseAnimationWideTimingFunction); > > Does an empty vector have a different meaning than an omitted vector? If > not, you might want to consider using that and not using std::optional. It’s > easy to accidentally make temporary copies of vectors when using > optional<Vector>. Addressed above. > > Source/WebCore/platform/graphics/transforms/TransformOperation.cpp:94 > > + return { }; > > Maybe std::nullopt is better? Also no blank line above this, would look > better. Done. > > Source/WebCore/platform/graphics/transforms/TransformOperation.h:74 > > + virtual bool isSameType(const TransformOperation& other) const { return type() == other.type(); } > > I don’t see any overriding of this. Why is it virtual? This was left over from a previous version of the change. Fixed. > > Source/WebCore/platform/graphics/transforms/TransformOperation.h:76 > > + virtual OperationType primitiveType() const { return m_type; } > > Does this really need to be a virtual function? Hrm. I think this needs to be virtual or a single implementation in TransformOperation.cpp would need to make a few calls to is<SubclassTransformOperation>. I'm not sure what's better here. The issue is that each operation has a different implementation of isRepresentableIn2D() and RotateTransformOperation needs to look at the return value of RotateTransformOperation::z(). > > Source/WebCore/platform/graphics/transforms/TransformOperation.h:78 > > + std::optional<OperationType> sharedPrimitiveType(OperationType otherType) const; > > + std::optional<OperationType> sharedPrimitiveType(const TransformOperation* other) const; > > These both could be called "other", or both could be called otherType and > otherOperation. But no need to be inconsistent. Good point. :) Fixed.

Created attachment 450391 [details] Patch

Created attachment 450395 [details] Patch

(In reply to Martin Robinson from comment #20) > I'm genuinely surprised the compiler can't do copy elision here! Maybe it can. I was assuming I knew the rules and could have been wrong. > I can also do > > return { WTFMove(sharedPrimitives) }; > > if that is better in some way. I only care that the vector is not copied. If it’s not then the specific style we choose doesn’t matter to me. > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:533 > > > + bool appendToUncommittedAnimations(const KeyframeValueList&, const std::optional<Vector<TransformOperation::OperationType>>& operations, const Animation*, const String& animationName, const FloatSize& boxSize, unsigned animationIndex, Seconds timeOffset, bool keyframesShouldUseAnimationWideTimingFunction); > > > > Does an empty vector have a different meaning than an omitted vector? If > > not, you might want to consider using that and not using std::optional. It’s > > easy to accidentally make temporary copies of vectors when using > > optional<Vector>. > > Addressed above. I thought about it more. I suggest we consider taking a std::optional<std::span<TransformOperation::OperationType>> here, reducing the change that we can accidentally copy. We should think of std::span (or WTF::Span) as VectorView.

Created attachment 450643 [details] Patch

(In reply to Darin Adler from comment #23) > > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:533 > > > > + bool appendToUncommittedAnimations(const KeyframeValueList&, const std::optional<Vector<TransformOperation::OperationType>>& operations, const Animation*, const String& animationName, const FloatSize& boxSize, unsigned animationIndex, Seconds timeOffset, bool keyframesShouldUseAnimationWideTimingFunction); > > > > > > Does an empty vector have a different meaning than an omitted vector? If > > > not, you might want to consider using that and not using std::optional. It’s > > > easy to accidentally make temporary copies of vectors when using > > > optional<Vector>. > > > > Addressed above. > > I thought about it more. I suggest we consider taking a > std::optional<std::span<TransformOperation::OperationType>> here, reducing > the change that we can accidentally copy. We should think of std::span (or > WTF::Span) as VectorView. Hrm. I gave this a shot and I had some trouble. The compiler complained when I tried to store the result of Vector::span() in a std::optional. I think this is the relevant part of the error message: ^ /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.sdk/usr/include/c++/v1/optional:786:5: note: candidate template ignored: substitution failure [with _Up = WTF::Span<const WebCore::TransformOperation::OperationType, 18446744073709551615>]: no member named '_EnableIfImpl' in 'std::_MetaBase<false>' operator=(_Up&& __v) Honestly, I'm sometimes a bit baffled by these errors. In any case, I thought about this some more and looked a bit at the next patch I wanted to write in this series. In that patch it is quite likely that this will need to return both a size_t and the list of shared primitives. I opted to modify this code to store the shared primitives in an out-parameter and return a boolean if there were shared primitives. The new signature is: static bool getSharedPrimitivesForTransformKeyframes(const KeyframeValueList&, Vector<TransformOperation::OperationType>& sharedPrimitives); This eliminates the use of std::optional entirely and, I think, also the risk of ever making a copy of the vector. Instead, references to Vector<...> are always passed. In additional, I've prefixed the method with "get" because it takes an out-parameter. The parameter is still a reference, even though it is unused by some callers, because the next patch in the series will remove the two callers that don't need the result and I wanted to avoid code churn.

I normally prefer optional<> to a boolean plus out argument. We need to figure out the span argument situation, because it should be absolutely solvable, and we need to be able to use that idiom in WebKit in the future. Maybe there’s a bug in WTF::Span?

You plans sound fine, though. Don’t want to put too big a burden on this one patch.

Committed r289032 (246740@main): <https://commits.webkit.org/246740@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450643 [details].

*** Bug 235801 has been marked as a duplicate of this bug. ***

*** Bug 235803 has been marked as a duplicate of this bug. ***

Thanks for fixing this Martin, this really helps WebKit with regards to css-transforms support and unique WPT failures.

This caused bug 239906.