Closed Bug 1268748 Opened 3 years ago Closed 3 years ago

Implement {Resolve,Probe}PseudoElementStyle

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

The servo side doesn't yet have the machinery to resolve non-eager selector-based pseudos, so I've hacked it up in my local tree to do something simple. Emilio said he'd look at making the required servo changes.
Comment on attachment 8746897 [details] [diff] [review]
Implement {Resolve,Probe}PseudoElementStyle. v1

Review of attachment 8746897 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.cpp
@@ +312,5 @@
>    MOZ_CRASH("stylo: shouldn't be calling Servo_GetComputedValuesForAnonymousBox in a "
>              "non-MOZ_STYLO build");
>  }
>  
> +ServoComputedValues* Servo_GetComputedValuesForPseudoElement(ServoComputedValues* parent_style,

Nit: newline before function name.  (I missed mentioning this in some of the other functions in this file.)

::: layout/style/ServoStyleSet.cpp
@@ +148,5 @@
>                                           nsStyleContext* aParentContext,
>                                           Element* aPseudoElement)
>  {
> +  MOZ_ASSERT(!aPseudoElement,
> +             "We don't support CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE yet");

"stylo: ..." for the message, just to be consistent with other assertions for missing stylo functionality we know we'll need to fix.

@@ +154,5 @@
> +  MOZ_ASSERT(aType < CSSPseudoElementType::Count);
> +  nsIAtom* pseudoTag = nsCSSPseudoElements::GetPseudoAtom(aType);
> +
> +  RefPtr<ServoComputedValues> computedValues =
> +    Servo_GetComputedValuesForPseudoElement(aParentContext->StyleSource().AsServoComputedValues(),

These long names (which I like) don't play well with 80 column limits (which I don't like, but try to keep to).  How about wrapping this as:

  RefPtr<SevoComputedValues> computedValues =
    Servo_GetComputedValuesForPseudoElement(
      aParentContext->StyleSource().AsServoComputedValues(),
      ...);

@@ +325,5 @@
> +  MOZ_ASSERT(aType < CSSPseudoElementType::Count);
> +  nsIAtom* pseudoTag = nsCSSPseudoElements::GetPseudoAtom(aType);
> +
> +  RefPtr<ServoComputedValues> computedValues =
> +    Servo_GetComputedValuesForPseudoElement(aParentContext->StyleSource().AsServoComputedValues(),

Here too.

@@ +328,5 @@
> +  RefPtr<ServoComputedValues> computedValues =
> +    Servo_GetComputedValuesForPseudoElement(aParentContext->StyleSource().AsServoComputedValues(),
> +                                            aParentElement, pseudoTag, mRawSet.get(),
> +                                            /* is_probe = */ true);
> +  if (!computedValues) {

Though I think, as I mentioned, it is probably only an optimization, since it is easy please do duplicate the logic from nsStyleSet::ProbePseudoElementStyle that returns null if the display or content properties are none when we're probing for ::before/::after style.

@@ +343,5 @@
>                                         TreeMatchContext& aTreeMatchContext,
>                                         Element* aPseudoElement)
>  {
> +  MOZ_ASSERT(!aPseudoElement,
> +             "We don't support CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE yet");

"stylo: ..." here too (or just rely on the assertion from the call to the other ProbePseudoElementStyle).
Attachment #8746897 - Flags: review?(cam)
Attachment #8747369 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/774c838c19e2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.