Closed Bug 1370719 Opened 7 years ago Closed 7 years ago

stylo: Move the Rule node pointer into ComputedValues

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bholley, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 4 obsolete files)

Bug 1370719 - Copy ComputedStyle to context.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1370719 - Shrink ElementData by moving pseudo count to type.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
This will save us two words on ElementData, on top of what we save in bug 1370711.

It's possible that there might be some reason this is more complicated than we think, and if so we should re-evaluate. But so far it seems like a reasonable thing to do.
jryans, can you look into this?
Flags: needinfo?(jryans)
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> jryans, can you look into this?

Sure, I'll investigate!
Assignee: nobody → jryans
Flags: needinfo?(jryans)
Priority: -- → P1
Note that you can probably avoid a lot of refactoring by temporarily storing the rule node in the CurrentElementInfo between the point when it's computed and the point where we have a ComputedValue to stash in the ElementData.
In ComputedStyle today, we have 2 StrongRuleNodes (1 wrapped in Option) and 2 Option<Arc<ComputedValues>>, which add up to 32 bytes in ElementData.  In this bug, we're talking about pushing the rule nodes inside each ComputedValues.  For extra savings, the visited values in ComputedStyle is a temporary value to ease data flow, so it could be removed.

The potential benefit of this work is shrinking ComputedStyle by 24 bytes down to 8 bytes (just a single Arc<ComputedValues>).  Each ComputedValues would grow by 8 bytes (each element may have 1 or 2 of those depending on visited).  Possibly the smaller ElementData would improve perf here by fitting better in a cache line, but I don't think we have clear evidence that this matters yet.

After attempting this for a bit, I am getting worried that it makes the data flow a lot harder to reason about.

There are various paths through matching / cascading, such as: (a) full match and cascade, (b) replace rules, re-cascade, (c) re-cascade without rule changes.  At the moment, they can all look to ComputedStyle for rules and values.  This change means that rules are stored permanently inside ComputedValues after the cascade, but sometimes referenced temporarily off CurrentElementInfo before the cascade.  The different paths through matching / cascading feed through common functions that can no longer assume where the "newest" rules are located since it's unclear what work has already happened at each step.

So, we could certainly sprinkle various "ensure you've got the latest rules" calls around, but it definitely feels more complex to me while I am working on it, and surely when trying to make future changes as well.  Should I continue to push forward here?  My feeling is we have higher priority issues for now, but we could return to this in the future.
Flags: needinfo?(bobbyholley)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> In ComputedStyle today, we have 2 StrongRuleNodes (1 wrapped in Option) and
> 2 Option<Arc<ComputedValues>>, which add up to 32 bytes in ElementData.  In
> this bug, we're talking about pushing the rule nodes inside each
> ComputedValues.  For extra savings, the visited values in ComputedStyle is a
> temporary value to ease data flow, so it could be removed.
> 
> The potential benefit of this work is shrinking ComputedStyle by 24 bytes
> down to 8 bytes (just a single Arc<ComputedValues>).  Each ComputedValues
> would grow by 8 bytes (each element may have 1 or 2 of those depending on
> visited).  Possibly the smaller ElementData would improve perf here by
> fitting better in a cache line, but I don't think we have clear evidence
> that this matters yet.
> 
> After attempting this for a bit, I am getting worried that it makes the data
> flow a lot harder to reason about.
> 
> There are various paths through matching / cascading, such as: (a) full
> match and cascade, (b) replace rules, re-cascade, (c) re-cascade without
> rule changes.  At the moment, they can all look to ComputedStyle for rules
> and values.  This change means that rules are stored permanently inside
> ComputedValues after the cascade, but sometimes referenced temporarily off
> CurrentElementInfo before the cascade.  The different paths through matching
> / cascading feed through common functions that can no longer assume where
> the "newest" rules are located since it's unclear what work has already
> happened at each step.
> 
> So, we could certainly sprinkle various "ensure you've got the latest rules"
> calls around, but it definitely feels more complex to me while I am working
> on it, and surely when trying to make future changes as well.  Should I
> continue to push forward here?  My feeling is we have higher priority issues
> for now, but we could return to this in the future.

So, I certainly agree that we shouldn't make the code significantly more complex or fragile on account of this. But I don't totally follow why this would actually happen.

In my mental model, we'd just expose a method on StyleContext called current_rule_node, which checks whether we have one cached in the CurrentElementInfo, and if not, forwards to the ComputedStyles (which would presumably need to be passed in as an arg). Is there some reason that wouldn't be easy?
Flags: needinfo?(bobbyholley) → needinfo?(jryans)
Chatted a bit more on IRC, I'll keep going with this.

I think I was losing confidence that there was actually a benefit overall.
Flags: needinfo?(jryans)
:bholley noted that this will also help with bug 1367635, which would need a rule node in the ComputedValues.
Blocks: 1367635
Blocks: 1373056
Blocks: 1368291
Comment on attachment 8878683 [details]
Bug 1370719 - Copy ComputedStyles to context.

https://reviewboard.mozilla.org/r/149986/#review154726
Attachment #8878683 - Flags: review?(bobbyholley) → review+
Comment on attachment 8878684 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/149988/#review154732

I've only skimmed the patch, but a few comments:
* ElementValues is too Option-happy. We have an Option on the outer struct and and Option on both of the inner ones. This means that the NonZero optimization has nowhere to stick the discriminant, so it needs to add an extra word. We should remove one of those Options (presumably the outer one), at which point ElementData should shrink. Really, ElementData should be 24 bytes, because there are 3 words (the primary computed values, the lazily-allocated pseudo array, and the RestyleData). What are we missing?
* The way you've reused the PseudoStyles stuff for the CurrentElementInfo causes us to do a transient heap-allocation, which isn't good. We should just store it inline.
* I don't think it really makes sense to use the old data structure names, specifically ComputedStyles, to store things that just have the rule nodes, because the actual computed values haven't been generated yet. Perhaps StylePrerequisites?
* In general I'd rather the temporary stuff moved to context.rs, and keep data.rs for the persistent pieces.
* We probably don't need size_of tests for the ephemeral structs, since they're stack-allocated and temporary.
Attachment #8878684 - Flags: review?(bobbyholley) → review-
It's probably easier to address the above and then have me look at it again. Thanks for working on this!
Comment on attachment 8878683 [details]
Bug 1370719 - Copy ComputedStyles to context.

Canceling review on this for now since it's not clear to me that we need this given my comments on part 2.
Attachment #8878683 - Flags: review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12)
> Comment on attachment 8878684 [details]
> Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.
> 
> https://reviewboard.mozilla.org/r/149988/#review154732
> 
> I've only skimmed the patch, but a few comments:
> * ElementValues is too Option-happy. We have an Option on the outer struct
> and and Option on both of the inner ones. This means that the NonZero
> optimization has nowhere to stick the discriminant, so it needs to add an
> extra word. We should remove one of those Options (presumably the outer
> one), at which point ElementData should shrink. Really, ElementData should
> be 24 bytes, because there are 3 words (the primary computed values, the
> lazily-allocated pseudo array, and the RestyleData). What are we missing?

Good points!  I removed the outer option, since that seems liked the simpler one to remove, so that gets us down to 32 bytes from ElementData.

The other issue was that the type holding the eager pseudo values was declared as a slice, so it was being stored as (ptr, len) and taking up 16 bytes.  Since the length is constant, I changed it to an array instead, and now we're at the expected 24 bytes for ElementData.

> * The way you've reused the PseudoStyles stuff for the CurrentElementInfo
> causes us to do a transient heap-allocation, which isn't good. We should
> just store it inline.

Good catch, I've changed this to use an array inline now.

> * I don't think it really makes sense to use the old data structure names,
> specifically ComputedStyles, to store things that just have the rule nodes,
> because the actual computed values haven't been generated yet. Perhaps
> StylePrerequisites?

I went with `CascadeInputs`, which seemed pretty clear and also short.

> * In general I'd rather the temporary stuff moved to context.rs, and keep
> data.rs for the persistent pieces.

Makes sense, I've moved it.  I'll attach a "prequel" patch that just copies `ComputedStyle` there first, to make it more obvious what is new code.

> * We probably don't need size_of tests for the ephemeral structs, since
> they're stack-allocated and temporary.

Okay, removed.
Attachment #8879743 - Attachment is obsolete: true
Attachment #8879743 - Flags: review?(bobbyholley)
Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

https://reviewboard.mozilla.org/r/151490/#review156410
Attachment #8880110 - Flags: review?(bobbyholley) → review+
Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

https://reviewboard.mozilla.org/r/151490/#review156412

::: commit-message-85070:1
(Diff revision 1)
> +Bug 1370719 - Copy ComputedStyles to context. r=bholley
> +
> +In the patch after this, the existing `ComputedStyles` type is renamed and
> +repurposed as temporary storage of inputs for the cascade.  To make it a bit
> +easier to follow what code is new, this patch starts by copying `ComputedStyles`
> +to context.rs without changes.

Oh, nit: The struct name is ComputedStyle, not ComputedStyles. Please fix up the title and commit message.
Comment on attachment 8880110 [details]
Bug 1370719 - Copy ComputedStyle to context.

https://reviewboard.mozilla.org/r/151490/#review156412

> Oh, nit: The struct name is ComputedStyle, not ComputedStyles. Please fix up the title and commit message.

Ah, good catch!
Comment on attachment 8880112 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

https://reviewboard.mozilla.org/r/151494/#review156414

Nice find. :-)

::: commit-message-ce61e:3
(Diff revision 1)
> +Bug 1370719 - Shrink ElementData by moving pseudo count to type. r=bholley
> +
> +`ElementValues` holds an optional list of values for each eager pseudo-element.

Nit - this should be ElementStyles given the rename you're doing now.
Attachment #8880112 - Flags: review?(bobbyholley) → review+
Comment on attachment 8880188 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

https://reviewboard.mozilla.org/r/151556/#review156506

I addressed the nit about values vs. styles.  Not sure why this went back to a review request...
So, few things, from poking at the patch a bit for the lifetime issue:

The lifetime issue seems unavoidable as things stand. But the root cause is that still we have that apply_declarations call from Servo's animation code, which instead of creating a rule node, it goes and applies them directly. Seems like we should really fix that and use a rule node there instead. Then all the iterator madness is moot and can go away, and the cascade() function can just take rule nodes.

That being said, there are quite a few things that look fishy in this patch. In particular:

 pub fn for_inheritance(parent: &'a ComputedValues, default: &'a ComputedValues) -> Self {
      Self::new(parent.rules.clone(), ...)

That bit is plain wrong. Something that inherits from a style doesn't have any rule applied to it. It should use the rule tree root instead.

Third, has anybody measured this is an actual memory win?

Not that we shouldn't do it (seems beneficial for code health in general), but just that the memory win (assuming that's the real reason for this patch) seems less obvious when you take into account that this patch is growing text styles...

Anyway, I think we should really fix the apply_declarations fix instead of hacking around it though.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> So, few things, from poking at the patch a bit for the lifetime issue:
> 
> The lifetime issue seems unavoidable as things stand. But the root cause is
> that still we have that apply_declarations call from Servo's animation code,
> which instead of creating a rule node, it goes and applies them directly.
> Seems like we should really fix that and use a rule node there instead. Then
> all the iterator madness is moot and can go away, and the cascade() function
> can just take rule nodes.
> 
> That being said, there are quite a few things that look fishy in this patch.
> In particular:
> 
>  pub fn for_inheritance(parent: &'a ComputedValues, default: &'a
> ComputedValues) -> Self {
>       Self::new(parent.rules.clone(), ...)
> 
> That bit is plain wrong. Something that inherits from a style doesn't have
> any rule applied to it. It should use the rule tree root instead.

BTW, The most elegant way to fix this at a glance seems making `rules` non optional (just holding the rule tree root instead), and using default.rules.clone() instead, btw. Why would it be optional anyway?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #34)
> So, few things, from poking at the patch a bit for the lifetime issue:
> 
> The lifetime issue seems unavoidable as things stand. But the root cause is
> that still we have that apply_declarations call from Servo's animation code,
> which instead of creating a rule node, it goes and applies them directly.
> Seems like we should really fix that and use a rule node there instead. Then
> all the iterator madness is moot and can go away, and the cascade() function
> can just take rule nodes.
> 
> That being said, there are quite a few things that look fishy in this patch.
> In particular:
> 
>  pub fn for_inheritance(parent: &'a ComputedValues, default: &'a
> ComputedValues) -> Self {
>       Self::new(parent.rules.clone(), ...)
> 
> That bit is plain wrong. Something that inherits from a style doesn't have
> any rule applied to it. It should use the rule tree root instead.

I agree that we shouldn't use the parent rule node here, but don't agree with using the root node as a sentinel. Using the root node means highly-contended atomic refcount traffic, whereas using None is free. And since we have the nonzero optimization, they take the same amount of space.

> 
> Third, has anybody measured this is an actual memory win?
> 
> Not that we shouldn't do it (seems beneficial for code health in general),
> but just that the memory win (assuming that's the real reason for this
> patch) seems less obvious when you take into account that this patch is
> growing text styles...

The memory win is a partial motivation, but not the only one - we're going to need it for bug 1368291.

That said, I would be very surprised if the impact on ComputedValues size ended up outweighing here the benefit here. Our style sharing is getting quite good - on wikipedia, we're sharing styles for about 85% of the elements, and with bug 1368290 those wins will translate to text nodes and anonymous boxes.

Finally, with bug 1367635, the rulenode-in-computedvalues will save us even more memory by allowing us to cache the reset structs in the rule tree.
 
> Anyway, I think we should really fix the apply_declarations fix instead of
> hacking around it though.

I think at this point that should be a followup. jryans, can you file?
Comment on attachment 8880188 [details]
Bug 1370719 - Shrink ElementData by moving pseudo count to type.

https://reviewboard.mozilla.org/r/151556/#review156540
Attachment #8880188 - Flags: review?(bobbyholley) → review+
Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156562

::: servo/components/style/animation.rs:505
(Diff revision 4)
>  
>              // This currently ignores visited styles, which seems acceptable,
>              // as existing browsers don't appear to animate visited styles.
>              let computed =
>                  properties::apply_declarations(context.stylist.device(),
> +                                               previous_style.rules(),

Please file a followup for fixing this up?

::: servo/components/style/data.rs:107
(Diff revision 4)
> -    /// Returns whether there are any pseudo styles.
> +    /// Returns whether there are any pseudo values.
>      pub fn is_empty(&self) -> bool {
>          self.0.is_none()
>      }
>  
> -    /// Returns a reference to the style for a given eager pseudo, if it exists.
> -    pub fn get(&self, pseudo: &PseudoElement) -> Option<&ComputedStyle> {
> +    /// Returns a reference to the values for a given eager pseudo, if it exists.
> +    pub fn get(&self, pseudo: &PseudoElement) -> Option<&Arc<ComputedValues>> {
>          debug_assert!(pseudo.is_eager());
>          self.0.as_ref().and_then(|p| p[pseudo.eager_index()].as_ref())
>      }
>  
> -    /// Returns a mutable reference to the style for a given eager pseudo, if it exists.
> -    pub fn get_mut(&mut self, pseudo: &PseudoElement) -> Option<&mut ComputedStyle> {
> +    /// Returns a mutable reference to the values for a given eager pseudo, if it exists.
> +    pub fn get_mut(&mut self, pseudo: &PseudoElement) -> Option<&mut Arc<ComputedValues>> {
>          debug_assert!(pseudo.is_eager());
>          self.0.as_mut().and_then(|p| p[pseudo.eager_index()].as_mut())
>      }
>  
> -    /// Returns true if the EagerPseudoStyles has a ComputedStyle for |pseudo|.
> +    /// Returns true if the EagerPseudoStyles has the values for |pseudo|.
>      pub fn has(&self, pseudo: &PseudoElement) -> bool {
>          self.get(pseudo).is_some()
>      }
>  
> -    /// Inserts a pseudo-element. The pseudo-element must not already exist.
> -    pub fn insert(&mut self, pseudo: &PseudoElement, style: ComputedStyle) {
> +    /// Sets the values for the eager pseudo.
> +    pub fn set(&mut self, pseudo: &PseudoElement, value: Arc<ComputedValues>) {

Nit: all this "values" renaming really isn't necessary.

::: servo/components/style/matching.rs:1388
(Diff revision 4)
> +        let mut primary_inputs = context.thread_local.current_element_info
> +                                        .as_mut().unwrap()
> +                                        .cascade_inputs.primary_mut();

Seems like we do this incantation a lot. How about making a helper cascade_inputs() method on context?

::: servo/components/style/properties/gecko.mako.rs:165
(Diff revision 4)
> +    /// Gets a reference to the rule node. Panic if the element does not have a
> +    /// rule node.

What Element? This should probably say Panics if the ComputedValues does not have a rule node.

::: servo/components/style/properties/properties.mako.rs:1886
(Diff revision 4)
> +    /// Gets a reference to the rule node. Panic if the element does not have a
> +    /// rule node.

Similar.

::: servo/components/style/properties/properties.mako.rs:2351
(Diff revision 4)
>      }
>  
>      /// Inherits style from the parent element, accounting for the default
>      /// computed values that need to be provided as well.
>      pub fn for_inheritance(parent: &'a ComputedValues, default: &'a ComputedValues) -> Self {
> -        Self::new(parent.custom_properties(),
> +        Self::new(parent.rules.clone(),

As emilio mentioned, this should pass None.

::: servo/components/style/properties/properties.mako.rs:2637
(Diff revision 4)
>      let custom_properties =
>          ::custom_properties::finish_cascade(
>              custom_properties, &inherited_custom_properties);
>  
>      let builder = if !flags.contains(INHERIT_ALL) {
> -        StyleBuilder::new(custom_properties,
> +        StyleBuilder::new(Some(rules.clone()),

Let's make sure we have a bug on file to eliminate this clone().
Attachment #8880111 - Flags: review?(bobbyholley) → review+
Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156618

::: servo/components/style/matching.rs:301
(Diff revision 4)
>  
>      fn cascade_with_rules(&self,
>                            shared_context: &SharedStyleContext,
>                            font_metrics_provider: &FontMetricsProvider,
>                            rule_node: &StrongRuleNode,
> -                          primary_style: &ComputedStyle,
> +                          primary_style: Option<&Arc<ComputedValues>>,

Please document when this can or cannot be `None`.

::: servo/components/style/properties/properties.mako.rs:2637
(Diff revision 4)
>      let custom_properties =
>          ::custom_properties::finish_cascade(
>              custom_properties, &inherited_custom_properties);
>  
>      let builder = if !flags.contains(INHERIT_ALL) {
> -        StyleBuilder::new(custom_properties,
> +        StyleBuilder::new(Some(rules.clone()),

So, if we're not keeping the root as the only way of saying "no rules matched", the has_rules function is kind of a lie, because you can arrive here without any matching rule, and has_rules will happily return true.

Should we check the rule node too?

::: servo/ports/geckolib/glue.rs:2605
(Diff revision 4)
>      // In the common case we already have the style. Check that before setting
>      // up all the computation machinery. (Don't use it when we're getting
>      // default styles, though.)
>      if rule_inclusion == RuleInclusion::All {
> -        if let Some(result) = element.mutate_data()
> -                                     .and_then(|d| d.get_styles().map(&finish)) {
> +        let styles = element.mutate_data().and_then(|d| {
> +            match d.has_styles() {

nit: I usually use `if` instead of matching with `Some` and `None`.
Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156618

> nit: I usually use `if` instead of matching with `Some` and `None`.

Err, matching with `true` and `false`.
Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156562

> Please file a followup for fixing this up?

I filed bug 1375525.  I'll mention it in a comment as well.

> Nit: all this "values" renaming really isn't necessary.

Good catch, I've changed these back to "style".

> Seems like we do this incantation a lot. How about making a helper cascade_inputs() method on context?

Okay, I added helpers and was able to use them in most places.  In some spots, I had to keep the long form because the helpers borrow all of context and some code paths need to simultaneously use other bits of context with different borrowing.

> What Element? This should probably say Panics if the ComputedValues does not have a rule node.

Okay, updated.

> As emilio mentioned, this should pass None.

Okay, fixed.

> Let's make sure we have a bug on file to eliminate this clone().

I mentioned this in same bug filed for the animation issue, and I'll add a comment here.
Comment on attachment 8880111 [details]
Bug 1370719 - Move match and cascade temporaries to CurrentElementInfo.

https://reviewboard.mozilla.org/r/151492/#review156618

> Please document when this can or cannot be `None`.

Good idea, added docs for this.

> Err, matching with `true` and `false`.

Yes, that's probably more natural!  Fixed.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #39)
> ::: servo/components/style/properties/properties.mako.rs:2637
> (Diff revision 4)
> >      let custom_properties =
> >          ::custom_properties::finish_cascade(
> >              custom_properties, &inherited_custom_properties);
> >  
> >      let builder = if !flags.contains(INHERIT_ALL) {
> > -        StyleBuilder::new(custom_properties,
> > +        StyleBuilder::new(Some(rules.clone()),
> 
> So, if we're not keeping the root as the only way of saying "no rules
> matched", the has_rules function is kind of a lie, because you can arrive
> here without any matching rule, and has_rules will happily return true.
> 
> Should we check the rule node too?

I don't quite follow your concern here, can you elaborate?  Or let's discuss on IRC if that's simpler.
Flags: needinfo?(emilio+bugs)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #43)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #39)
> > ::: servo/components/style/properties/properties.mako.rs:2637
> > (Diff revision 4)
> > >      let custom_properties =
> > >          ::custom_properties::finish_cascade(
> > >              custom_properties, &inherited_custom_properties);
> > >  
> > >      let builder = if !flags.contains(INHERIT_ALL) {
> > > -        StyleBuilder::new(custom_properties,
> > > +        StyleBuilder::new(Some(rules.clone()),
> > 
> > So, if we're not keeping the root as the only way of saying "no rules
> > matched", the has_rules function is kind of a lie, because you can arrive
> > here without any matching rule, and has_rules will happily return true.
> > 
> > Should we check the rule node too?
> 
> I don't quite follow your concern here, can you elaborate?  Or let's discuss
> on IRC if that's simpler.

We dicsussed this on IRC.  The issue was about whether `has_rules` is still correct for the visited case.  It is because we only have Some(rules) that type of matching was done now or in the past.

I added some comments to clarify this.
Flags: needinfo?(emilio+bugs)
https://hg.mozilla.org/integration/autoland/rev/f620402d752e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: