Skip to content

Commit 97b052d

Browse files
darinadlerdarinadler
committed
Simplify gradient parsing
https://bugs.webkit.org/show_bug.cgi?id=208417 Reviewed by Anders Carlsson. - Use Optional<> and invalid Color to represent unspecified positions and colors. This is simpler and easier to get right than separate booleans. - Simplified sorting of stops in legacy gradients to remove extra CSS value evaluation and unnecessary "sort in place" technique. - Rewrote equals functions for CSS gradient value classes. The new pattern is to compare all the data members that hold parsed CSS data, handling null correctly, since the parser won't set inappropriate ones. The old code had complex logic to only compare certain data members, which was unnecessary and hard to read to tell if it was correct. - Added some more use of WTFMove to cut down on reference count churn. * css/CSSGradientValue.cpp: (WebCore::CSSGradientValue::image): Removed unneeded call to get(). (WebCore::compareStops): Deleted. (WebCore::CSSGradientValue::sortStopsIfNeeded): Deleted. (WebCore::resolveStopColors): Take advantage of the fact that we know because of parsing rules that the only stops without colors are midpoints to drastically simplify this function to a trivial loop. (WebCore::CSSGradientValue::hasColorDerivedFromElement const): Added. Checks to see if any of the stop colors is derived from the element. The old code confusingly would store the answer to this in the stop, but only in the first stop with this property. Computing it without modifying the stop, and memoizing it in the gradient preserves the same performance characteristics as before without requiring a boolean in each stop in the stops vector. (WebCore::CSSGradientValue::gradientWithStylesResolved): Call the new hasColorDerivedFromElement function instead of having the logic here. (WebCore::LinearGradientAdapter::normalizeStopsAndEndpointsOutsideRange): Update since GradientStop now has optional offsets. By the time this function is called they are all guaranteed to be filled in, so we can just use the * operator. (WebCore::RadialGradientAdapter::normalizeStopsAndEndpointsOutsideRange): Ditto. (WebCore::ConicGradientAdapter::normalizeStopsAndEndpointsOutsideRange): Ditto. (WebCore::CSSGradientValue::computeStops): Moved the sorting of stops for the deprecated gradients here. Also updated since Gradient::ColorStop no longer uses "m_" prefixes on its public struct data members. Some simplification because we no longer need to explicitly set "specified" to true since it's no longer a separate boolean. (WebCore::positionFromValue): Handle a null pointer for value by returning 0, which is what the caller was doing explicitly before. Use float instead of int for some internal computations that were mixing the two for no good reason. (WebCore::computeEndPoint): Removed null checks now that positionFromValue does them for us, turning this into a one-liner. (WebCore::CSSGradientValue::isCacheable const): Use hasColorDerivedFromElement. (WebCore::CSSGradientValue::knownToBeOpaque const): Removed unnnecessary checking the color both before and after when a color filter is involved. (WebCore::CSSGradientValue::equals const): Added. Shared by all the equals functions for derived classes. (WebCore::appendGradientStops): Updated for changes to CSSGradientColorStop. (WebCore::appendSpaceSeparatedOptionalCSSPtrText): Added template helper for writing two optional CSS values with a space between. (WebCore::writeColorStop): Ditto. Also converted to non-member function, removed unneeded isMidpoint check, use appendSpaceSeparatedOptionalCSSPtrText. (WebCore::CSSLinearGradientValue::customCSSText const): Call function members so we don't have to expose CSSGradientValue data members as protected things that can be accessed by derived classes. Some other small refactoring, such as getting rid of extra boolean wroteFirstStop. (WebCore::CSSLinearGradientValue::createGradient): Updated to use function members instead of protected data members. (WebCore::CSSLinearGradientValue::equals const): Compare all data members and use CSSGradientValue::equals, makes this a 1-liner. (WebCore::CSSRadialGradientValue::customCSSText const): Call function members as described above and use appendSpaceSeparatedOptionalCSSPtrText. (WebCore::CSSRadialGradientValue::createGradient): Ditto. (WebCore::CSSRadialGradientValue::equals const): Compare all data members and use CSSGradientValue::equals. (WebCore::CSSConicGradientValue::customCSSText const): Call function members as described above and use appendSpaceSeparatedOptionalCSSPtrText. (WebCore::CSSConicGradientValue::createGradient): Ditto. (WebCore::CSSConicGradientValue::equals const): Compare all data members and use CSSGradientValue::equals, makes this a 1-liner. * css/CSSGradientValue.h: Removed unneeded includes and forward declarations. Renamed CSSGradientColorStop data members to not use m_ prefix since this is a struct with public data members, and WebKit style says not to do that here. Removed m_colorIsDerivedFromElement and isMidpoint from CSSGradientColorStop, m_colorIsDerivedFromElement is now stored in the gradient, not the color stop, and midpoints are any color stop with null color. Replaced the CSSGradientValue::stopCount function, which mixed size_t and unsigned types, with a hasTwoStops function, which is all the caller needs. Converted the isFixedSize, fixedSize, isPending, and loadSubimages into static member functions: they don't do any work and so don't need an instance. Removed the unneeded gradient type argument to the cloning constructors. Removed m_stopsSorted and added m_hasColorDerivedFromElement and hasColorDerivedFromElement. Added getter functions that are protected so the data members themselves can be private. Removed sortStopsIfNeeded and writeColorStop. * css/parser/CSSPropertyParserHelpers.cpp: (WebCore::CSSPropertyParserHelpers::consumeDeprecatedGradientColorStop): Updated for CSSGradientColorStop member renaming. (WebCore::CSSPropertyParserHelpers::consumeDeprecatedGradient): Use more WTFMove to save a little bit of reference count churn; in some cases that means moving the setter calls to the end of the function after all the error checking. (WebCore::CSSPropertyParserHelpers::consumeGradientColorStops): Ditto. Removed code to set isMidpoint and the FIXME-NEWPARSER comment that said it could be removed. Used lambda to cut down on repeated code. Changed parsing of stops with a second position to repeat the color instead of relying on later computation to repeat it; this is required so we can always treat an omitted color as a midpoint. (WebCore::CSSPropertyParserHelpers::consumeDeprecatedRadialGradient): Ditto. (WebCore::CSSPropertyParserHelpers::consumeRadialGradient): Ditto. (WebCore::CSSPropertyParserHelpers::consumeLinearGradient): Ditto. (WebCore::CSSPropertyParserHelpers::consumeConicGradient): Ditto. * html/HTMLInputElement.cpp: (WebCore::autoFillStrongPasswordMaskImage): Updated for the renamed CSSGradientColorStop members, added a missing call to doneAddingStops, and use some WTFMove to cut down on reference count churn. Canonical link: https://commits.webkit.org/221597@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@257966 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent a7f066a commit 97b052d

5 files changed

Lines changed: 433 additions & 472 deletions

File tree

‎Source/WebCore/ChangeLog‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,123 @@
1+
2020-02-29 Darin Adler <[email protected]>
2+
3+
Simplify gradient parsing
4+
https://bugs.webkit.org/show_bug.cgi?id=208417
5+
6+
Reviewed by Anders Carlsson.
7+
8+
- Use Optional<> and invalid Color to represent unspecified positions and colors.
9+
This is simpler and easier to get right than separate booleans.
10+
- Simplified sorting of stops in legacy gradients to remove extra CSS value
11+
evaluation and unnecessary "sort in place" technique.
12+
- Rewrote equals functions for CSS gradient value classes. The new pattern is
13+
to compare all the data members that hold parsed CSS data, handling null
14+
correctly, since the parser won't set inappropriate ones. The old code had
15+
complex logic to only compare certain data members, which was unnecessary
16+
and hard to read to tell if it was correct.
17+
- Added some more use of WTFMove to cut down on reference count churn.
18+
19+
* css/CSSGradientValue.cpp:
20+
(WebCore::CSSGradientValue::image): Removed unneeded call to get().
21+
(WebCore::compareStops): Deleted.
22+
(WebCore::CSSGradientValue::sortStopsIfNeeded): Deleted.
23+
(WebCore::resolveStopColors): Take advantage of the fact that we know because
24+
of parsing rules that the only stops without colors are midpoints to drastically
25+
simplify this function to a trivial loop.
26+
(WebCore::CSSGradientValue::hasColorDerivedFromElement const): Added.
27+
Checks to see if any of the stop colors is derived from the element. The old
28+
code confusingly would store the answer to this in the stop, but only in the
29+
first stop with this property. Computing it without modifying the stop, and
30+
memoizing it in the gradient preserves the same performance characteristics
31+
as before without requiring a boolean in each stop in the stops vector.
32+
(WebCore::CSSGradientValue::gradientWithStylesResolved): Call the new
33+
hasColorDerivedFromElement function instead of having the logic here.
34+
(WebCore::LinearGradientAdapter::normalizeStopsAndEndpointsOutsideRange):
35+
Update since GradientStop now has optional offsets. By the time this
36+
function is called they are all guaranteed to be filled in, so we can
37+
just use the * operator.
38+
(WebCore::RadialGradientAdapter::normalizeStopsAndEndpointsOutsideRange):
39+
Ditto.
40+
(WebCore::ConicGradientAdapter::normalizeStopsAndEndpointsOutsideRange):
41+
Ditto.
42+
(WebCore::CSSGradientValue::computeStops): Moved the sorting of stops for
43+
the deprecated gradients here. Also updated since Gradient::ColorStop
44+
no longer uses "m_" prefixes on its public struct data members. Some
45+
simplification because we no longer need to explicitly set "specified"
46+
to true since it's no longer a separate boolean.
47+
(WebCore::positionFromValue): Handle a null pointer for value by returning
48+
0, which is what the caller was doing explicitly before. Use float
49+
instead of int for some internal computations that were mixing the two
50+
for no good reason.
51+
(WebCore::computeEndPoint): Removed null checks now that positionFromValue
52+
does them for us, turning this into a one-liner.
53+
(WebCore::CSSGradientValue::isCacheable const): Use hasColorDerivedFromElement.
54+
(WebCore::CSSGradientValue::knownToBeOpaque const): Removed unnnecessary
55+
checking the color both before and after when a color filter is involved.
56+
(WebCore::CSSGradientValue::equals const): Added. Shared by all the equals
57+
functions for derived classes.
58+
(WebCore::appendGradientStops): Updated for changes to CSSGradientColorStop.
59+
(WebCore::appendSpaceSeparatedOptionalCSSPtrText): Added template helper
60+
for writing two optional CSS values with a space between.
61+
(WebCore::writeColorStop): Ditto. Also converted to non-member function,
62+
removed unneeded isMidpoint check, use appendSpaceSeparatedOptionalCSSPtrText.
63+
(WebCore::CSSLinearGradientValue::customCSSText const): Call function
64+
members so we don't have to expose CSSGradientValue data members as
65+
protected things that can be accessed by derived classes. Some other
66+
small refactoring, such as getting rid of extra boolean wroteFirstStop.
67+
(WebCore::CSSLinearGradientValue::createGradient): Updated to use
68+
function members instead of protected data members.
69+
(WebCore::CSSLinearGradientValue::equals const): Compare all data
70+
members and use CSSGradientValue::equals, makes this a 1-liner.
71+
(WebCore::CSSRadialGradientValue::customCSSText const): Call function
72+
members as described above and use appendSpaceSeparatedOptionalCSSPtrText.
73+
(WebCore::CSSRadialGradientValue::createGradient): Ditto.
74+
(WebCore::CSSRadialGradientValue::equals const): Compare all data
75+
members and use CSSGradientValue::equals.
76+
(WebCore::CSSConicGradientValue::customCSSText const): Call function
77+
members as described above and use appendSpaceSeparatedOptionalCSSPtrText.
78+
(WebCore::CSSConicGradientValue::createGradient): Ditto.
79+
(WebCore::CSSConicGradientValue::equals const): Compare all data
80+
members and use CSSGradientValue::equals, makes this a 1-liner.
81+
82+
* css/CSSGradientValue.h: Removed unneeded includes and forward declarations.
83+
Renamed CSSGradientColorStop data members to not use m_ prefix since this is
84+
a struct with public data members, and WebKit style says not to do that here.
85+
Removed m_colorIsDerivedFromElement and isMidpoint from CSSGradientColorStop,
86+
m_colorIsDerivedFromElement is now stored in the gradient, not the color stop,
87+
and midpoints are any color stop with null color. Replaced the
88+
CSSGradientValue::stopCount function, which mixed size_t and unsigned types,
89+
with a hasTwoStops function, which is all the caller needs. Converted the
90+
isFixedSize, fixedSize, isPending, and loadSubimages into static member
91+
functions: they don't do any work and so don't need an instance. Removed
92+
the unneeded gradient type argument to the cloning constructors. Removed
93+
m_stopsSorted and added m_hasColorDerivedFromElement and
94+
hasColorDerivedFromElement. Added getter functions that are protected so
95+
the data members themselves can be private. Removed sortStopsIfNeeded
96+
and writeColorStop.
97+
98+
* css/parser/CSSPropertyParserHelpers.cpp:
99+
(WebCore::CSSPropertyParserHelpers::consumeDeprecatedGradientColorStop):
100+
Updated for CSSGradientColorStop member renaming.
101+
(WebCore::CSSPropertyParserHelpers::consumeDeprecatedGradient): Use
102+
more WTFMove to save a little bit of reference count churn; in some cases
103+
that means moving the setter calls to the end of the function after all
104+
the error checking.
105+
(WebCore::CSSPropertyParserHelpers::consumeGradientColorStops): Ditto.
106+
Removed code to set isMidpoint and the FIXME-NEWPARSER comment that said
107+
it could be removed. Used lambda to cut down on repeated code. Changed
108+
parsing of stops with a second position to repeat the color instead of
109+
relying on later computation to repeat it; this is required so we can
110+
always treat an omitted color as a midpoint.
111+
(WebCore::CSSPropertyParserHelpers::consumeDeprecatedRadialGradient): Ditto.
112+
(WebCore::CSSPropertyParserHelpers::consumeRadialGradient): Ditto.
113+
(WebCore::CSSPropertyParserHelpers::consumeLinearGradient): Ditto.
114+
(WebCore::CSSPropertyParserHelpers::consumeConicGradient): Ditto.
115+
116+
* html/HTMLInputElement.cpp:
117+
(WebCore::autoFillStrongPasswordMaskImage): Updated for the renamed
118+
CSSGradientColorStop members, added a missing call to doneAddingStops,
119+
and use some WTFMove to cut down on reference count churn.
120+
1121
2020-03-05 Said Abou-Hallawa <[email protected]>
2122

3123
Remove the optimization for discarding no operation DisplayList items between Save and Restore items

0 commit comments

Comments
 (0)