1448665 - Simplify the unanimated style setup in nsComputedDOMStyle.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

It's really complicated, and not for a good reason right now, I'd say.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1448665: Simplify the unanimated style setup in nsComputedDOMStyle.

We never create nsComputedDOMStyle objects with an unanimated computed style, so
this can be much simpler.

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

Comment 2

[Jonathan Watt [:jwatt]]

Worth checking with devtools whether they want to keep this before removing it?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Watt [:jwatt] (back April 2nd) from comment #2)
> Worth checking with devtools whether they want to keep this before removing
> it?

We don't remove the API, we just simplify and deduplicate how's implemented, and removes paths that aren't taken.

The change should be idempotent.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8962159 [details]
Bug 1448665: Simplify the unanimated style setup in nsComputedDOMStyle.

https://reviewboard.mozilla.org/r/231018/#review236462

r=me with the comments addressed.

::: layout/style/nsComputedDOMStyle.cpp:521
(Diff revision 1)
> -  // We do this check to avoid having to add too much special casing of
> -  // Servo functions we call to explicitly ignore any element data in
> +  auto pseudoType = GetPseudoType(aPseudo);
> +  if (aPseudo && pseudoType >= CSSPseudoElementType::Count) {
> -  // the tree. (See comment in ServoStyleSet::ResolveStyleLazily.)
> -  MOZ_RELEASE_ASSERT((aStyleType == eAll && aAnimationFlag == eWithAnimation) ||
> -                     !aElement->OwnerDoc()->GetBFCacheEntry(),
> -                     "nsComputedDOMStyle doesn't support getting styles without "
> -                     "document rules or without animation for documents in the "
> -                     "bfcache");

It is not clear to me why is this assertion removed. Did we resolve this problem so that it no longer applies? If not, please keep it, otherwise, please remove it in a separate commit with commit message explaining why this is not needed.

::: layout/style/nsComputedDOMStyle.cpp:521
(Diff revision 1)
>      if (!presShell) {
>        return nullptr;
>      }
>    }
>  
> -  // We do this check to avoid having to add too much special casing of
> +  auto pseudoType = GetPseudoType(aPseudo);

I'd prefer you to write down the type here (and below) explicitly since it isn't all that clear what it is without looking at the definition of the function.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Comment on attachment 8962159 [details]
> Bug 1448665: Simplify the unanimated style setup in nsComputedDOMStyle.
> 
> https://reviewboard.mozilla.org/r/231018/#review236462
> 
> r=me with the comments addressed.
> 
> ::: layout/style/nsComputedDOMStyle.cpp:521
> (Diff revision 1)
> > -  // We do this check to avoid having to add too much special casing of
> > -  // Servo functions we call to explicitly ignore any element data in
> > +  auto pseudoType = GetPseudoType(aPseudo);
> > +  if (aPseudo && pseudoType >= CSSPseudoElementType::Count) {
> > -  // the tree. (See comment in ServoStyleSet::ResolveStyleLazily.)
> > -  MOZ_RELEASE_ASSERT((aStyleType == eAll && aAnimationFlag == eWithAnimation) ||
> > -                     !aElement->OwnerDoc()->GetBFCacheEntry(),
> > -                     "nsComputedDOMStyle doesn't support getting styles without "
> > -                     "document rules or without animation for documents in the "
> > -                     "bfcache");
> 
> It is not clear to me why is this assertion removed. Did we resolve this
> problem so that it no longer applies? If not, please keep it, otherwise,
> please remove it in a separate commit with commit message explaining why
> this is not needed.

Yeah, this is not needed since bug 1414999.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0671a7d7cc58
Remove assertion that is no longer relevant. rs=xidorn
https://hg.mozilla.org/integration/autoland/rev/8191223ad8a3
Simplify the unanimated style setup in nsComputedDOMStyle. r=xidorn

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0671a7d7cc58
https://hg.mozilla.org/mozilla-central/rev/8191223ad8a3