1303241 - Make GetVisitedDependentColor not use StyleAnimationValue::ExtractComputedValue

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

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).

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1303241 part 1 - Move visited-dependent style fields into a list file.

Review commit: https://reviewboard.mozilla.org/r/101722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101722/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

I think there are three advantages of this change:
1. removes some dependencies from layout / painting code to pre-computed
   value stuff in the style system;
2. makes it easier to audit usage of specific fields in style structs
   (which is probably a side effect of the first one);
3. potentially improves performance since it doesn't go through the
   unnecessary general logic in ExtractComputedValue.

Also, combined with the part before, we get a unified list for visited-
dependent properties so that we can ensure the assertion here and the
style difference calc code are consistent.

Review commit: https://reviewboard.mozilla.org/r/101724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101724/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b3e1a29520afb4b665f8f63d04bcdfc5b338b4

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 7

[Xidorn Quan [:xidorn] UTC+10]

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 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8823146 [details]
Bug 1303241 part 1 - Move visited-dependent style fields into a list file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101722/diff/1-2/

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101724/diff/1-2/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101724/diff/2-3/

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8823147 [details]
Bug 1303241 part 2 - Make GetVisitedDependentColor use style structs directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101724/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab9310993e1d
https://hg.mozilla.org/mozilla-central/rev/b144475237d3

Comment 16

[Gerry Chang [:gchang]]

We've built 51 RC. Mark 51 won't fix.