Bug 220079 - Move the space transform outside the Gradient class
Summary: Move the space transform outside the Gradient class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 219468
  Show dependency treegraph
 
Reported: 2020-12-21 20:41 PST by Said Abou-Hallawa
Modified: 2021-01-25 11:28 PST (History)
15 users (show)

See Also:


Attachments
Patch (39.56 KB, patch)
2020-12-21 20:53 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (44.98 KB, patch)
2020-12-21 21:18 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (45.27 KB, patch)
2020-12-21 21:45 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (31.74 KB, patch)
2021-01-12 23:01 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-12-21 20:41:49 PST
The goal is get the suitable abstraction for the Gradient class so we can share it among different clients easier.
Comment 1 Said Abou-Hallawa 2020-12-21 20:53:24 PST
Created attachment 416650 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-12-21 21:18:09 PST
Created attachment 416651 [details]
Patch
Comment 3 Said Abou-Hallawa 2020-12-21 21:45:51 PST
Created attachment 416653 [details]
Patch
Comment 4 Simon Fraser (smfr) 2020-12-22 08:48:48 PST
Comment on attachment 416653 [details]
Patch

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

> Source/WebCore/platform/graphics/Gradient.h:77
> +        GradientSpreadMethod spreadMethod { GradientSpreadMethod::Pad };

I think you can keep GradientSpreadMethod as part of Gradient. All existing code sets it at the same time as the color stops. It's only gradientSpaceTransform that's change per-paint (see RenderSVGResourceGradient::applyResource()).
Comment 5 Said Abou-Hallawa 2020-12-22 10:39:38 PST
Comment on attachment 416653 [details]
Patch

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

>> Source/WebCore/platform/graphics/Gradient.h:77
>> +        GradientSpreadMethod spreadMethod { GradientSpreadMethod::Pad };
> 
> I think you can keep GradientSpreadMethod as part of Gradient. All existing code sets it at the same time as the color stops. It's only gradientSpaceTransform that's change per-paint (see RenderSVGResourceGradient::applyResource()).

I looked at CanvasGradient https://developer.mozilla.org/en-US/docs/Web/API/CanvasGradient and tried to make the Gradient class store what it stores: gradient data and color stops.
Comment 6 Simon Fraser (smfr) 2020-12-22 11:00:46 PST
(In reply to Said Abou-Hallawa from comment #5)
> Comment on attachment 416653 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416653&action=review
> 
> >> Source/WebCore/platform/graphics/Gradient.h:77
> >> +        GradientSpreadMethod spreadMethod { GradientSpreadMethod::Pad };
> > 
> > I think you can keep GradientSpreadMethod as part of Gradient. All existing code sets it at the same time as the color stops. It's only gradientSpaceTransform that's change per-paint (see RenderSVGResourceGradient::applyResource()).
> 
> I looked at CanvasGradient
> https://developer.mozilla.org/en-US/docs/Web/API/CanvasGradient and tried to
> make the Gradient class store what it stores: gradient data and color stops.

Right but the spread method isn't exposed for canvas gradients.
Comment 7 Radar WebKit Bug Importer 2020-12-28 20:42:13 PST
<rdar://problem/72715158>
Comment 8 Said Abou-Hallawa 2021-01-12 23:01:29 PST
Created attachment 417509 [details]
Patch
Comment 9 Simon Fraser (smfr) 2021-01-13 09:37:53 PST
Comment on attachment 417509 [details]
Patch

Nice!
Comment 10 EWS 2021-01-13 16:07:30 PST
Committed r271472: <https://trac.webkit.org/changeset/271472>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417509 [details].