Remove the dependency on CSSInterpolationTypes from PropertyRegistration [chromium/src : master]

0 views
Skip to first unread message

Alan Cutter (Gerrit)

unread,
Jun 21, 2017, 10:08:45 PM6/21/17
to apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, chromium...@chromium.org, Renée Wright, Rob Buis, Alexis Menard, Eric Willigers, Shane Stephens

Alan Cutter uploaded patch set #4 to this change.

View Change

Remove the dependency on CSSInterpolationTypes from PropertyRegistration

The existing construction code for PropertyRegistration required that
every InterpolationType was a CSSInterpolationType so that the
PropertyRegistration object could pass a reference to itself to the
CSSInterpolationTypes.
This change has the PropertyRegistration pass the reference to itself
to CreateInterpolationTypesForSyntax() so it can pass it in the
constructor parameters for specific CSSInterpolationTypes.
This allows CreateInterpolationTypesForSyntax() to return
InterpolationTypes that don't inherit from CSSInterpolationType
removing a prohibitive constraint that's getting in the way of
implementing var() reference cycle detection for animations.

This patch is a refactor and introduces no changes in behaviour.

Bug: 671904
Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
---
M third_party/WebKit/Source/core/animation/CSSAngleInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSBasicShapeInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSBorderImageLengthBoxInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSClipInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSColorInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSFilterListInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSFontSizeInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSFontVariationSettingsInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSFontWeightInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSImageInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSImageListInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSImageSliceInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
M third_party/WebKit/Source/core/animation/CSSInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSInterpolationTypesMap.cpp
M third_party/WebKit/Source/core/animation/CSSInterpolationTypesMap.h
M third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp
M third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSLengthListInterpolationType.cpp
M third_party/WebKit/Source/core/animation/CSSLengthListInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSLengthPairInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSNumberInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSOffsetRotateInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSPaintInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSPathInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSPositionAxisListInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSPositionInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSResolutionInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSRotateInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSScaleInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSShadowListInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSSizeListInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSTextIndentInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSTimeInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSTransformInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSTransformOriginInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSTranslateInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSValueInterpolationType.h
M third_party/WebKit/Source/core/animation/CSSVisibilityInterpolationType.h
M third_party/WebKit/Source/core/css/PropertyRegistration.cpp
M third_party/WebKit/Source/core/css/PropertyRegistration.h
M third_party/WebKit/Source/core/css/PropertyRegistry.cpp
M third_party/WebKit/Source/core/css/PropertyRegistry.h
43 files changed, 168 insertions(+), 142 deletions(-)

To view, visit change 538443. To unsubscribe, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
Gerrit-Change-Number: 538443
Gerrit-PatchSet: 4
Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
Gerrit-CC: Alexis Menard <alexis...@intel.com>
Gerrit-CC: Eric Willigers <ericwi...@chromium.org>
Gerrit-CC: Renée Wright <rjwr...@chromium.org>
Gerrit-CC: Rob Buis <rob....@samsung.com>
Gerrit-CC: Shane Stephens <sh...@chromium.org>

Alan Cutter (Gerrit)

unread,
Jun 21, 2017, 10:09:53 PM6/21/17
to Eric Willigers, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org

Alan Cutter would like Eric Willigers to review this change.

Gerrit-MessageType: newchange
Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
Gerrit-Change-Number: 538443
Gerrit-PatchSet: 4
Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
Gerrit-CC: Alexis Menard <alexis...@intel.com>

Alan Cutter (Gerrit)

unread,
Jun 22, 2017, 9:40:57 PM6/22/17
to Eric Willigers, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, chromium...@chromium.org, Renée Wright, Rob Buis, Alexis Menard, Shane Stephens

Alan Cutter uploaded patch set #6 to this change.

View Change

Remove the dependency on CSSInterpolationTypes from PropertyRegistration

The existing construction code for PropertyRegistration required that
every InterpolationType was a CSSInterpolationType so that the
PropertyRegistration object could pass a reference to itself to the
CSSInterpolationTypes via the
CSSInterpolationType::SetCustomPropertyRegistration() method.


This change has the PropertyRegistration pass the reference to itself
to CreateInterpolationTypesForSyntax() so it can pass it in the
constructor parameters for specific CSSInterpolationTypes instead of
using SetCustomPropertyRegistration() to make the association between
the two objects.
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
Gerrit-Change-Number: 538443
Gerrit-PatchSet: 6

Bugs Nash (Gerrit)

unread,
Jun 25, 2017, 11:38:00 PM6/25/17
to Alan Cutter, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

Bugs Nash posted comments on this change.

View Change

Patch set 7:

I'm not familiar with this code so I may be off with my interpretation here, but from what i can see it seems like:

previously: PropertyRegistration had an InterpolationTypes member, but it was always a CSSInterpolationTypes (I don't know why the member wasn't of CSSInterpolationTypes type to be more clear?)

with this patch: PropertyRegistration has an InterpolationTypes member that is always an InterpolationTypes, but the InterpolationType objects inside this vector are always of CSSInterpolationType. Again, I feel like it's clearer to just have CSSInterpolationTypes as the type?

what exactly about CSSInterpolationTypes is blocking you from implementiong var() reference cycle detection for animations?

    To view, visit change 538443. To unsubscribe, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
    Gerrit-Change-Number: 538443
    Gerrit-PatchSet: 7
    Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
    Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
    Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
    Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Renée Wright <rjwr...@chromium.org>
    Gerrit-CC: Rob Buis <rob....@samsung.com>
    Gerrit-CC: Shane Stephens <sh...@chromium.org>
    Gerrit-Comment-Date: Mon, 26 Jun 2017 03:37:56 +0000
    Gerrit-HasComments: No
    Gerrit-HasLabels: No

    Bugs Nash (Gerrit)

    unread,
    Jun 26, 2017, 12:27:30 AM6/26/17
    to Alan Cutter, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

    Bugs Nash posted comments on this change.

    View Change

    Patch set 7:

    (3 comments)

    To view, visit change 538443. To unsubscribe, visit settings.

    Gerrit-Project: chromium/src
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
    Gerrit-Change-Number: 538443
    Gerrit-PatchSet: 7
    Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
    Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
    Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
    Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
    Gerrit-CC: Alexis Menard <alexis...@intel.com>
    Gerrit-CC: Renée Wright <rjwr...@chromium.org>
    Gerrit-CC: Rob Buis <rob....@samsung.com>
    Gerrit-CC: Shane Stephens <sh...@chromium.org>
    Gerrit-Comment-Date: Mon, 26 Jun 2017 04:27:25 +0000
    Gerrit-HasComments: Yes
    Gerrit-HasLabels: No

    Alan Cutter (Gerrit)

    unread,
    Jun 26, 2017, 12:34:31 AM6/26/17
    to apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Bugs Nash, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

    Alan Cutter posted comments on this change.

    View Change

    Patch set 7:

    what exactly about CSSInterpolationTypes is blocking you from implementiong var() reference cycle detection for animations?

    CSSInterpolationType has a ton of code that doesn't always apply in every case. I want the flexibility to have classes without code that doesn't apply to them in these vectors.

      To view, visit change 538443. To unsubscribe, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
      Gerrit-Change-Number: 538443
      Gerrit-PatchSet: 7
      Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
      Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
      Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
      Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
      Gerrit-CC: Alexis Menard <alexis...@intel.com>
      Gerrit-CC: Renée Wright <rjwr...@chromium.org>
      Gerrit-CC: Rob Buis <rob....@samsung.com>
      Gerrit-CC: Shane Stephens <sh...@chromium.org>
      Gerrit-Comment-Date: Mon, 26 Jun 2017 04:34:26 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: No

      Alan Cutter (Gerrit)

      unread,
      Jun 26, 2017, 12:53:52 AM6/26/17
      to apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Bugs Nash, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

      Alan Cutter posted comments on this change.

      View Change

      Patch set 7:

      (3 comments)

        • Patch Set #7, Line 102: const PropertyRegistration* registration)

          why not have this PropertyRegistration default to nullptr too rather than a

        • Patch Set #7, Line 25: static InterpolationTypes CreateInterpolationTypesForSyntax(

          perhaps CreateInterpolationTypesForCSSSyntax? at first this read to me like

        • Yasyasyaysayapyapyapyapyyapyapapap.

      To view, visit change 538443. To unsubscribe, visit settings.

      Gerrit-Project: chromium/src
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
      Gerrit-Change-Number: 538443
      Gerrit-PatchSet: 7
      Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
      Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
      Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
      Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
      Gerrit-CC: Alexis Menard <alexis...@intel.com>
      Gerrit-CC: Renée Wright <rjwr...@chromium.org>
      Gerrit-CC: Rob Buis <rob....@samsung.com>
      Gerrit-CC: Shane Stephens <sh...@chromium.org>
      Gerrit-Comment-Date: Mon, 26 Jun 2017 04:53:48 +0000
      Gerrit-HasComments: Yes
      Gerrit-HasLabels: No

      Bugs Nash (Gerrit)

      unread,
      Jun 26, 2017, 1:48:23 AM6/26/17
      to Alan Cutter, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

      Bugs Nash posted comments on this change.

      View Change

      Patch set 8:Code-Review +1

        To view, visit change 538443. To unsubscribe, visit settings.

        Gerrit-Project: chromium/src
        Gerrit-Branch: master
        Gerrit-MessageType: comment
        Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
        Gerrit-Change-Number: 538443
        Gerrit-PatchSet: 8
        Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
        Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
        Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
        Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
        Gerrit-CC: Alexis Menard <alexis...@intel.com>
        Gerrit-CC: Renée Wright <rjwr...@chromium.org>
        Gerrit-CC: Rob Buis <rob....@samsung.com>
        Gerrit-CC: Shane Stephens <sh...@chromium.org>
        Gerrit-Comment-Date: Mon, 26 Jun 2017 05:48:19 +0000
        Gerrit-HasComments: No
        Gerrit-HasLabels: Yes

        Alan Cutter (Gerrit)

        unread,
        Jun 26, 2017, 1:49:53 AM6/26/17
        to apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Bugs Nash, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

        Alan Cutter posted comments on this change.

        View Change

        Patch set 8:Commit-Queue +2

          To view, visit change 538443. To unsubscribe, visit settings.

          Gerrit-Project: chromium/src
          Gerrit-Branch: master
          Gerrit-MessageType: comment
          Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
          Gerrit-Change-Number: 538443
          Gerrit-PatchSet: 8
          Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
          Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
          Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
          Gerrit-Reviewer: Eric Willigers <ericwi...@chromium.org>
          Gerrit-CC: Alexis Menard <alexis...@intel.com>
          Gerrit-CC: Renée Wright <rjwr...@chromium.org>
          Gerrit-CC: Rob Buis <rob....@samsung.com>
          Gerrit-CC: Shane Stephens <sh...@chromium.org>
          Gerrit-Comment-Date: Mon, 26 Jun 2017 05:49:49 +0000
          Gerrit-HasComments: No
          Gerrit-HasLabels: Yes

          Commit Bot (Gerrit)

          unread,
          Jun 26, 2017, 3:35:52 AM6/26/17
          to Alan Cutter, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, dglazko...@chromium.org, Bugs Nash, Eric Willigers, Alexis Menard, chromium...@chromium.org, Renée Wright, Rob Buis, Shane Stephens

          Commit Bot merged this change.

          View Change

          Approvals: Bugs Nash: Looks good to me Alan Cutter: Commit
          Remove the dependency on CSSInterpolationTypes from PropertyRegistration

          The existing construction code for PropertyRegistration required that
          every InterpolationType was a CSSInterpolationType so that the
          PropertyRegistration object could pass a reference to itself to the
          CSSInterpolationTypes via the
          CSSInterpolationType::SetCustomPropertyRegistration() method.

          This change has the PropertyRegistration pass the reference to itself
          to CreateInterpolationTypesForSyntax() so it can pass it in the
          constructor parameters for specific CSSInterpolationTypes instead of
          using SetCustomPropertyRegistration() to make the association between
          the two objects.

          This allows CreateInterpolationTypesForSyntax() to return
          InterpolationTypes that don't inherit from CSSInterpolationType
          removing a prohibitive constraint that's getting in the way of
          implementing var() reference cycle detection for animations.

          This patch is a refactor and introduces no changes in behaviour.

          Bug: 671904
          Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
          Reviewed-on: https://chromium-review.googlesource.com/538443
          Reviewed-by: Bugs Nash <bugs...@chromium.org>
          Commit-Queue: Alan Cutter <alanc...@chromium.org>
          Cr-Commit-Position: refs/heads/master@{#482210}
          ---
          M third_party/WebKit/Source/core/animation/CSSAngleInterpolationType.h
          M third_party/WebKit/Source/core/animation/CSSColorInterpolationType.h

          M third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
          M third_party/WebKit/Source/core/animation/CSSInterpolationType.h
          M third_party/WebKit/Source/core/animation/CSSInterpolationTypesMap.cpp
          M third_party/WebKit/Source/core/animation/CSSInterpolationTypesMap.h
          M third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp
          M third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.h
          M third_party/WebKit/Source/core/animation/CSSNumberInterpolationType.h
          M third_party/WebKit/Source/core/animation/CSSResolutionInterpolationType.h
          M third_party/WebKit/Source/core/animation/CSSTimeInterpolationType.h
          M third_party/WebKit/Source/core/animation/CSSValueInterpolationType.h

          M third_party/WebKit/Source/core/css/PropertyRegistration.cpp
          M third_party/WebKit/Source/core/css/PropertyRegistration.h
          M third_party/WebKit/Source/core/css/PropertyRegistry.cpp
          M third_party/WebKit/Source/core/css/PropertyRegistry.h
          16 files changed, 75 insertions(+), 93 deletions(-)


          To view, visit change 538443. To unsubscribe, visit settings.

          Gerrit-Project: chromium/src
          Gerrit-Branch: master
          Gerrit-MessageType: merged
          Gerrit-Change-Id: I38f99c769c72711deec16e0330d5c92bbd39b7e8
          Gerrit-Change-Number: 538443
          Gerrit-PatchSet: 9
          Gerrit-Owner: Alan Cutter <alanc...@chromium.org>
          Gerrit-Reviewer: Alan Cutter <alanc...@chromium.org>
          Gerrit-Reviewer: Bugs Nash <bugs...@chromium.org>
          Gerrit-Reviewer: Commit Bot <commi...@chromium.org>
          Reply all
          Reply to author
          Forward
          0 new messages