Closed Bug 1303241 Opened 3 years ago Closed 3 years ago

Make GetVisitedDependentColor not use StyleAnimationValue::ExtractComputedValue

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

After bug 1216843, using StyleAnimationValue::ExtractComputedValue for color values become much more expensive than before, due to the additional allocation.

We may want to make GetVisitedDependentColor read data from the style struct directly instead of using StyleAnimationValue::ExtractComputedValue.

After bug 1299741 and bug 760345 gets implemented, the majority of color values would be stored as a StyleComplexColor in style struct, which makes it easier to use a unified logic.

Border colors may need some more investigation, as well as the paint server properties (fill and stroke).
Depends on: 1326209
Severity: normal → enhancement
Comment on attachment 8823146 [details]
Bug 1303241 part 1 - Move visited-dependent style fields into a list file.

https://reviewboard.mozilla.org/r/101722/#review102704

::: layout/style/nsCSSVisPropList.h:13
(Diff revision 1)
> +
> +/* This file contains the list of all style structs and fields that can
> + * be visited-dependent. Each entry is defined as a STYLE_STRUCT macro
> + * with the following parameters:
> + * - 'name_' the name of the style struct
> + * - 'fields_' the list of style fields which can be visited-dependent

maybe s/style fields/fields (each representing the computed value of a property)/

(The use of the term "field" here also feels a little odd to me -- maybe "member variable" is better?  But maybe it's ok...)

Also, s/which/that/, since it's a restriction on the fields/properties

::: layout/style/nsCSSVisPropList.h:15
(Diff revision 1)
> + * be visited-dependent. Each entry is defined as a STYLE_STRUCT macro
> + * with the following parameters:
> + * - 'name_' the name of the style struct
> + * - 'fields_' the list of style fields which can be visited-dependent
> + *
> + * Users of this list should define a macro STYLE_STRUCT(name, fields_)

missing underscore in "name_"

::: layout/style/nsCSSVisPropList.h:18
(Diff revision 1)
> + * - 'fields_' the list of style fields which can be visited-dependent
> + *
> + * Users of this list should define a macro STYLE_STRUCT(name, fields_)
> + * before including this file.
> + *
> + * Note that, currently there is a restriction that all fields in a

Use a comma on both sides of "currently" or neither, but not only one.
Attachment #8823146 - Flags: review?(dbaron) → review+
Oh, and another comment on part 1 that I forgot to write because I was thinking about where to write it -- the filename is too short -- it should be "VisitedDependent" instead of just "Vis".
Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

https://reviewboard.mozilla.org/r/101724/#review102708

::: layout/base/nsLayoutUtils.h:1554
(Diff revision 1)
> -  // Get a suitable foreground color for painting aProperty for aFrame.
> -  static nscolor GetColor(nsIFrame* aFrame, nsCSSPropertyID aProperty);
> +  // Get a suitable foreground color for painting aColor for aFrame.
> +  static nscolor DarkenColorIfNeeded(nsIFrame* aFrame, nscolor aColor);
> +
> +  // Get a suitable foreground color for painting aField for aFrame.
> +  // Type of aFrame is made a template parameter because nsIFrame is not
> +  // a complete type in the header. Type-safety is not hurted given that

s/hurted/hurt/, although maybe harmed would be better

::: layout/style/nsStyleContext.h
(Diff revision 1)
> -   *
> -   * aProperty must be a color-valued property that StyleAnimationValue
> -   * knows how to extract.  It must also be a property that we know to
> -   * do change handling for in nsStyleContext::CalcDifference.

Please add a new comment saying what aField needs to be.

::: layout/style/nsStyleContext.cpp:1427
(Diff revision 1)
> +  nsStyleContext::GetVisitedDependentColor(                                   \
> +    decltype(nsStyle##name_::MOZ_ARG_1 fields_) nsStyle##name_::* aField)     \

I don't understand how it's OK that the template specializations aren't declared in the header file.  What makes something in a different translation unit use the correct code?

::: layout/style/nsStyleContext.cpp:1433
(Diff revision 1)
> +    decltype(nsStyle##name_::MOZ_ARG_1 fields_) nsStyle##name_::* aField)     \
> +  {                                                                           \
> +    MOZ_ASSERT(MOZ_FOR_EACH(STYLE_FIELD, (nsStyle##name_,), fields_) false,   \
> +               "Getting visited-dependent color for a field in nsStyle"#name_ \
> +               " which is not listed in nsCSSVisPropList.h");                 \
> +    return GetVisitedDependentColorInternal(this, [=](nsStyleContext* sc) {   \

Please explicitly capture aField rather than capturing everything.
Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

https://reviewboard.mozilla.org/r/101724/#review102708

> I don't understand how it's OK that the template specializations aren't declared in the header file.  What makes something in a different translation unit use the correct code?

As far as there is no function body defined in the header file, the compiler does not try to instantiate anything when it sees the reference to this function template. It would just specialize the template declaration with the template parameters. The specialized declaration just behaves like a normal function declaration, which has a symbol, and is linked in the link time.

So in this case, if someone uses `GetVisitedDependentColor` with a class which is not explicit specialized anywhere (e.g. `Foo`), they would get a link time error that there is no definition of `nsStyleContext::GetVisitedDependentColor<Foo>`.
Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

https://reviewboard.mozilla.org/r/101724/#review102744

::: layout/style/nsStyleContext.cpp:1433
(Diff revisions 1 - 3)
>    {                                                                           \
>      MOZ_ASSERT(MOZ_FOR_EACH(STYLE_FIELD, (nsStyle##name_,), fields_) false,   \
>                 "Getting visited-dependent color for a field in nsStyle"#name_ \
> -               " which is not listed in nsCSSVisPropList.h");                 \
> -    return GetVisitedDependentColorInternal(this, [=](nsStyleContext* sc) {   \
> +               " which is not listed in nsCSSVisitedDependentPropList.h");    \
> +    return GetVisitedDependentColorInternal(this,                             \
> +                                            [&aField](nsStyleContext* sc) {   \

why not capture by value (like before) rather than by reference?
Attachment #8823147 - Flags: review?(dbaron) → review+
Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

https://reviewboard.mozilla.org/r/101724/#review102744

> why not capture by value (like before) rather than by reference?

I somehow thought pointer to member variable is a fat pointer... which is wrong. I was confused by pointer to member function, which is fat. Will fix it.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab9310993e1d
part 1 - Move visited-dependent style fields into a list file. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/b144475237d3
part 2 - Make GetVisitedDependentColor use style structs directly. r=dbaron
Assignee: nobody → xidorn+moz
https://hg.mozilla.org/mozilla-central/rev/ab9310993e1d
https://hg.mozilla.org/mozilla-central/rev/b144475237d3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
We've built 51 RC. Mark 51 won't fix.
You need to log in before you can comment on or make changes to this bug.