Closed Bug 1382017 Opened 2 years ago Closed 2 years ago

Various followup cleanups for the fusing of style contexts

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(4 files, 1 obsolete file)

No description provided.
Blocks: 1382019
Comment on attachment 8887683 [details]
Bug 1382017 - stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ;

https://reviewboard.mozilla.org/r/158580/#review163862
Attachment #8887683 - Flags: review+
Comment on attachment 8887682 [details]
Bug 1382017 - stylo: Replace stylearc with servo_arc;

https://reviewboard.mozilla.org/r/158578/#review163864
Attachment #8887682 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8887683 [details]
Bug 1382017 - stylo: Don't crash on null parent contexts in nsCSSFrameConstructor::AddFrameConstructionItemsInternal ;

https://reviewboard.mozilla.org/r/158580/#review163868

::: layout/base/nsCSSFrameConstructor.cpp:5890
(Diff revision 1)
>            // Servo's should_traverse_children() in traversal.rs skips
>            // styling descendants of elements with a -moz-binding the
>            // first time. Thus call StyleNewChildren() again.
>            styleSet->StyleNewChildren(element);
>  
> +          auto parent = styleContext->GetParentAllowServo();

Please write down `nsStyleContext*` here, or at least `auto*` rather than just `auto`.

::: layout/base/nsCSSFrameConstructor.cpp:5891
(Diff revision 1)
>            // styling descendants of elements with a -moz-binding the
>            // first time. Thus call StyleNewChildren() again.
>            styleSet->StyleNewChildren(element);
>  
> +          auto parent = styleContext->GetParentAllowServo();
> +          ServoStyleContext* parentServo =  parent ? parent->AsServo() : nullptr;

There is a redundant whitespace between `=` and `parent`.
Attachment #8887683 - Flags: review?(xidorn+moz) → review+
The second patch is being removed, it was moved to bug 1382019
Attachment #8887683 - Attachment is obsolete: true
Comment on attachment 8888192 [details]
Bug 1382017 - stylo: Remove usage of ServoComputedValues from binding functions;

https://reviewboard.mozilla.org/r/159124/#review164516

::: servo/components/style/gecko_bindings/sugar/ownership.rs:212
(Diff revision 1)
>      #[inline]
>      /// Produces a null strong FFI reference.
>      pub fn null() -> Self {
>          unsafe { transmute(ptr::null::<GeckoType>()) }
>      }
> +

Unrelated change?

::: layout/style/nsStyleContext.cpp:230
(Diff revision 3)
>      } else {
>        *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
>      }
>    } else {
>      if (Servo_ComputedValues_EqualCustomProperties(
> -          ComputedValues(),
> +          this->ComputedValues(),

`AsServo()`?

::: layout/style/nsStyleContext.cpp:231
(Diff revision 3)
>        *aEqualStructs |= NS_STYLE_INHERIT_BIT(Variables);
>      }
>    } else {
>      if (Servo_ComputedValues_EqualCustomProperties(
> -          ComputedValues(),
> +          this->ComputedValues(),
>            aNewContext->ComputedValues())) {

`aNewContext->AsServo()`?

::: servo/components/style/gecko/arc_types.rs:135
(Diff revision 3)
> +impl From<Arc<ComputedValues>> for Strong<ComputedValues> {
> +    fn from(arc: Arc<ComputedValues>) -> Self {
> +        unsafe { mem::transmute(Arc::into_raw_offset(arc)) }
> +    }
> +}

Could this be generic like `impl<T> From<Arc<T>> for Strong<T>` and put into sugar/ownership.rs instead here?

::: servo/components/style/gecko_bindings/sugar/ownership.rs:212
(Diff revision 3)
>      #[inline]
>      /// Produces a null strong FFI reference.
>      pub fn null() -> Self {
>          unsafe { transmute(ptr::null::<GeckoType>()) }
>      }
> +

unrelated change?
Attachment #8888192 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8888192 [details]
Bug 1382017 - stylo: Remove usage of ServoComputedValues from binding functions;

https://reviewboard.mozilla.org/r/159124/#review164516

> `AsServo()`?

heycam is fixing these

> Could this be generic like `impl<T> From<Arc<T>> for Strong<T>` and put into sugar/ownership.rs instead here?

I'd rather not be doing this for arbitrary types because we only use `Strong<ComputedValues>` in this paradigm. In all the other cases T and U are different.
Comment on attachment 8888192 [details]
Bug 1382017 - stylo: Remove usage of ServoComputedValues from binding functions;

https://reviewboard.mozilla.org/r/159124/#review164516

> I'd rather not be doing this for arbitrary types because we only use `Strong<ComputedValues>` in this paradigm. In all the other cases T and U are different.

OK, makes sense.
Comment on attachment 8888583 [details]
Bug 1382017 - stylo: Remove usage of ServoComputedValues from most Gecko code;

https://reviewboard.mozilla.org/r/159566/#review164940
Attachment #8888583 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8888646 [details]
Bug 1382017 - stylo: Rename ServoComputedValues -> ServoComputedData;

https://reviewboard.mozilla.org/r/159672/#review165104

::: layout/style/ServoBindings.cpp:236
(Diff revision 4)
> -    const ServoComputedValuesForgotten aValue)
> +    const ServoComputedDataForgotten aValue)
>  {
>    PodAssign(this, aValue.mPtr);
>  }
>  
> -const nsStyleVariables* ServoComputedValues::GetStyleVariables() const
> +const nsStyleVariables* ServoComputedData::GetStyleVariables() const

Nit: add a newline after "nsStyleVariables*" while you're here?

::: layout/style/ServoStyleContext.h:28
(Diff revision 4)
>                      nsIAtom* aPseudoTag,
>                      CSSPseudoElementType aPseudoType,
> -                    ServoComputedValuesForgotten aComputedValues);
> +                    ServoComputedDataForgotten aComputedValues);
>  
>    nsPresContext* PresContext() const { return mPresContext; }
> -  const ServoComputedValues* ComputedValues() const { return &mSource; }
> +  const ServoComputedData* ComputedValues() const { return &mSource; }

Does it make sense to rename ComputedValues() as well?

::: layout/style/ServoTypes.h:179
(Diff revision 4)
>  
>  class ServoStyleContext;
>  
>  struct ServoVisitedStyle {
>    // This is actually a strong reference
> -  // but ServoComputedValues' destructor is
> +  // but ServoComputedData' destructor is

"s" after the apostrophe now.  Although more importantly, please rewrap this comment so it takes full advantage of the 80 columns the universe has provided us with. :-)
Attachment #8888646 - Flags: review?(cam) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f66914e1a2a2
part 1 Gecko side - Replace stylearc with servo_arc; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/af628cf1f4db
part 2 Gecko piece - Remove usage of ServoComputedValues from binding functions; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/959eb43f301e
part 4 Gecko piece - Remove usage of ServoComputedValues from most Gecko code; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/cea58fa7bd22
part 4 Gecko piece - Rename ServoComputedValues -> ServoComputedData; r=heycam
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2772b4c382ff
followup: fix alignment of macro definitions. r=whitespace-only
You need to log in before you can comment on or make changes to this bug.