Closed
Bug 1455784
Opened 6 years ago
Closed 6 years ago
Use a tagged union to shrink StyleSource
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
Details
Attachments
(3 files, 1 obsolete file)
5.09 KB,
patch
|
manishearth
:
feedback+
|
Details | Diff | Splinter Review |
30.84 KB,
patch
|
manishearth
:
review+
|
Details | Diff | Splinter Review |
6.84 KB,
patch
|
manishearth
:
review+
|
Details | Diff | Splinter Review |
right now StyleSource is a three-variant enum, with one of them being "None" to avoid consuming a third word. We could turn this all into a one-word Option<ArcUnion<Rule, Declaration>> with the appropriate machinery. I hacked together a quick implementation of ArcUnion. Haven't finished hooking it up to StyleSource, but looking for quick feedback on the approach and ergonomics.
Assignee | ||
Comment 1•6 years ago
|
||
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8969836 -
Flags: feedback?(manishearth)
Comment 2•6 years ago
|
||
Comment on attachment 8969836 [details] [diff] [review] ArcUnion prototype. v1 Review of attachment 8969836 [details] [diff] [review]: ----------------------------------------------------------------- this looks pretty good. I'd love to eventually publish ServoArc and/or get the Rust stdlib to expose enough so that it can be implemented compatibly with sync::Arc ::: servo/components/servo_arc/lib.rs @@ +976,5 @@ > + /// Similar to deref, but uses the lifetime |a| rather than the lifetime of > + /// self, which is incompatible with the signature of the Deref trait. > + #[inline] > + pub fn get(&self) -> &'a T { > + &*self.0 nit: this can just be self.0, immutable references are Copy
Attachment #8969836 -
Flags: feedback?(manishearth) → feedback+
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970741 -
Flags: review?(manishearth)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: AT4sud9goGV
Attachment #8970742 -
Flags: review?(manishearth)
Assignee | ||
Comment 5•6 years ago
|
||
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970743 -
Flags: review?(manishearth)
Assignee | ||
Updated•6 years ago
|
Attachment #8970741 -
Attachment is obsolete: true
Attachment #8970741 -
Flags: review?(manishearth)
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ea482e400c7c9aa9f1da32f6cb271984486c2bc
Comment 7•6 years ago
|
||
Comment on attachment 8970743 [details] [diff] [review] Part 1 - ArcUnion. v2 Review of attachment 8970743 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/servo_arc/lib.rs @@ +1021,5 @@ > + First(ArcBorrow<'a, A>), > + Second(ArcBorrow<'a, B>), > +} > + > +impl<A: 'static, B: 'static> ArcUnion<A, B> { might wish to mark some of these methods `#[inline]`
Attachment #8970743 -
Flags: review?(manishearth) → review+
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #7) > might wish to mark some of these methods `#[inline]` I thought that was unnecessary given that they're generic?
Comment 9•6 years ago
|
||
Oh, that's correct. Sorry.
Updated•6 years ago
|
Attachment #8970742 -
Flags: review?(manishearth) → review+
Comment 10•6 years ago
|
||
Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5a517a7c8cf2 ArcUnion. r=Manishearth https://hg.mozilla.org/integration/autoland/rev/af621dc793aa Update StyleSource to use ArcUnion. r=Manishearth
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a517a7c8cf2 https://hg.mozilla.org/mozilla-central/rev/af621dc793aa
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•