Closed Bug 1350175 Opened 8 years ago Closed 8 years ago

stylo: Support getting line / column number of CSS rules

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: ferjm)

References

Details

Attachments

(3 files, 5 obsolete files)

Bug 1350175 - Part 2: FFI changes to get line and column.
59 bytes, text/x-review-board-request
Details
Bug 1350175 - Part 3: Set source position for the different rules.
59 bytes, text/x-review-board-request
SimonSapin
: review+
Details
Bug 1350175 - Part 1: Set stylesheet line offset.
59 bytes, text/x-review-board-request
Details
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: nobody → ferjmoreno
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.
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+
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)
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)
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)
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)
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 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`.
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+
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.
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+
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+
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)
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.
Attachment #8861435 - Flags: review+
Attachment #8861436 - Flags: review+
Attachment #8861437 - Flags: review?(xidorn+moz)
Attachment #8861438 - Flags: review?(xidorn+moz)
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.
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)
Attachment #8861437 - Attachment is obsolete: true
Attachment #8861437 - Flags: review?(simon.sapin)
Attachment #8861438 - Attachment is obsolete: true
Attachment #8861438 - Flags: review?(simon.sapin)
Attachment #8861836 - Attachment is obsolete: true
Attachment #8861836 - Flags: review?(simon.sapin)
Attachment #8861837 - Attachment is obsolete: true
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.
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+
I think it is supposed to carry over the r+ automatically... there might be a bug...
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.
Attachment #8861436 - Flags: review?(xidorn+moz)
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.
Attachment #8861436 - Flags: review?(xidorn+moz) → review+
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 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 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+
Attachment #8861436 - Attachment is obsolete: true
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: