Implement CSS Properties & Values API in Servo. #18167

jyc · 2017-08-21T08:02:19Z

Here are some patches to implement the CSS Properties & Values API in Servo! I have a simple patch to implement it in Firefox via Stylo afterwards. I also have patches to implement animations for these in Stylo, but not in Servo.

One-line Gecko change to ServoBindings.toml required as a consequence of replacing custom_properties::CustomPropertiesMap with properties_and_values::CustomPropertiesMap in the ComputedValues.

Animations patches involve much more changes to Stylo than Servo, so I think those should be reviewed separately & through the Stylo process.

Some disabled tests:

We don't implement <transform-list> serialization.
We don't serialize colors according to spec & as Gecko does.
We don't round floats correctly (see servo/rust-cssparser#173).
We don't implement <resolution>.

It's not clear whether or not we actually need initialValue (see w3c/css-houdini-drafts#286).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #17155 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-08-21T08:02:24Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties_and_values.rs, components/style/properties/properties.mako.rs, components/style/lib.rs, components/style/stylist.rs, components/style/properties/gecko.mako.rs and 6 more
@canaltinova: components/style/properties_and_values.rs, components/style/properties/properties.mako.rs, components/style/lib.rs, components/style/stylist.rs, components/style/properties/gecko.mako.rs and 6 more
@KiChjang: components/script/dom/document.rs, components/script/dom/bindings/trace.rs, components/script/dom/webidls/CSS.webidl, components/script/dom/css.rs, components/script/dom/webidls/PropertyDescriptorDict.webidl
@fitzgen: components/script/dom/document.rs, components/script/dom/bindings/trace.rs, components/script/dom/webidls/CSS.webidl, components/script/dom/css.rs, components/script/dom/webidls/PropertyDescriptorDict.webidl
@emilio: components/style/properties_and_values.rs, components/style/properties/properties.mako.rs, components/style/lib.rs, components/style/stylist.rs, components/style/properties/gecko.mako.rs and 6 more

highfive · 2017-08-21T08:02:26Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio

Just a super-quick skim... I think I'd prefer no RwLock.

I've also noticed a bunch of other nits, but those are probably not a huge deal, and I'll comment with them later.

emilio · 2017-08-21T08:39:06Z

components/style/stylist.rs

+    /// The set of registered custom property associated with the document.
+    /// Should be initialized as soon as possible.
+    #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]
+    registered_property_set: Option<Arc<RwLock<RegisteredPropertySet>>>,


Why does this need to live in Stylist? It's only used to hand it out, right? If so, I suspect it should live in SharedStyleContext instead.

The the generation, if any, should be either in PerDocumentStyleDataImpl in Gecko, or LayoutThread / Document in Servo.

Or either just make it live on the stylist (since I see you require it from cascade), but forward commands to update it to layout from script. Would that work?

Also, that would prevent the RwLock.

Hmm... I guess it won't work if we allow querying the custom properties from script, which is kinda annoying... :(

Yep, can't make it live on the Stylist because the Stylist is not around for all the times someone might query from JS, unfortunately.

Can we at least use a SharedRwLock instead? That'd avoid the atomic operations in cascade.

Can try, but IIRC I ran into some issues. Might have been before I found the spec was changed to not require changing used declarations, though.

Which SharedRwLock are you thinking of? Do you mean instead you'd just prefer if cascade did not do any locking at all wrt the RegisteredPropertySet? That would certainly make sense & is a good catch, thanks.

FWIW what we do for stylesheets and similar is keeping two sets, one on layout and one on script (see the Document::stylesheets field). That makes the handling of them easier from style, so we may want to consider that, given Gecko wouldn't have this annoying problem of having to move the set across threads.

In any case, I haven't gone through it in detail, but I'd be very confused if we'd need to mutate this kind of global state each time we style an element, so I suspect we can at least get rid of the RwLock.

Also, I see no call to set_registered_custom_properties in Stylo, right?

As noted, the Stylo CSS.registerProperty/unregisterProperty is in a separate patch. That goes through the same codepath (properties_and_values::register_property) as Servo's CSS.registerProperty/unregisterProperty.

Going to try using the SharedRwLock!

emilio · 2017-08-21T08:39:06Z

components/style/values/computed/length.rs

+        // 3. Otherwise, serialize as a calc() containing the summation, with
+        //    the units ordered ASCII case-insensitive alphabetically, the
+        //    number (if present) coming before all units, and the percentage
+        //    (if present) coming after all units.


Please undo this commit, we serialize percentage before intentionally per w3c/csswg-drafts#1731.

Hm, a comment would be nice. :P Will revert.

Yep, I added it into the specified calc serialization, but not to the computed one, sorry! :)

bors-servo · 2017-08-21T17:03:13Z

☔ The latest upstream changes (presumably #18143) made this pull request unmergeable. Please resolve the merge conflicts.


        style: Refine custom_properties::parse_name; disallow extra tokens.

Part 1 of a series of patches to implement the CSS Properties & Values API.


        resources: Add a pref for CSS Properties & Values API.

The pref is layout.css.properties-and-values.enabled, and it currently defaults to false. Future patches in this series will add code that uses this pref. Part 2 of a series of patches to implement the CSS Properties & Values API.


        script: Add WebIDL and stubs for CSS.(un)registerProperty.

Part 3 of a series of patches to implement the CSS Properties & Values API.


        style: Expose transform::SpecifiedValue's internals.

A later patch in this series will use this when to inspect transform values for typed custom properties. Part 4 of a series of patches to implement the CSS Properties & Values API.


        style: Add/refactor types of custom_properties for Properties & Values.

Add a new ExtraData enum. We carry it along with a custom property's specified value in order to compute it, if it ends up being registered as being able to contain URLs through Properties & Values. When the specified value comes from a declaration, we keep track of the associated UrlExtraData in the Specified variant. However, specified values will also (implemented by a later patch in this series) be able to come from animations: in that case we are able to carry along a copy of the computed value in the Precomputed variant, to save us from recomputation and also to save us from having to re-resolve the URL. Add a new BorrowedExtraData enum, which is used for BorrowedSpecifiedValues. We use BorrowedSpecifiedValues when resolving custom properties; in the case that we inherit an untyped custom property, we create one from its computed value, which is a token stream. In this case we don't have a UrlExtraData or (Properties & Values) ComputedValue to compute with, and in fact we don't want to perform any computation. The BorrowedExtraData enum thus has an additional InheritedUntyped variant for that case. Pull out the css, first_token_type, and last_token_type fields of SpecifiedValue, BorrowedSpecifiedValue, and ComputedValue into a TokenStream struct (ComptuedValue then becomes a type alias of TokenStream). We'll use the TokenStream to store computed token stream values for typed custom properties: it wouldn't make sense to use SpecifiedValue because we have no need of the additional information (and that would also lead to properties_and_values::ComputedValue containing a SpecifiedValue which contains an ExtraData which may contain a properties_and_values::ComputedValue, which the compiler does not like). The properties_and_values module is addded by a later patch in this series, so we omit the Precomputed variant for now. Part 5 of a series of patches to implement the CSS Properties & Values API.


        style: Add a properties_and_values module for CSS Properties & Values.

Typed custom properties are exactly the same as regular custom properties except when it comes to inheritance, computed values, and animation, so this module is intended to be as independent from custom_properties as possible (they do use types from each other, though, where it would be clumsy to do otherwise). This commit doesn't hook up the module to anything: that's handled by later patches in this series. Part 6 of a series of patches to implement the CSS Properties & Values API.


        script: Store typed custom property registrations on the document.

CSS Properties & Values API says the set of registered properties should be associated with the document. Part 7 of a series of patches to implement the CSS Properties & Values API.


        layout_thread, style: Add guard for the registered property set.

Add a guard to StylesheetGuards for the RegisteredPropertySet. Part 8 of a series of patches to implement the CSS Properties & Values API.

jyc · 2017-08-23T05:40:26Z

Changes:

Lock the RegisteredPropertySet with a SharedRwLock.
Update tests & code to account for initialValue being required when syntax is neither omitted nor "*".
Remove fix to calc() serialization.
Rebase.
Fix commit message for the patch to update cascading; we removed StyleBuilder::custom_properties, not ComputedValues::custom_properties.


        layout_thread, style: Attach registered property set to the Stylist.

We store a pointer to the registered property set on the Stylist because that seems to be the cleanest way to give access to it to all the functions that might need to cascade or compute typed custom properties. This also lets us use the Stylist's existing mechanism for determining we need to rebuild styles: if it last saw the registered property set with an older generation than the current generation, we should do things again. This is handled by the same path in layout_thread::lib as the device being dirtied or the stylesheets changing. Also update the comment on Stylist::rebuild, which appears not to have been updated when stylesheets_changed was replaced with origins_to_rebuild. Part 9 of a series of patches to implement the CSS Properties & Values API.


        style: Fix comments in style::properties.

"An CSS-wide keyword" should be "A CSS-wide keyword." Also, the PropertyDeclaration::Custom variant must not contain a DeclaredValueOwned::WithVariables variant: this is in fact already asserted in style::custom_properties::cascade. Part 10 of a series of patches to implement the CSS Properties & Values API.


        style: Update cascade to take account of typed custom properties.

We have to do some additional work to compute typed custom properties and to handle cyclical declarations involving font-size. (Unfortunately there doesn't seem to be a really good way to split this patch up without spending a ton of effort, and I don't think it'd make the patch more clear. Sorry!) Make custom_properties::OrderedMap::remove public, so that we can remove cyclical custom property declarations from properties. We also need to insert the initial value (possibly typed) if necessary and detect if font size is cyclical and remove it in that case, so it makes sense to handle them in the same place instead of custom_properties mutating things behind the scenes and making it worry about typed custom properties. custom_properties::cascade takes two function arguments, inherited_computed and handle_keyword. This allows us to maintain separation of concerns wrt typed custom property registrations (also makes it clearer who is getting data from where). custom_properties::substitute_all and custom_properties::substitute_one gains function arguments compute handle_invalid with the same justification. custom_properties::finish_cascade is replaced by custom_properties::compute_ordering, which computes whether or not font-size's declaration is cycles, the custom properties to compute before early properties (including those upon which early properties depend), and the set of properties that are cyclical. Fix the comment on substitute, which does not type. Have Gecko's ComputedValuesInner::get_custom_properties return a properties_and_values::CustomPropertiesMap instead of a custom_properties::CustomPropertiesMap. This stores typed computed values as well. Replace Servo's Stylist::custom_properties with get_custom_properties, which returns a reference instead of an Arc, because it doesn't make sense to clone the Arc when we never need it as one. Also add get_parent_custom_properties, so a later patch in the series can inherit typed custom property values. Note that this also requires a change to ServoBindings.toml to account for replacing custom_properties::CustomPropertiesMap with properties_and_values::CustomPropertiesMap. Part 11 of a series of patches to implement the CSS Properties & Values API.


        script: Implement CSS.(un)registerProperty.

Part 12 of a series of patches to implement the CSS Properties & Values API.


        tests: Add tests for CSS Properties & Values API.

Some disabled tests: - We don't implement <transform-list> serialization. - We don't serialize colors according to spec & as Gecko does. - We don't round floats correctly (see rust-cssparser/#173). - We don't implement <resolution>. It's not clear whether or not we actually need initialValue (see css-houdini-drafts/#286). Part 13 of a series of patches to implement the CSS Properties & Values API.

jyc · 2017-08-23T21:59:52Z

Oops, looks like the changes for rebasing were left out. Pushing that.

bors-servo · 2017-08-24T19:26:19Z

☔ The latest upstream changes (presumably #18209) made this pull request unmergeable. Please resolve the merge conflicts.

jyc · 2017-08-24T19:30:08Z

r? @emilio

Hi! Manish suggested asking you to review--could you review this when you have the time? (if not, perhaps you have an idea of who else might review; perhaps the patches could be reviewed by separate people as well, like last time). I have some other patches that depend on it & those and this seem likely to continue to need rebasing.

emilio · 2017-08-24T19:34:54Z

Sure, will take a look, sorry for not doing it before :)

jyc · 2017-08-24T19:36:45Z

No problem, thanks!

emilio

This is just a first pass, since it's quite late.

I think overall it looks reasonable. I'm slightly concerned about the perf impact even when custom properties aren't used.

When they're used, it seems like there's a fair amount of HashSet stuff going around, and I'd probably need to review the details to ensure we're not creating a lot of HashSets unconditionally.

In general, I think this could've benefited from a bit less macro-patch, and trying to land stuff more incrementally.

For example, seems to me that all the syntax parsing code could've been an standalone PR. Maybe stubbing the methods could've been another one, and so on.

That'd make this way easier to review.

I think @SimonSapin and @heycam are familiar with the custom properties spec, so if they'd take a look I'd also appreciate it.

I think there are a few bits that look somewhat fishy, specially the generation stuff, which would force us to always do a stylist rebuild all the time.

It's possible that I've missed some stuff, but I have a fair amount of comments, I think :P

Also, thanks a lot for working on this! It's quite awesome. Let's ensure we get the details right :)

emilio · 2017-08-24T21:04:46Z

components/style/custom_properties.rs

-        Ok(&s[2..])
-    } else {
-        Err(())
+    let mut input = ParserInput::new(s);


Can you elaborate into why this patch is needed?

Part 1 of a series of patches to implement the CSS Properties & Values API.

Doesn't look like a great commit message. This looks fine, but this invokes a lot of tokenization machinery that we don't need in most cases.

Thanks! Will update the commit message. parse_name doesn't implement the spec that it cites:

A custom property is any property whose name starts with two dashes (U+002D HYPHEN-MINUS), like --foo. The production corresponds to this: it’s defined as any valid identifier that starts with two dashes.

s needs not to not just start with two dashes, but to also be an identifier.

This function is intended to be with the value of Token::Identifier(_), after CSS backslash-escaping has already been unescaped, or with a string coming from a CSSOM API where the property name is already a separate argument and does not need to using CSS syntax to delimit an identifier. Using a CSS parser again here is incorrect.

https://drafts.css-houdini.org/css-properties-values-api/#register-a-custom-property does suggest using a CSS parser but that’s wrong. I’ve filed w3c/css-houdini-drafts#465.

emilio · 2017-08-24T21:04:46Z

components/script/dom/webidls/PropertyDescriptorDict.webidl

+ */
+
+// Renamed from PropertyDescriptor to avoid conflicting with a JS class of the
+// same name.


Please check with @jdm that renaming this is fine. I think it is, but just want to make sure.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1273706#c325.

(Though actually, the reason might no longer apply. Going to try!)

No, unfortunately we still have that problem. :(

[jyc@jc-mbp-osx servo] (git pv/HEAD tests: Add tests …) → ./mach build Compiling script v0.0.1 (file:///Users/jyc/projects/servo/components/script) Compiling style v0.0.1 (file:///Users/jyc/projects/servo/components/style) Compiling gfx v0.0.1 (file:///Users/jyc/projects/servo/components/gfx) Compiling metrics v0.0.1 (file:///Users/jyc/projects/servo/components/metrics) Compiling layout_traits v0.0.1 (file:///Users/jyc/projects/servo/components/layout_traits) Compiling script_layout_interface v0.0.1 (file:///Users/jyc/projects/servo/components/script_layout_interface) Compiling constellation v0.0.1 (file:///Users/jyc/projects/servo/components/constellation) Compiling layout v0.0.1 (file:///Users/jyc/projects/servo/components/layout) error[E0252]: the name `PropertyDescriptor` is defined multiple times --> /Users/jyc/projects/servo/target/debug/build/script-9d08ccc9e36b2d2c/out/Bindings/CSSBinding.rs:443:5 | 251 | use dom::bindings::codegen::Bindings::PropertyDescriptorBinding::PropertyDescriptor; | ------------------------------------------------------------------------------- previous import of the type `PropertyDescriptor` here ... 443 | use js::jsapi::PropertyDescriptor; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `PropertyDescriptor` reimported here | = note: `PropertyDescriptor` must be defined only once in the type namespace of this module error[E0255]: the name `PropertyDescriptor` is defined multiple times --> /Users/jyc/projects/servo/target/debug/build/script-9d08ccc9e36b2d2c/out/Bindings/PropertyDescriptorBinding.rs:239:1 | 204 | use js::jsapi::PropertyDescriptor; | ----------------------------- previous import of the type `PropertyDescriptor` here ... 239 | / pub struct PropertyDescriptor { 240 | | pub inherits: bool, 241 | | pub initialValue: Option<DOMString>, 242 | | pub name: DOMString, 243 | | pub syntax: DOMString, 244 | | } | |_^ `PropertyDescriptor` redefined here | = note: `PropertyDescriptor` must be defined only once in the type namespace of this module error: aborting due to 2 previous errors error: Could not compile `script`. To learn more, run the command again with --verbose. Build FAILED in 0:01:39

emilio · 2017-08-24T21:04:46Z

components/style/custom_properties.rs

+pub enum ExtraData {
+    /// The specified value comes from a declaration (whence we get
+    /// the UrlExtraData).
+    Specified(UrlExtraData),


A link or quote from the section of the spec that requires this would be nice.

Also, why making it an enum with a single variant? (I guess for symmetry with BorrowedExtraData? If so, sounds fine.

Thanks! Will add link to w3c/css-houdini-drafts#393 (comment) . Other variants are added in subsequent patches (in the same PR).

emilio · 2017-08-24T21:04:46Z

components/style/custom_properties.rs

+impl Default for TokenStream {
+    fn default() -> Self {
+        TokenStream {
+            css: "".to_owned(),


nit: I'd just use String::new(),

Sounds good!

emilio · 2017-08-24T21:04:46Z

components/style/custom_properties.rs

+    pub last_token_type: TokenSerializationType,
+}
+
+impl Default for TokenStream {


Why is this needed? I don't see any use of this. I guess I may discover as I go on reviewing.

Thanks for catching this–this is used by the PR (not yet made, waiting for this one to land) implementing animations. Will move it over to the PR for that.

emilio · 2017-08-24T21:04:46Z

components/style/properties/properties.mako.rs

@@ -3085,9 +3090,338 @@ pub fn cascade(
        font_metrics_provider,
        flags,
        quirks_mode,
+        registered_property_set.read_with(guards.registered_property_set),


I'd require just a &RegisteredPropertySet, and move the read_with to the caller when possible here, but no big deal.

Did for consistency with other arguments to cascade.

emilio · 2017-08-24T21:04:46Z

components/style/properties/properties.mako.rs

+            // b) has an initial value,
+            // then we should set it to its initial value.
+            let mut computed = {
+                let has_value = specified.iter().map(|(name, _)| name.clone()).collect();


This doesn't seem particularly fast... But I guess

emilio · 2017-08-24T21:04:46Z

components/style/properties/properties.mako.rs

+            // There were no inherited or specified values.
+            // If there are any properties with initial values, we should set
+            // them.
+            (Some(Arc::new(registered_property_set.initial_values_except(context, None))), false)


So we're creating this and calling initial_values_except for every element, even if we don't use custom properties?

That seems quite suboptimal, not only for the unnecessary allocation specially since the time initial_values_except takes grows with the number of registered properties :/

Also, no branch seems to return None for computed_early, is it intentional? If not, something needs to get fixed. If it is, I don't see why it should return an Option.

So we're creating this and calling initial_values_except for every element, even if we don't use custom properties?
That seems quite suboptimal, not only for the unnecessary allocation specially since the time initial_values_except takes grows with the number of registered properties :/

Well, creating the Arc is expensive. But the cost of initial_values_except would not apply in the case you mentioned except on the root, as either we are inheriting precisely these values, or in fact there are no registered properties whatsoever.

I will fix it so that if there are no registered properties we return None as we would have in the past. In the future, and in line with w3c/css-houdini-drafts#286, it seems we should really be not storing computed initial values here at all (so that we can be completely analogous with regular properties); will have to make sure to do in a nice way so that we don't violate the abstraction barrier between substition and registrations.

emilio · 2017-08-24T21:04:46Z

components/style/properties/properties.mako.rs

@@ -3194,12 +3534,39 @@ where
            let mut font_size = None;
            let mut font_family = None;
        % endif
+        %if category_to_cascade_now == "other":


nit: Space after % .

emilio · 2017-08-24T21:04:46Z

components/script/dom/css.rs

+    match result {
+        PropertyRegistrationResult::Ok => {
+            // Should lead to the RestyleHint::restyle_subtree() being inserted
+            // eventually in handle_reflow due to checks in Stylist.


No need to do this. Afaict you could just mark the document's stylesheets list as dirty from here, and everything else should follow. You'd even avoid needing the registered_property_set_generation!

Nice! Would be cool if this kind of thing were documented.

Neat, this seems to be another consequence of the spec change.

emilio · 2017-08-24T21:07:28Z

One last bit. Resolution is only used for media query stuff, so it lives in gecko/media_queries.rs. A PR moving it to values would be nice.

Also, I'd really prefer everything to be in smaller modules, when possible. They're much nicer to read. Specially I'd love if it'd be avoidable to put more stuff in our mako files, which are already pretty bloated, and I'd like for them to eventually disappear.

jyc · 2017-08-27T03:57:20Z

Sorry for the delay–ran into some confusion moving in this year. Should be settled down now! Going through review comments.

Also, I'd really prefer everything to be in smaller modules, when possible. They're much nicer to read.

Hm, the properties_and_values module is approximately the same size as the custom_properties module. I've already separated them in order to keep the functionality as distinct as possible; it's not clear to me how making the implementation of, say, typed custom property animation in a separate module from the definition of what types typed custom properties can have includes readability. If you have concrete suggestions about what to split, I'm happy to try them, though.

jdm · 2017-11-15T22:38:37Z

Closing due to inactivity. Tracking this work in #17155.

highfive assigned wafflespeanut Aug 21, 2017

highfive added the S-awaiting-review label Aug 21, 2017

jyc changed the title ~~[WIP] Implement CSS Properties & Values API in Servo.~~ Implement CSS Properties & Values API in Servo. Aug 21, 2017

emilio reviewed Aug 21, 2017

View changes

jdm assigned emilio and unassigned wafflespeanut Aug 21, 2017

highfive added the S-needs-rebase label Aug 21, 2017

jyc added 8 commits Aug 20, 2017

style: Refine custom_properties::parse_name; disallow extra tokens.

b404f7a

Part 1 of a series of patches to implement the CSS Properties & Values API.

resources: Add a pref for CSS Properties & Values API.

a53656c

The pref is layout.css.properties-and-values.enabled, and it currently defaults to false. Future patches in this series will add code that uses this pref. Part 2 of a series of patches to implement the CSS Properties & Values API.

script: Add WebIDL and stubs for CSS.(un)registerProperty.

1f1e933

Part 3 of a series of patches to implement the CSS Properties & Values API.

style: Expose transform::SpecifiedValue's internals.

fcee76f

A later patch in this series will use this when to inspect transform values for typed custom properties. Part 4 of a series of patches to implement the CSS Properties & Values API.

script: Store typed custom property registrations on the document.

8273c33

CSS Properties & Values API says the set of registered properties should be associated with the document. Part 7 of a series of patches to implement the CSS Properties & Values API.

layout_thread, style: Add guard for the registered property set.

54b495c

Add a guard to StylesheetGuards for the RegisteredPropertySet. Part 8 of a series of patches to implement the CSS Properties & Values API.

jyc force-pushed the jyc:pv branch from 063cba7 to 71d7eea Aug 23, 2017

jyc added 5 commits Aug 20, 2017

script: Implement CSS.(un)registerProperty.

ee05a9a

Part 12 of a series of patches to implement the CSS Properties & Values API.

jyc force-pushed the jyc:pv branch from 71d7eea to c2dc915 Aug 23, 2017

highfive added the S-needs-rebase label Aug 24, 2017

emilio reviewed Aug 24, 2017

View changes

asajeffrey mentioned this pull request Oct 5, 2017

Review CSS Houdini #13557

Open

jdm closed this Nov 15, 2017

jdm mentioned this pull request Nov 15, 2017

Implement CSS Houdini "Properties & Values" #17155

Open

servo / servo

Join GitHub today

Implement CSS Properties & Values API in Servo. #18167

Implement CSS Properties & Values API in Servo. #18167

Conversation

jyc commented Aug 21, 2017 • edited by SimonSapin

highfive commented Aug 21, 2017

highfive commented Aug 21, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Aug 21, 2017

jyc commented Aug 23, 2017

jyc commented Aug 23, 2017

bors-servo commented Aug 24, 2017

jyc commented Aug 24, 2017 • edited

emilio commented Aug 24, 2017

jyc commented Aug 24, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jyc Aug 27, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio Aug 24, 2017 • edited

This comment has been minimized.

This comment has been minimized.

emilio commented Aug 24, 2017

jyc commented Aug 27, 2017

jdm commented Nov 15, 2017

jyc commented Aug 21, 2017 •

edited by SimonSapin

jyc commented Aug 24, 2017 •

edited

jyc Aug 27, 2017 •

edited

emilio Aug 24, 2017 •

edited