Bug 182741 - [Web Animations] Make KeyframeEffect target nullable and read-write
Summary: [Web Animations] Make KeyframeEffect target nullable and read-write
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 182756 182939
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-13 12:48 PST by Antoine Quint
Modified: 2018-02-19 16:36 PST (History)
10 users (show)

See Also:


Attachments
Patch (162.09 KB, patch)
2018-02-13 13:56 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2018-02-13 17:06 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (270.08 KB, patch)
2018-02-13 21:50 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.82 MB, application/zip)
2018-02-13 22:50 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (2.24 MB, application/zip)
2018-02-13 22:57 PST, EWS Watchlist
no flags Details
Patch (270.02 KB, patch)
2018-02-14 04:58 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.75 MB, application/zip)
2018-02-14 06:11 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.32 MB, application/zip)
2018-02-14 06:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (2.97 MB, application/zip)
2018-02-14 06:32 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews101 for mac-sierra (2.36 MB, application/zip)
2018-02-14 07:53 PST, EWS Watchlist
no flags Details
Patch (269.98 KB, patch)
2018-02-14 07:55 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (122.27 KB, patch)
2018-02-19 14:11 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.25 MB, application/zip)
2018-02-19 14:55 PST, EWS Watchlist
no flags Details
Patch (123.52 KB, patch)
2018-02-19 14:58 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (123.40 KB, patch)
2018-02-19 15:04 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (221.91 KB, patch)
2018-02-19 15:06 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.55 MB, application/zip)
2018-02-19 15:50 PST, EWS Watchlist
no flags Details
Patch (221.66 KB, patch)
2018-02-19 15:52 PST, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-02-13 12:48:18 PST
[Web Animations] Make KeyframeEffect target nullable and read-write
Comment 1 Antoine Quint 2018-02-13 13:56:09 PST
Created attachment 333722 [details]
Patch
Comment 2 Dean Jackson 2018-02-13 14:18:49 PST
Comment on attachment 333722 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        We used to completely disregard null targets, for instance not parsing keyframes, but targets
> +        can be null and are also supposed to be read-write for KeyframeEffect. We now update the IDL

This sentence is weird.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
> +    if (!scriptExecutionContext.isDocument())
> +        return Exception { TypeError };

Is there any way to create a keyframe animation in a worker that operates on a non-element?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:558
> +    auto& document = downcast<Document>(scriptExecutionContext);
> +    auto documentElement = document.documentElement();

No need for the document local variable.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:586
> +        renderStyle->fontCascade().update(nullptr);

Maybe put a comment here?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:656
> +        previousTarget->invalidateStyleAndLayerComposition();
> +        previousTarget->document().updateStyleIfNeeded();

Why do you need both?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:700
> +    m_target->invalidateStyleAndLayerComposition();
> +    m_target->document().updateStyleIfNeeded();

