Closed
Bug 1455784
Opened 7 years ago
Closed 7 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•7 years ago
|
||
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8969836 -
Flags: feedback?(manishearth)
Comment 2•7 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•7 years ago
|
||
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970741 -
Flags: review?(manishearth)
Assignee | ||
Comment 4•7 years ago
|
||
MozReview-Commit-ID: AT4sud9goGV
Attachment #8970742 -
Flags: review?(manishearth)
Assignee | ||
Comment 5•7 years ago
|
||
MozReview-Commit-ID: Jxp2A7cj6CV
Attachment #8970743 -
Flags: review?(manishearth)
Assignee | ||
Updated•7 years ago
|
Attachment #8970741 -
Attachment is obsolete: true
Attachment #8970741 -
Flags: review?(manishearth)
Assignee | ||
Comment 6•7 years ago
|
||
Comment 7•7 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•7 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•7 years ago
|
||
Oh, that's correct. Sorry.
Updated•7 years ago
|
Attachment #8970742 -
Flags: review?(manishearth) → review+
Comment 10•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a517a7c8cf2
https://hg.mozilla.org/mozilla-central/rev/af621dc793aa
Status: NEW → RESOLVED
Closed: 7 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
•