New issue
Advanced search Search tips

Issue 614368 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Simplify Gradient and Pattern

Project Member Reported by fmalita@chromium.org, May 24 2016

Issue description

Tracking bug for refactoring Gradient & Pattern as immutable classes.

Gradient in particular has several mutation methods that seem ripe for removal:

  * addColorStop, setP{0,1}, set{Start,End}Radius, setDrawsInPMColorSpace, setSpreadMethod: only used shortly after construction, should be folded into the constructor.

  * setGradientSpaceTransform: tricky usage - some clients always update before applying to paint, while others ignore it (relying on existing value which seems fragile).  This can be removed if we pass the local matrix to applyToMatrix instead.

If all mutation methods are removed, then all that's left of Gradient & Pattern is a collection of factory methods + an SkShader cache.  At that point it may be possible to unify them (Shader superclass?) and simplify the client code which now has to treat them differently.
 

Comment 1 by f...@opera.com, May 24 2016

Cc: f...@opera.com
I think there might be some layers that could be stripped in the SVG paintserver code-path. I did some work around LayoutSVGResourceGradient for setup (and other things), it may or may not pan out to something, but feel free to ping me about it...
Project Member

Comment 2 by bugdroid1@chromium.org, May 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32296b3b4ae6c39f1add59880f90da075b9e104b

commit 32296b3b4ae6c39f1add59880f90da075b9e104b
Author: fmalita <fmalita@chromium.org>
Date: Fri May 27 21:07:02 2016

Retire setGradientSpaceTransform, setPatternSpaceTransform

Most gradient/pattern clients always reset the transform before
applying to a paint, while few (CanvasPattern) actually rely on the
transform state being persistent.  This yields confusing semantics
where it's unclear whether it is safe to whack the state.

The CL removes the transform from the gradient/pattern state, and makes
it an argument to applyToPaint().  This simplifies the gradient/pattern
impl and clarifies that clients are responsible for tracking the local
matrix.  The local matrix is still used as part of the shader cache key
- but since SkShaders already track their local matrix, there is no
need for explicit tracking in Gradient/Pattern.

R=fs@opera.com,junov@chromium.org
BUG=614368

Review-Url: https://codereview.chromium.org/2007553002
Cr-Commit-Position: refs/heads/master@{#396557}

[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceGradient.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePaintServer.h
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/core/paint/ThemePainterMac.mm
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/modules/canvas2d/CanvasPattern.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/modules/canvas2d/CanvasPattern.h
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/modules/canvas2d/CanvasStyle.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/Gradient.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/Gradient.h
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/GradientGeneratedImage.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/ImagePattern.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/ImagePattern.h
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/Pattern.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/Pattern.h
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/PicturePattern.cpp
[modify] https://crrev.com/32296b3b4ae6c39f1add59880f90da075b9e104b/third_party/WebKit/Source/platform/graphics/PicturePattern.h

Project Member

Comment 3 by bugdroid1@chromium.org, May 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/563863e714a03721046689452c7d2ff3de9c228a

commit 563863e714a03721046689452c7d2ff3de9c228a
Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org>
Date: Fri May 27 22:29:12 2016

Auto-rebaseline for r396557

https://chromium.googlesource.com/chromium/src/+/32296b3b4

BUG=614368
TBR=fmalita@chromium.org

Review URL: https://codereview.chromium.org/2021563002 .

Cr-Commit-Position: refs/heads/master@{#396586}

[modify] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/fast/gradients/css3-radial-gradients4-expected.png
[add] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/platform/android/fast/gradients/css3-radial-gradients4-expected.png
[add] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/platform/android/fast/gradients/css3-repeating-radial-gradients-expected.png
[add] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/platform/android/fast/gradients/unprefixed-repeating-radial-gradients-expected.png
[modify] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/platform/linux/fast/gradients/css3-radial-gradients4-expected.png
[modify] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/platform/linux/fast/gradients/css3-repeating-radial-gradients-expected.png
[modify] https://crrev.com/563863e714a03721046689452c7d2ff3de9c228a/third_party/WebKit/LayoutTests/platform/linux/fast/gradients/unprefixed-repeating-radial-gradients-expected.png

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e

commit aaa44070561b21e6ea2d9ae2acd2b0676c5b853e
Author: fs <fs@opera.com>
Date: Fri Jun 03 18:30:41 2016

Reduce LayoutSVGResource*Gradient building dependency on GradientData

The buildGradient() method can just create and return a Gradient instead
of populating the GradientData struct it's being passed.
Also make calculateGradientTransform() use the return value rather than
an out variable, and make it const qualified. Make
platformSpreadMethodFromSVGType static and use Traversal<> sugar in
SVGGradientElement::buildStops.

BUG=614368

Review-Url: https://codereview.chromium.org/2031053004
Cr-Commit-Position: refs/heads/master@{#397763}

[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceGradient.cpp
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceGradient.h
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceLinearGradient.cpp
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceLinearGradient.h
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceRadialGradient.cpp
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceRadialGradient.h
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/svg/SVGGradientElement.cpp
[modify] https://crrev.com/aaa44070561b21e6ea2d9ae2acd2b0676c5b853e/third_party/WebKit/Source/core/svg/SVGStopElement.h

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1f25a6c5efe6eaf1f5fd5d32593f60e4586e6fe

commit a1f25a6c5efe6eaf1f5fd5d32593f60e4586e6fe
Author: fmalita <fmalita@chromium.org>
Date: Thu Mar 16 18:24:26 2017

Remove Gradient point/radius setters

These are only called from CSSGradientValue to adjust the points/radii
immediately after Gradient instantiation.

Refactor CSSGradientValue to perform the adjustment before constructing
the Gradient object.  No functional changes.

BUG=614368
R=schenney@chromium.org,fs@opera.com

Review-Url: https://codereview.chromium.org/2754703003
Cr-Commit-Position: refs/heads/master@{#457498}

[modify] https://crrev.com/a1f25a6c5efe6eaf1f5fd5d32593f60e4586e6fe/third_party/WebKit/Source/core/css/CSSGradientValue.cpp
[modify] https://crrev.com/a1f25a6c5efe6eaf1f5fd5d32593f60e4586e6fe/third_party/WebKit/Source/core/css/CSSGradientValue.h
[modify] https://crrev.com/a1f25a6c5efe6eaf1f5fd5d32593f60e4586e6fe/third_party/WebKit/Source/platform/graphics/Gradient.h

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb6b69556d51265620cc759174e77484fc6c6067

commit fb6b69556d51265620cc759174e77484fc6c6067
Author: fmalita <fmalita@chromium.org>
Date: Fri Mar 31 20:23:19 2017

Split Gradient impl into separate classes

In preparation of adding a new gradient type (conic), refactor Gradient into separate, private
subclasses.

 * more specific factory names: createLinear(), createRadial()

 * remove unused getters: p0(), p1(), startRadius(), endRadius(), spreadMethod().

 * refactor isRadial() as a type getter.

 * relocate isZeroSize() to CanvasGradient (its only client).

Refactor-only, no functional changes.

BUG=614368

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2785203002
Cr-Commit-Position: refs/heads/master@{#461207}

[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/core/css/CSSGradientValue.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceLinearGradient.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceRadialGradient.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/core/paint/ThemePainterMac.mm
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.h
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/modules/canvas2d/CanvasGradient.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/modules/canvas2d/CanvasGradient.h
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/platform/graphics/Gradient.cpp
[modify] https://crrev.com/fb6b69556d51265620cc759174e77484fc6c6067/third_party/WebKit/Source/platform/graphics/Gradient.h

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55633a0e8db901aa6ce0990c99352b5d77087d06

commit 55633a0e8db901aa6ce0990c99352b5d77087d06
Author: fmalita <fmalita@chromium.org>
Date: Wed Apr 12 15:34:51 2017

Cleanup CSSGradientValue

1) move member fields to subclasses, to ensure they're only used/stored
   by gradient types which need them

2) remove setters; instead of manipulating fields after instantiation,
   pass all needed params to the ctor

3) refactor the CSS parsing methods for #2: defer gradient value
   instantiation until all needed params are parsed

4) constify member value pointers

Refactor only, no functional changes.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=614368

Review-Url: https://codereview.chromium.org/2817523002
Cr-Commit-Position: refs/heads/master@{#464034}

[modify] https://crrev.com/55633a0e8db901aa6ce0990c99352b5d77087d06/third_party/WebKit/Source/core/css/CSSGradientValue.cpp
[modify] https://crrev.com/55633a0e8db901aa6ce0990c99352b5d77087d06/third_party/WebKit/Source/core/css/CSSGradientValue.h
[modify] https://crrev.com/55633a0e8db901aa6ce0990c99352b5d77087d06/third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
[modify] https://crrev.com/55633a0e8db901aa6ce0990c99352b5d77087d06/third_party/WebKit/Source/core/paint/NinePieceImageGridTest.cpp

Sign in to add a comment