1376077 - ProbePseudoElementStyle's aPseudoElement argument doesn't make much sense

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez [:emilio]]

Why would we want to probe a pseudo if you already have one?

Also, it's unused.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1376077: Remove aPseudoElement argument from the StyleSet classes.

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

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8881017 [details]
Bug 1376077: Remove aPseudoElement argument from the StyleSet classes.

https://reviewboard.mozilla.org/r/152388/#review157472

::: layout/style/StyleSetHandleInlines.h:273
(Diff revision 1)
> -                                              *aTreeMatchContext, aPseudoElement);
> +                                              *aTreeMatchContext);
> -  } else {
> -    return AsServo()->ProbePseudoElementStyle(aParentElement, aType, aParentContext,
> -                                              aPseudoElement);
>    }
> +  return AsServo()->ProbePseudoElementStyle(aParentElement, aType, aParentContext);

This is one of the style guidelines that I don't always like.  For cases like this, where the two branches are similar, I find the symmetry/alignment makes the code easier to read.  Oh well.  :-)

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8881017 [details]
Bug 1376077: Remove aPseudoElement argument from the StyleSet classes.

https://reviewboard.mozilla.org/r/152388/#review157474

::: layout/style/StyleSetHandleInlines.h:273
(Diff revision 1)
> -                                              *aTreeMatchContext, aPseudoElement);
> +                                              *aTreeMatchContext);
> -  } else {
> -    return AsServo()->ProbePseudoElementStyle(aParentElement, aType, aParentContext,
> -                                              aPseudoElement);
>    }
> +  return AsServo()->ProbePseudoElementStyle(aParentElement, aType, aParentContext);

I guess we could use something like:

```
MOZ_ASSERT_IF(IsGecko(), aTreeMatchContext);
return IsGecko()
  ? AsGecko()->Probe(...)
  : AsServo()->Probe(...);
```

I don't mind a lot either way... This is one of the cases where it's easier to read with rust's "ifs are expressions too" :P

I'll leave it for now as-is, because I don't feel strongly about it really, but you can land either the previous structure (guidelines are not rules, I guess, and in this case I see your point) or the one I suggested with r=me if you want :)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7f1f58665ac
Remove aPseudoElement argument from the StyleSet classes. r=heycam

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7f1f58665ac