Closed Bug 1380890 Opened 7 years ago Closed 7 years ago

Stylo: GetRuleColumn needs to report a 1-based value, but instead reports 0-based

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jryans, Assigned: ferjm)

References

Details

Attachments

(1 file)

Bug 1380890 - stylo: make GetRuleColumn report a 1-based value.
59 bytes, text/x-review-board-request
jdm
: review+
Details 
DevTools expects inIDOMUtils::GetRuleColumn to return a 1-based value, but Stylo returns a 0-based value.  (The same is true for line numbers, but it appears Stylo lines are already 1-based.)

The leads to failures in tests like:

* devtools/client/inspector/computed/test/browser_computed_original-source-link.js

which loads CSS file containing:

---
div {
  color: #ff0066; }

span {
  background-color: #EEE; }

/*# sourceMappingURL=doc_sourcemaps.css.map */
---

The `div` rule is expected to have line 1 column 1, but Stylo gives line 1 column 0.
:ferjm, maybe you are interested in this one?
Flags: needinfo?(ferjmoreno)
Assignee: nobody → ferjmoreno
Flags: needinfo?(ferjmoreno)
This also appears to be the cause of ~8 failures in the tests devtools/client/inspector/boxmodel/test/browser_boxmodel*.
I keep finding more tests blocked on this one, so marking P1.
Priority: -- → P1
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> Example test log:
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=113641285&repo=try&lineNumber=4276

I am afraid that I cannot reproduce this failure locally, but looking at the try log it seems that this is related to source maps. 

The errors says: "Column must be greater than or equal to 0, got -1" and comes from https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/new/pretty-print-worker.js#5040 . Note that for lines it expects them to be greater or equal to 1.

FWIW all the tests that I found with references to inIDOMUtils::GetRuleColumn/GetRuleLine are passing locally for me:

- layout/inspector/tests/test_parseStyleSheet.html
- layout/inspector/tests/test_bug856317.html
- layout/inspector/tests/test_bug462789.html
- layout/inspector/tests/test_bug462787.html
- layout/inspector/tests/test_getRelativeRuleLine.html

I need to investigate more.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> > Example test log:
> > 
> > https://treeherder.mozilla.org/logviewer.
> > html#?job_id=113641285&repo=try&lineNumber=4276
> 
> I am afraid that I cannot reproduce this failure locally

Actually, this is not true. I just realized that I was running the tests on a non-stylo build :\ Building and testing with stylo support now.
While the first test mentioned does use source maps, the others in comment 3 do not, so I don't believe it's related to source maps.
With the attached patch all the tests previously mentioned are passing except:

- 2 tests from layout/inspector/tests/test_getRelativeRuleLine.html I filed bug 1382285 to fix them.
- 2 tests from devtools/client/inspector/boxmodel/test/browser_boxmodel_pseudo-element.js which seem unrelated. I filed bug 1382288 for them.
Comment on attachment 8887990 [details]
Bug 1380890 - stylo: make GetRuleColumn report a 1-based value.

https://reviewboard.mozilla.org/r/158864/#review164272

::: servo/ports/geckolib/glue.rs:1124
(Diff revision 1)
>              }
>  
>              match rules.0[index] {
>                  CssRule::$name(ref rule) => {
>                      let location = rule.read_with(&guard).source_location;
> +                    // Gecko devtools expects 1-based line and column values.

I am confused by this, because `SourceLocation` says[1] that its line and column indexes are 0-based, but clearly that's not what we're seeing.

I'll redirect to Simon, who should have more context on this.

[1]: http://searchfox.org/mozilla-central/source/third_party/rust/cssparser/src/tokenizer.rs#399
Attachment #8887990 - Flags: review?(jryans) → review?(simon.sapin)
https://github.com/servo/rust-cssparser/pull/168 changed cssparser to use 1-based line and column numbers but did not update the doc-comment.
Sorry, I was confused. That PR changed *from* 1-based to 0-based. (And updated docs correctly.)
Attachment #8887990 - Flags: review?(simon.sapin) → review?(josh)
Comment on attachment 8887990 [details]
Bug 1380890 - stylo: make GetRuleColumn report a 1-based value.

https://reviewboard.mozilla.org/r/158864/#review165176

::: servo/ports/geckolib/glue.rs:1127
(Diff revision 1)
>                  CssRule::$name(ref rule) => {
>                      let location = rule.read_with(&guard).source_location;
> +                    // Gecko devtools expects 1-based line and column values.
> +                    // source_location provides 1-based lines but 0-based columns.
>                      *unsafe { line.as_mut().unwrap() } = location.line as u32;
> -                    *unsafe { column.as_mut().unwrap() } = location.column as u32;
> +                    *unsafe { column.as_mut().unwrap() } = location.column as u32 + 1;

Instead of making this change here, let's add 1 to the column value in get_location_with_offset in rule_parser.rs. We can add the following comment there:
// Column offsets are not yet supported, but Gecko devtools expect 1-based columns.
Attachment #8887990 - Flags: review?(josh) → review-
Comment on attachment 8887990 [details]
Bug 1380890 - stylo: make GetRuleColumn report a 1-based value.

https://reviewboard.mozilla.org/r/158864/#review165516
Attachment #8887990 - Flags: review?(josh) → review+
Fixed by https://github.com/servo/servo/commit/5cabb0f86bd71d86c5716f8504b8c46bfa1a7da8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: