Various followup cleanups for the fusing of style contexts

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

9 months ago
Blocks: 1382019

Comment 3

9 months ago
mozreview-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/#review163862
Attachment #8887683 - Flags: review+

Comment 4

9 months ago
mozreview-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 5

9 months ago
mozreview-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+
(Assignee)

Comment 6

9 months ago
The second patch is being removed, it was moved to bug 1382019
(Assignee)

Updated

9 months ago
Attachment #8887683 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

9 months ago
mozreview-review
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+
(Assignee)

Comment 13

9 months ago
mozreview-review-reply
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 14

9 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

9 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

9 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

9 months ago
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

Comment 32

9 months ago
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.