Ditto?
Comment 3 Antoine Quint 2018-02-13 14:36:39 PST
(In reply to Dean Jackson from comment #2)
> Comment on attachment 333722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333722&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
> > +    if (!scriptExecutionContext.isDocument())
> > +        return Exception { TypeError };
> 
> Is there any way to create a keyframe animation in a worker that operates on
> a non-element?

No, it's only exposed on Window.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:558
> > +    auto& document = downcast<Document>(scriptExecutionContext);
> > +    auto documentElement = document.documentElement();
> 
> No need for the document local variable.

Will remove.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:586
> > +        renderStyle->fontCascade().update(nullptr);
> 
> Maybe put a comment here?

Will do.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:656
> > +        previousTarget->invalidateStyleAndLayerComposition();
> > +        previousTarget->document().updateStyleIfNeeded();
> 
> Why do you need both?
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:700
> > +    m_target->invalidateStyleAndLayerComposition();
> > +    m_target->document().updateStyleIfNeeded();
> 
> Ditto?

I'll add comments there.
Comment 4 Antoine Quint 2018-02-13 14:43:59 PST
Committed r228437: <https://trac.webkit.org/changeset/228437>
Comment 5 Radar WebKit Bug Importer 2018-02-13 14:44:32 PST
<rdar://problem/37514127>
Comment 6 Chris Dumez 2018-02-13 15:18:34 PST
Follow-Up build fix in <https://trac.webkit.org/changeset/228439>.
Comment 7 Antoine Quint 2018-02-13 15:23:13 PST
Committed r228440: <https://trac.webkit.org/changeset/228440>
Comment 8 Chris Dumez 2018-02-13 15:49:05 PST
Comment on attachment 333722 [details]
Patch

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

>>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:555
>>> +        return Exception { TypeError };
>> 
>> Is there any way to create a keyframe animation in a worker that operates on a non-element?
> 
> No, it's only exposed on Window.

We prefer is<Document>(scriptExecutionContext) nowadays. Looks nice with the downcast<Document>() below.

> Source/WebCore/animation/KeyframeEffectReadOnly.h:82
> +    void setTarget(RefPtr<Element>);

RefPtr<Element>&& ?

> Source/WebCore/animation/WebAnimation.cpp:164
> +void WebAnimation::effectTargetDidChange(RefPtr<Element> previousTarget, RefPtr<Element> newTarget)

RefPtr<Element>&& ? This is doing refcounting churn. or Element* if you're now transferring ownership.
Comment 9 Antoine Quint 2018-02-13 17:06:34 PST
Reopening to attach new patch.
Comment 10 Antoine Quint 2018-02-13 17:06:35 PST
Created attachment 333750 [details]
Patch
Comment 11 Antoine Quint 2018-02-13 21:50:31 PST
Created attachment 333766 [details]
Patch
Comment 12 EWS Watchlist 2018-02-13 22:50:09 PST
Comment on attachment 333766 [details]
Patch

Attachment 333766 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6495570

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 13 EWS Watchlist 2018-02-13 22:50:10 PST
Created attachment 333770 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 14 EWS Watchlist 2018-02-13 22:57:18 PST
Comment on attachment 333766 [details]
Patch

Attachment 333766 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6495772

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 15 EWS Watchlist 2018-02-13 22:57:20 PST
Created attachment 333771 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 16 Daniel Bates 2018-02-13 22:57:36 PST
Comment on attachment 333766 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:61
> +static inline CSSPropertyID IDLAttributeNameToAnimationPropertyName(String idlAttributeName)

I know that you are just moving code here. This function should take a "const String&" instead of String because the latter may unnecessarily ref the underlying StringImpl (if the caller passed an lvalue) and this function does not take ownership of the passed string (if the caller passed an rvalue).

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:645
> +    auto previousTarget = m_target;
> +    if (previousTarget == newTarget)
> +        return;

This is causing unnecessary ref-count churn of m_target when previousTarget == newTarget since we ref m_target when we do the copy assignment on line 643 only to deref it on early return. We can avoid the ref-count churn when m_target == newTarget by explicitly comparing m_target and newTarget:

if (m_target == newTarget)
    return;

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:647
> +    m_target = newTarget;

Then we can write this as:

auto previousTarget = WTFMove(m_target);
m_target = WTFMove(newTarget);

OR take advantage of std::exchange() to simplify this to:

auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> +    if (auto effectAnimation = animation())

For your consideration, I suggest that we use auto* instead of auto to convey to the reader that animation() returns a pointer. I know that this can be inferred from the context, but C++ has all sorts of craziness and it saves me a second to see the '*' instead of having to read the line below.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:660
> +        previousTarget->invalidateStyleAndLayerComposition();
> +        previousTarget->document().updateStyleIfNeeded();

Can we find some way to avoid duplicating this logic here and in KeyframeEffectReadOnly::invalidate()? One idea is to extract this logic into a file-level static function F that take an Element&. Then have this function and KeyframeEffectReadOnly::invalidate() invoke F.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:704
> +    m_target->invalidateStyleAndLayerComposition();
> +    m_target->document().updateStyleIfNeeded();

This duplicates the logic in KeyframeEffectReadOnly::setTarget(). Can we find some to share it? See my remark above for more details.

> Source/WebCore/animation/WebAnimation.cpp:164
> +void WebAnimation::effectTargetDidChange(RefPtr<Element>&& previousTarget, RefPtr<Element>&& newTarget)

This function does not take ownership of previousTarget or newTarget. So, these arguments should be raw pointers.

In general, it is only beneficial to pass by rvalue reference when the function- or a function it calls- takes ownership of the passed value (by WTFMove()'ing it into something).
Comment 17 Antoine Quint 2018-02-14 04:45:06 PST
(In reply to Daniel Bates from comment #16)
> Comment on attachment 333766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333766&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:61
> > +static inline CSSPropertyID IDLAttributeNameToAnimationPropertyName(String idlAttributeName)
> 
> I know that you are just moving code here. This function should take a
> "const String&" instead of String because the latter may unnecessarily ref
> the underlying StringImpl (if the caller passed an lvalue) and this function
> does not take ownership of the passed string (if the caller passed an
> rvalue).

As a matter of fact, I did make a change in IDLAttributeNameToAnimationPropertyName, so I'll make the suggested change.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:645
> > +    auto previousTarget = m_target;
> > +    if (previousTarget == newTarget)
> > +        return;
> 
> This is causing unnecessary ref-count churn of m_target when previousTarget
> == newTarget since we ref m_target when we do the copy assignment on line
> 643 only to deref it on early return. We can avoid the ref-count churn when
> m_target == newTarget by explicitly comparing m_target and newTarget:
> 
> if (m_target == newTarget)
>     return;
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:647
> > +    m_target = newTarget;
> 
> Then we can write this as:
> 
> auto previousTarget = WTFMove(m_target);
> m_target = WTFMove(newTarget);
> 
> OR take advantage of std::exchange() to simplify this to:
> 
> auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

Actually, this sets m_target to a null value in both cases. This works:

auto previousTarget = std::exchange(m_target, newTarget);

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> > +    if (auto effectAnimation = animation())
> 
> For your consideration, I suggest that we use auto* instead of auto to
> convey to the reader that animation() returns a pointer. I know that this
> can be inferred from the context, but C++ has all sorts of craziness and it
> saves me a second to see the '*' instead of having to read the line below.

Will fix in updated patch.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:660
> > +        previousTarget->invalidateStyleAndLayerComposition();
> > +        previousTarget->document().updateStyleIfNeeded();
> 
> Can we find some way to avoid duplicating this logic here and in
> KeyframeEffectReadOnly::invalidate()? One idea is to extract this logic into
> a file-level static function F that take an Element&. Then have this
> function and KeyframeEffectReadOnly::invalidate() invoke F.
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:704
> > +    m_target->invalidateStyleAndLayerComposition();
> > +    m_target->document().updateStyleIfNeeded();
> 
> This duplicates the logic in KeyframeEffectReadOnly::setTarget(). Can we
> find some to share it? See my remark above for more details.

I'll add `static inline void invalidateElement(Element* element)` in an updated patch.

> > Source/WebCore/animation/WebAnimation.cpp:164
> > +void WebAnimation::effectTargetDidChange(RefPtr<Element>&& previousTarget, RefPtr<Element>&& newTarget)
> 
> This function does not take ownership of previousTarget or newTarget. So,
> these arguments should be raw pointers.
> 
> In general, it is only beneficial to pass by rvalue reference when the
> function- or a function it calls- takes ownership of the passed value (by
> WTFMove()'ing it into something).

Sure, will fix in new patch.
Comment 18 Antoine Quint 2018-02-14 04:58:38 PST
Created attachment 333788 [details]
Patch
Comment 19 EWS Watchlist 2018-02-14 06:11:07 PST
Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6499523

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 20 EWS Watchlist 2018-02-14 06:11:09 PST
Created attachment 333790 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 21 EWS Watchlist 2018-02-14 06:25:33 PST
Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6499475

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 22 EWS Watchlist 2018-02-14 06:25:35 PST
Created attachment 333793 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 23 EWS Watchlist 2018-02-14 06:32:01 PST
Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6499662

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 24 EWS Watchlist 2018-02-14 06:32:04 PST
Created attachment 333794 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 25 Antoine Quint 2018-02-14 07:43:51 PST
(In reply to Antoine Quint from comment #17)
> (In reply to Daniel Bates from comment #16)
> > Comment on attachment 333766 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333766&action=review
> > 
> > OR take advantage of std::exchange() to simplify this to:
> > 
> > auto previousTarget = std::exchange(m_target, WTFMove(newTarget));
> 
> Actually, this sets m_target to a null value in both cases. This works:

That was my mistake, this works fine. I'll have a new patch up shortly.
Comment 26 EWS Watchlist 2018-02-14 07:53:17 PST
Comment on attachment 333788 [details]
Patch

Attachment 333788 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6500337

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 27 EWS Watchlist 2018-02-14 07:53:18 PST
Created attachment 333799 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 28 Antoine Quint 2018-02-14 07:55:36 PST
Created attachment 333800 [details]
Patch
Comment 29 Daniel Bates 2018-02-14 09:59:59 PST
Comment on attachment 333800 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:78
> +    // FIXME: we don't support the CSS "offset" property

The remark after the FIXME should be complete sentences that begin with a capital letter and ends with a period.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:516
>      // 1. If object is null, return an empty sequence of keyframes.

This comment no longer matches what the code does OR I am unclear what "object" refers to in this comment. Is the code correct?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:517
> +    if (!keyframesInput.get())

No need for calling get(). It is sufficient to write this as:

if (!keyframesInput)

(Assuming this change is correct. See my remark above about the discrepancy with the comment).

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:564
> +    if (!is<Document>(scriptExecutionContext))
> +        return Exception { TypeError };

From talking with you today in person, you clarified that this code would never run in a worker. So, I suggest we turn this into an assertion and assert it at the top of the function.
Comment 30 Chris Dumez 2018-02-14 10:01:40 PST
Comment on attachment 333800 [details]
Patch

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

>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:564
>> +        return Exception { TypeError };
> 
> From talking with you today in person, you clarified that this code would never run in a worker. So, I suggest we turn this into an assertion and assert it at the top of the function.

If this is true then you may want to consider using CallWith=Document instead in the IDL.
Comment 31 Antoine Quint 2018-02-15 10:06:00 PST
Comment on attachment 333800 [details]
Patch

After discussions with Ryosuke, Chris and Antti, we've decided there is some preliminary work to be made before we can implement this elegantly. Specifically, we should decouple parsing keyframes from a JS object and actually building the data structure that contains resolved styles. The JS object parsing should be done in custom bindings code and the styles resolution once we have a target to work with.
Comment 32 Antoine Quint 2018-02-19 14:11:58 PST
Created attachment 334186 [details]
Patch
Comment 33 EWS Watchlist 2018-02-19 14:55:04 PST
Comment on attachment 334186 [details]
Patch

Attachment 334186 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6578143

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
Comment 34 EWS Watchlist 2018-02-19 14:55:06 PST
Created attachment 334190 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 35 Antoine Quint 2018-02-19 14:58:37 PST
Created attachment 334191 [details]
Patch
Comment 36 Antoine Quint 2018-02-19 15:04:24 PST
Created attachment 334192 [details]
Patch
Comment 37 Antoine Quint 2018-02-19 15:06:41 PST
Created attachment 334193 [details]
Patch
Comment 38 EWS Watchlist 2018-02-19 15:50:12 PST
Comment on attachment 334193 [details]
Patch

Attachment 334193 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6579038

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
Comment 39 EWS Watchlist 2018-02-19 15:50:14 PST
Created attachment 334195 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 40 Antoine Quint 2018-02-19 15:52:18 PST
Created attachment 334196 [details]
Patch
Comment 41 Dean Jackson 2018-02-19 16:02:20 PST
Comment on attachment 334196 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:53
> +    element->invalidateStyleAndLayerComposition();
> +    element->document().updateStyleIfNeeded();

Did you ever work out why we need to call both here? Shouldn't Element::invalidateStyleAndLayerComposition call document().updateStyleIfNeeded()?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:72
> +    // 1. If attribute conforms to the <custom-property-name> production, return attribute.

Add a comment as to why this isn't being handled.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:76
> +    // 3. If attribute is the string "cssOffset", then return an animation property representing the CSS offset property.

Add a blank line here.

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:167
> +                easing = convert<IDLDOMString>(state, keyframe->get(&state, ownPropertyName));

Why do you have to pass the ExecState to the keyframe?

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> +    auto previousTarget = std::exchange(m_target, WTFMove(newTarget));

std::exchange, nice.
Comment 42 Antoine Quint 2018-02-19 16:32:24 PST
(In reply to Dean Jackson from comment #41)
> Comment on attachment 334196 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334196&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:53
> > +    element->invalidateStyleAndLayerComposition();
> > +    element->document().updateStyleIfNeeded();
> 
> Did you ever work out why we need to call both here? Shouldn't
> Element::invalidateStyleAndLayerComposition call
> document().updateStyleIfNeeded()?

Zalan explained that invalidateStyleAndLayerComposition() only sets the dirty flags and the document needs to be updated manually.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:72
> > +    // 1. If attribute conforms to the <custom-property-name> production, return attribute.
> 
> Add a comment as to why this isn't being handled.

Will fix in commit.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:76
> > +    // 3. If attribute is the string "cssOffset", then return an animation property representing the CSS offset property.
> 
> Add a blank line here.

Will fix in commit.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:167
> > +                easing = convert<IDLDOMString>(state, keyframe->get(&state, ownPropertyName));
> 
> Why do you have to pass the ExecState to the keyframe?

The keyframe is a JSC object and to obtain its value you must pass the state.

> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:649
> > +    auto previousTarget = std::exchange(m_target, WTFMove(newTarget));
> 
> std::exchange, nice.

I can't take credit for that, dbates suggested that in a previous review.
Comment 43 Antoine Quint 2018-02-19 16:36:26 PST
Committed r228717: <https://trac.webkit.org/changeset/228717>