stylo: Support getting line / column number of CSS rules

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: xidorn, Assigned: ferjm)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 5 obsolete attachments)

59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
SimonSapin
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
(Reporter)

Description

a year ago
Devtools uses inDOMUtils::GetRule{Line,Column} to get line / column number of a given CSS rule, and this information is stored in Rule::m{Line,Column}Number.

Currently, all CSS rules from stylo assigns zero for these fields, which means devtools cannot get the correct numbers.

To fix this, we would need Servo side to store the corresponding information during parsing, and feed that to Gecko.
Priority: -- → P2
(Assignee)

Updated

a year ago
Assignee: nobody → ferjmoreno
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
These patches make the first two tests from layout/inspector/tests/test_parseStyleSheet.html pass. The tests break at https://dxr.mozilla.org/mozilla-central/source/layout/inspector/tests/test_parseStyleSheet.html#27 though, but for reasons that seems unrelated to this bug:

5 INFO TEST-UNEXPECTED-FAIL | layout/inspector/tests/test_parseStyleSheet.html | uncaught exception - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [inIDOMUtils.parseStyleSheet] at doApply@chrome://specialpowers/content/specialpowersAPI.js:146:10

I'll investigate this failure in a follow up.
(Reporter)

Comment 13

a year ago
mozreview-review
Comment on attachment 8861565 [details]
Bug 1350175 - Part 1: Set stylesheet line offset.

https://reviewboard.mozilla.org/r/133524/#review136572
Attachment #8861565 - Flags: review?(xidorn+moz) → review+
(Reporter)

Comment 14

a year ago
mozreview-review
Comment on attachment 8861434 [details]
Bug 1350175 - Part 2: FFI changes to get line and column.

https://reviewboard.mozilla.org/r/133420/#review136574

This seems to add lots of unnecessary FFI calls. Could you just change all constructors of `Servo*Rule` to take line and column as parameters, and change `Servo_CssRules_Get*RuleAt` to take two pointers for outputing those numbers? That should unify more code and avoid two additional FFI calls for each rule when queried via CSSOM.
Attachment #8861434 - Flags: review?(xidorn+moz)
(Reporter)

Comment 15

a year ago
mozreview-review
Comment on attachment 8861435 [details]
Bug 1350175 - Part 3: Set source position for the different rules.

https://reviewboard.mozilla.org/r/133422/#review136578
Attachment #8861435 - Flags: review?(xidorn+moz)
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8861436 [details]
Bug 1350175 - Part 4: Set line and column values for CSS rules from Gecko glue.

https://reviewboard.mozilla.org/r/133424/#review136580
Attachment #8861436 - Flags: review?(xidorn+moz)
(Reporter)

Comment 17

a year ago
mozreview-review
Comment on attachment 8861437 [details]
Bug 1350175 - stylo: Support getting line / column number of CSS rules. Part 4: NamespaceRule.

https://reviewboard.mozilla.org/r/133426/#review136582
Attachment #8861437 - Flags: review?(xidorn+moz)
(Reporter)

Comment 18

a year ago
mozreview-review
Comment on attachment 8861438 [details]
Bug 1350175 - stylo: Support getting line / column number of CSS rules. Part 5: MediaRule.

https://reviewboard.mozilla.org/r/133428/#review136584
Attachment #8861438 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 27

a year ago
mozreview-review
Comment on attachment 8861565 [details]
Bug 1350175 - Part 1: Set stylesheet line offset.

https://reviewboard.mozilla.org/r/133524/#review136734

::: commit-message-a30dc:1
(Diff revision 2)
> +Bug 1350175 - stylo: Support getting line / column number of CSS rules. Part 0: Set stylesheet line offset. r=xidorn

Please don't prepend the whole bug summary in the commit message. `Bug XXXX part X: ...` is enough, and preferable. Bug summary is redundant here. Also it is recommended to use `r?xxx` for requesting review, rather than `r=xxx`.
(Reporter)

Comment 28

a year ago
mozreview-review
Comment on attachment 8861434 [details]
Bug 1350175 - Part 2: FFI changes to get line and column.

https://reviewboard.mozilla.org/r/133420/#review136738

::: layout/style/ServoMediaRule.h:23
(Diff revision 3)
>  
>  class ServoMediaRule final : public dom::CSSMediaRule
>  {
>  public:
> -  explicit ServoMediaRule(RefPtr<RawServoMediaRule> aRawRule);
> +  ServoMediaRule(RefPtr<RawServoMediaRule> aRawRule,
> +                 uint32_t, uint32_t);

`uint32_t aLine, uint32_t aColumn`.

::: layout/style/ServoMediaRule.cpp:19
(Diff revision 3)
>  using namespace mozilla::dom;
>  
>  namespace mozilla {
>  
> -ServoMediaRule::ServoMediaRule(RefPtr<RawServoMediaRule> aRawRule)
> +ServoMediaRule::ServoMediaRule(RefPtr<RawServoMediaRule> aRawRule,
> +                               uint32_t, uint32_t)

ditto.

::: servo/ports/geckolib/glue.rs:759
(Diff revision 3)
> +                        unsafe {
> +                            *line = 0; // TODO rule.source_location.line as u32
> +                            *column = 0; // TODO rule.source_location.column as u32
> +                        }

Probably better
```rust
*unsafe { line.as_mut().unwrap() } = xxx;
```
is better for limiting the scope of unsafe, also for explicit panic if pointer is null.
Attachment #8861434 - Flags: review?(xidorn+moz) → review+
(Reporter)

Comment 29

a year ago
mozreview-review
Comment on attachment 8861434 [details]
Bug 1350175 - Part 2: FFI changes to get line and column.

https://reviewboard.mozilla.org/r/133420/#review136744

::: layout/style/ServoCSSRuleList.cpp:89
(Diff revision 3)
> -        ruleObj = new Servo##name_##Rule(                                   \
> -          Servo_CssRules_Get##name_##RuleAt(mRawRules, aIndex).Consume());  \
> +        uint32_t line = 0, column = 0;                                      \
> +        RefPtr<RawServo##name_##Rule> rule =                                \
> +          Servo_CssRules_Get##name_##RuleAt(                                \
> +              mRawRules, aIndex, &line, &column                             \
> +          ).Consume();                                                      \
> +        ruleObj = new Servo##name_##Rule(rule, line, column);               \

You are doing an unnecessary refcount change. You should do either `rule.forget()` or `Move(rule)` here.

If you use `rule.forget()`, you would not need to change `already_AddRefed<>` to `RefPtr<>` in many of the constructors.
(Reporter)

Comment 30

a year ago
mozreview-review
Comment on attachment 8861435 [details]
Bug 1350175 - Part 3: Set source position for the different rules.

https://reviewboard.mozilla.org/r/133422/#review136748

::: servo/components/style/stylesheets.rs:1212
(Diff revision 3)
> +                line: location.line + self.context.line_number_offset as usize - 1,
> +                column: location.column,

I wonder whether this is correct. If I have
```html
<style>html{}</style>
```
what is the location of the style rule?

I'm not sure whether it works correctly in Gecko, and probably Servo doesn't handle this case correctly. We may address this in a separate bug.
Attachment #8861435 - Flags: review?(xidorn+moz) → review+
(Reporter)

Comment 31

a year ago
mozreview-review
Comment on attachment 8861436 [details]
Bug 1350175 - Part 4: Set line and column values for CSS rules from Gecko glue.

https://reviewboard.mozilla.org/r/133424/#review136750

::: servo/components/style/stylesheets.rs:1190
(Diff revision 3)
> -                )))))
> +                    source_location: SourceLocation {
> +                        line: location.line + self.context.line_number_offset as usize - 1,
> +                        column: location.column,
> +                    },

Can we have some function to unify all this code?
Attachment #8861436 - Flags: review?(xidorn+moz) → review+
(Reporter)

Updated

a year ago
Attachment #8861435 - Flags: review?(simon.sapin)
Attachment #8861436 - Flags: review?(simon.sapin)
Attachment #8861437 - Flags: review?(simon.sapin)
Attachment #8861438 - Flags: review?(simon.sapin)
Attachment #8861836 - Flags: review?(xidorn+moz) → review?(simon.sapin)
(Reporter)

Comment 32

a year ago
I'm not sure whether the usage of current_source_location is correct (whether we are really getting the location of the start of the rule), so redirect to Simon.
(Reporter)

Updated

a year ago
Attachment #8861435 - Flags: review+
(Reporter)

Updated

a year ago
Attachment #8861436 - Flags: review+
(Reporter)

Updated

a year ago
Attachment #8861437 - Flags: review?(xidorn+moz)
(Reporter)

Updated

a year ago
Attachment #8861438 - Flags: review?(xidorn+moz)
(Reporter)

Comment 33

a year ago
I don't think it is necessary to split different rules into different commits... but I'm fine with it.

An additional note that for Servo-only changes, we usually don't give them a part number because we strip the bug number and part number for Servo pull request. That means if you allocate part number for them, the part number landed in m-c would not be continuous.

This is just some personal preference, though.
(Reporter)

Comment 34

a year ago
mozreview-review
Comment on attachment 8861837 [details]
Bug 1350175 - stylo: Support getting line / column number of CSS rules. Part 7: Set line and column values.

https://reviewboard.mozilla.org/r/133842/#review136800

::: servo/ports/geckolib/glue.rs:761
(Diff revision 1)
>              read_locked_arc(rules, |rules: &CssRules| {
>                  match rules.0[index as usize] {
>                      CssRule::$name(ref rule) => {
> +                        {
> +                            let global_style_data = &*GLOBAL_STYLE_DATA;
> +                            let guard = global_style_data.shared_lock.read();

Hmmm, you are locking again... Although it allows nested read lock, but it is probably better to avoid it as much as possible.

Please remove the use of `read_locked_arc` and get the guard explicitly, then use the same guard in the two places.
Attachment #8861837 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8861437 - Attachment is obsolete: true
Attachment #8861437 - Flags: review?(simon.sapin)
(Assignee)

Updated

a year ago
Attachment #8861438 - Attachment is obsolete: true
Attachment #8861438 - Flags: review?(simon.sapin)
(Assignee)

Updated

a year ago
Attachment #8861836 - Attachment is obsolete: true
Attachment #8861836 - Flags: review?(simon.sapin)
(Assignee)

Updated

a year ago
Attachment #8861837 - Attachment is obsolete: true
(Assignee)

Comment 39

a year ago
Thank you for the feedback, Xidorn. Very useful. I updated the patches trying to address all of it.

I am still trying to learn how mozreview works. My intention was to carry over your r+ for the first two patches and request review for the last two patches.
(Assignee)

Comment 40

a year ago
Comment on attachment 8861434 [details]
Bug 1350175 - Part 2: FFI changes to get line and column.

r=xidorn from Comment 28
Attachment #8861434 - Flags: review+
(Reporter)

Comment 41

a year ago
I think it is supposed to carry over the r+ automatically... there might be a bug...
(Assignee)

Comment 42

a year ago
Comment on attachment 8861565 [details]
Bug 1350175 - Part 1: Set stylesheet line offset.

r=xidorn from Comment 30
Attachment #8861565 - Flags: review+
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #41)
> I think it is supposed to carry over the r+ automatically... there might be
> a bug...

I think it may only carry over r+ if the submitter has L3 commit access.
(Assignee)

Updated

a year ago
Attachment #8861436 - Flags: review?(xidorn+moz)
(Assignee)

Comment 44

a year ago
I have L3 commit access. I guess what happened here is that I changed the commit messages and removed some of the patches from the previous review request.
(Reporter)

Updated

a year ago
Attachment #8861436 - Flags: review?(xidorn+moz) → review+
(Reporter)

Comment 45

a year ago
Note that I'm landing @supports rule object in bug 1355394. After that you may need to also change ServoSupportsRule's ctor, but that should be a trivial change.

Comment 46

a year ago
mozreview-review
Comment on attachment 8861435 [details]
Bug 1350175 - Part 3: Set source position for the different rules.

https://reviewboard.mozilla.org/r/133422/#review137924

::: servo/components/style/font_face.rs:243
(Diff revision 4)
>         impl<'a, 'b> DeclarationParser for FontFaceRuleParser<'a, 'b> {
>              type Declaration = ();
>  
>              fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<(), ()> {
> +                let location = input.current_source_location();
> +                self.rule.source_location = get_location_with_offset(location,

This doesn’t look right. This is setting `rule.source_location` to the location of a declaration’s value, not of the rule. It is also overwriting that location again for every declaration in the rule.

The place closest to the correct one to call current_source_location() for an at-rule is at the start of the AtRuleParser::parse_prelude method implementation in stylel/stylesheets.rs. (It gives the location just *after* the at-keyword. We might prefer the location just before, but that would require extending the cssparser::AtRuleParser trait.)

::: servo/components/style/stylesheets.rs:1173
(Diff revision 4)
>                  let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::FontFace));
>                  Ok(CssRule::FontFace(Arc::new(self.shared_lock.wrap(
>                     parse_font_face_block(&context, input).into()))))
>              }
>              AtRulePrelude::Media(media_queries) => {
> +                let location = input.current_source_location();

Here and for other at-rules, the start of parse_prelude() is probably a better source location. Using it probably requires adding a SourceLocation field in the AtRulePrelude type.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

a year ago
mozreview-review
Comment on attachment 8861435 [details]
Bug 1350175 - Part 3: Set source position for the different rules.

https://reviewboard.mozilla.org/r/133422/#review141682
Attachment #8861435 - Flags: review?(simon.sapin) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8861436 - Attachment is obsolete: true

Comment 55

a year ago
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b900cd113877
Part 1: Set stylesheet line offset. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/a9d0e6db7048
Part 2: FFI changes to get line and column. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/667b85abce96
Part 3: Set source position for the different rules. r=SimonSapin,xidorn

Comment 56

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b900cd113877
https://hg.mozilla.org/mozilla-central/rev/a9d0e6db7048
https://hg.mozilla.org/mozilla-central/rev/667b85abce96
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.