Closed
Bug 1382017
Opened 7 years ago
Closed 7 years ago
Various followup cleanups for the fusing of style contexts
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: manishearth, Assigned: manishearth)
References
Details
Attachments
(4 files, 1 obsolete file)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years 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•7 years 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•7 years 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•7 years ago
|
||
The second patch is being removed, it was moved to bug 1382019
Assignee | ||
Updated•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f66914e1a2a2 https://hg.mozilla.org/mozilla-central/rev/af628cf1f4db https://hg.mozilla.org/mozilla-central/rev/959eb43f301e https://hg.mozilla.org/mozilla-central/rev/cea58fa7bd22
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 32•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2772b4c382ff followup: fix alignment of macro definitions. r=whitespace-only
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2772b4c382ff
You need to log in
before you can comment on or make changes to this bug.
Description
•