Closed Bug 1394994 Opened 7 years ago Closed 7 years ago

Stylo: Keyframe rules missing line / column info

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jryans, Assigned: chenpighead)

References

Details

Attachments

(3 files, 1 obsolete file)

Servo's keyframe rules appear to always return line 0 column 0, which breaks DevTools tests, such as:

devtools/client/inspector/rules/test/browser_rules_keyframes-rule_01.js
devtools/client/inspector/rules/test/browser_rules_keyframes-rule_02.js
devtools/client/inspector/rules/test/browser_rules_keyframeLineNumbers.js

(They are currently skipped, so unskip them to test!)

I believe we are fetching[1] the numbers, but ServoKeyframeRule ignores[2] it, so it looks like an easy fix.

[1]: http://searchfox.org/mozilla-central/source/layout/style/ServoCSSRuleList.cpp#101
[2]: http://searchfox.org/mozilla-central/source/layout/style/ServoKeyframeRule.h#22
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> FWIW, ServoKeyframeRule is not created in ServoCSSRuleList, but in
> ServoKeyframesRule:
> http://searchfox.org/mozilla-central/rev/
> 51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/style/ServoKeyframesRule.
> cpp#56

Oh, right!  I confused `Keyframes` and `Keyframe`...
I think:

devtools/client/inspector/rules/test/browser_rules_search-filter_02.js

is also affected by this.
So, it may not be that trivial to fix, because we don't currently store line and column for keyframe rules, see http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/servo/components/style/stylesheets/keyframes_rule.rs#186-195

But it may not be that hard either.
At a quick glance, I think store an extra SourceLocation [1] member for keyframe rules should fix this. I'll investigate this.


[1] https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/servo/components/style/stylesheets/style_rule.rs#25
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Blocks: 1395479
If this is a dupe of bug 1395749, I want to point out the user-facing symptom of this isn't just missing line/column info for an animation, (and some failing tests) it entirely prevents use of the Rules panel (display and interaction with any matching rules and properties) for an element which has an animation applied to it.
(In reply to Sam Foster [:sfoster] from comment #9)
> If this is a dupe of bug 1395749, I want to point out the user-facing
> symptom of this isn't just missing line/column info for an animation, (and
> some failing tests) it entirely prevents use of the Rules panel (display and
> interaction with any matching rules and properties) for an element which has
> an animation applied to it.

Yes, that sounds like this issue based on reports from other duplicates like bug 1395484.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #5)
> At a quick glance, I think store an extra SourceLocation [1] member for
> keyframe rules should fix this. I'll investigate this.
> 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/servo/components/style/stylesheets/
> style_rule.rs#25

More detail about the plan:

1. store an extra SourceLocation member for keyframe structure [1] in Servo

2. implement Servo_KeyframesRule_GetKeyframeAt (or fix the existing Servo_KeyframesRule_GetKeyframe directly?) for keyframe, to get line and column info from Servo

3. fix ServoKeyframeRule constructor [1] to accept line and column parameters and construct CSSKeyframeRule accordingly



[1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/servo/components/style/stylesheets/keyframes_rule.rs#186-195
[2] https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/style/ServoKeyframeRule.h#21-23
> 3. fix ServoKeyframeRule constructor [1] to accept line and column

This reference should be [2].
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #11)
> 2. implement Servo_KeyframesRule_GetKeyframeAt (or fix the existing
> Servo_KeyframesRule_GetKeyframe directly?) for keyframe, to get line and
> column info from Servo

Seems like you could rename `Servo_KeyframesRule_GetKeyframe` to `Servo_KeyframesRule_GetKeyframeAt`, since it already takes an index arg (which is what seems to trigger the `At` suffix in the current convention) and I believe that's the same usage you would modify to also take line and column args, so there's no need for 2 separate FFI methods.
With the current WIP, I can see the Keyframes from the inspector now. However, there seems something wrong shown on the devtool panel. No idea why the declarations are always invalid.... However, the animation does works (I use the testcase in Bug 1395749 [1] to do the verification). So, I think the direction should be correct, probably something implementation detail that I missed.... will keep digging tomorrow.


[1] https://bug1395749.bmoattachments.org/attachment.cgi?id=8903371
Oops, I should use keyframe selector's location, not the KeyframesRule's location. After fixing this, the results of keyframe rules are presented correctly on devtool pannel. The tests mentioned in comment 0 and comment 3 are passed on my local. I'll clean up the patchset and ask for review very soon.
Comment on attachment 8904053 [details]
Bug 1394994 - stylo: store location information for keyframe rules.

https://reviewboard.mozilla.org/r/175796/#review180888

::: servo/components/style/stylesheets/keyframes_rule.rs:195
(Diff revision 2)
> +    /// The line and column of the rule's source code.
> +    pub source_location: SourceLocation,

Probably add a blank line between the two members given there is one above...

::: servo/components/style/stylesheets/keyframes_rule.rs:490
(Diff revision 2)
>      type AtRule = Arc<Locked<Keyframe>>;
>      type Error = SelectorParseError<'i, StyleParseError<'i>>;
>  }
>  
> +/// A wrapper to wraps the KeyframeSelector with its source location
> +pub struct KeyframeSelectorParserPrelude {

This struct probably doesn't need to be `pub`.

::: servo/components/style/stylesheets/keyframes_rule.rs:588
(Diff revision 2)
> +/// Adjust a location's column to accommodate DevTools.
> +fn get_location_with_offset(location: SourceLocation) -> SourceLocation {
> +    SourceLocation {
> +        line: location.line,
> +        // Column offsets are not yet supported, but Gecko devtools expect 1-based columns.
> +        column: location.column + 1,
> +    }
> +}

Please try to move or expose the function in rule_parser somehow, rather than duplicating it here.

::: servo/ports/geckolib/glue.rs:1514
(Diff revision 2)
>      read_locked_arc(rule, |rule: &KeyframesRule| {
> -        rule.keyframes[index as usize].clone().into_strong()
> +        let global_style_data = &*GLOBAL_STYLE_DATA;
> +        let guard = global_style_data.shared_lock.read();
> +        let key = rule.keyframes[index as usize].clone();
> +        let location = key.read_with(&guard).source_location;
> +        *unsafe { line.as_mut().unwrap() } = location.line as u32;
> +        *unsafe { column.as_mut().unwrap() } = location.column as u32;
> +        key.into_strong()
>      })

Probably you should expand `read_locked_arc` here. That function was only meant to be used when one don't need `guard` other than reading the rule itself.

If you need to generate the guard anyway, it is better just use the same guard for both read, rather than obtaining another.

See the `Servo_KeyframesRule_FindRule` function below for example.
Attachment #8904053 - Flags: review?(xidorn+moz)
Comment on attachment 8904128 [details]
Bug 1394994 - stylo: get line and column for ServoKeyframeRule.

https://reviewboard.mozilla.org/r/175906/#review180896

::: layout/style/ServoKeyframeRule.h:21
(Diff revision 1)
>  class ServoKeyframeDeclaration;
>  
>  class ServoKeyframeRule final : public dom::CSSKeyframeRule
>  {
>  public:
> -  explicit ServoKeyframeRule(already_AddRefed<RawServoKeyframe> aRaw)
> +  explicit ServoKeyframeRule(already_AddRefed<RawServoKeyframe> aRaw,

You can remove the `explicit` now given it has more than one parameters.

::: layout/style/ServoKeyframesRule.cpp:58
(Diff revision 1)
> -      mRules.ReplaceObjectAt(rule, aIndex);
> -      rule->SetStyleSheet(mStyleSheet);
> +        Servo_KeyframesRule_GetKeyframeAt(mRawRule, aIndex, &line, &column
> +        ).Consume();

If you have to break this line... probably breaking it before `&line` is better.
Attachment #8904128 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8904129 [details]
Bug 1394994 - stylo: update annotations for Devtools tests.

https://reviewboard.mozilla.org/r/175908/#review180898
Attachment #8904129 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8904053 [details]
Bug 1394994 - stylo: store location information for keyframe rules.

https://reviewboard.mozilla.org/r/175796/#review180888

> This struct probably doesn't need to be `pub`.

You're right. Will fix this.

> Please try to move or expose the function in rule_parser somehow, rather than duplicating it here.

Yeah, it seems the duplication should be avoidable. I'm not sure where this function should belong to. Moving it into cssparser doesn't seem right to me, since this is a Gecko devtools specific helper function.

What do you think if I remove the duplication of get_location_with_offset and just inline the offset process here (see below) for now?

```
let location = SourceLocation {
    line: start_location.line,
    // Column offsets are not yet supported, but Gecko devtools expect 1-based columns.
    column: start_location.column + 1,
}
```

> Probably you should expand `read_locked_arc` here. That function was only meant to be used when one don't need `guard` other than reading the rule itself.
> 
> If you need to generate the guard anyway, it is better just use the same guard for both read, rather than obtaining another.
> 
> See the `Servo_KeyframesRule_FindRule` function below for example.

Ok, since `read_locked_arc` has been used in many places, I'll go with not using `read_locked_arc` here. Instead, I'll refer to `Servo_KeyframesRule_FindRule` and use the same guard for both read.
Comment on attachment 8904053 [details]
Bug 1394994 - stylo: store location information for keyframe rules.

https://reviewboard.mozilla.org/r/175796/#review180888

> Yeah, it seems the duplication should be avoidable. I'm not sure where this function should belong to. Moving it into cssparser doesn't seem right to me, since this is a Gecko devtools specific helper function.
> 
> What do you think if I remove the duplication of get_location_with_offset and just inline the offset process here (see below) for now?
> 
> ```
> let location = SourceLocation {
>     line: start_location.line,
>     // Column offsets are not yet supported, but Gecko devtools expect 1-based columns.
>     column: start_location.column + 1,
> }
> ```

Oops, please ignore my proposal above. It seems pretty easy to just expose the function and reuse it, so I'll fix the duplication in this way.
Comment on attachment 8904053 [details]
Bug 1394994 - stylo: store location information for keyframe rules.

https://reviewboard.mozilla.org/r/175796/#review180942
Attachment #8904053 - Flags: review?(xidorn+moz) → review+
Attachment #8904053 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6774e07b2fc9
stylo: get line and column for ServoKeyframeRule. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/c549d5f5fe6d
stylo: update annotations for Devtools tests. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/6774e07b2fc9
https://hg.mozilla.org/mozilla-central/rev/c549d5f5fe6d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.