Closed Bug 591303 Opened 9 years ago Closed 6 years ago

Allow passing a nsIDOMCSSRule into domUtils.getRule[Line|Column]

Categories

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

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jwalker, Assigned: gl)

References

Details

Attachments

(3 files, 10 obsolete files)

24.42 KB, patch
poiru
: review+
Details | Diff | Splinter Review
7.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
31.83 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
When the domRule passed to domUtils.getRuleLine has domRule.type === CSSRule.IMPORT_RULE, and exception is thrown.

This was noticed as part of the production of the inspector style panel. We have a work-around that does not affect the user, so this is extremely trivial, however it's probably worth noting.
getRuleLine takes an nsIDOMCSSStyleRule.  You're passing something that's not an nsIDOMCSSStyleRule.  It throws as a result, like all IDL methods do if you pass the wrong type.  What's the bug?
And specifically, how is this different from passing 5 or "my string" in?
How should code determine what line of a CSS file an @import came from, then?

The code in question (bug 582596) is basically iterating over document.styleSheets[x].cssRules, and wanting to match up each entry with a line number in the source file.

Could getRuleLine be changed to take a nsIDOMCSSRule? [Presumably this same issue exists for @charset, @fontface, etc; that should kill all with one stone.]
> How should code determine what line of a CSS file an @import came from, then?

It can't right now.  We simply don't store that information.

> Could getRuleLine be changed to take a nsIDOMCSSRule?

We could, but it wouldn't be able to return anything useful without more changes to core style system data structures.  It could return 0 or something, of course.

Maybe we should make said changes.

> Presumably this same issue exists for @charset, @fontface, etc

Yep.
(In reply to comment #4)
> > How should code determine what line of a CSS file an @import came from, then?
> 
> It can't right now.  We simply don't store that information.

Sadfaces.

> Maybe we should make said changes.

Shall we morph this bug?
That might be the simplest thing to do.
No longer blocks: 586984
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Really need this for @media rules for a Style Editor feature.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: domUtils.getRuleLine fails for @import rules → Allow passing a nsIDOMCSSRule into domUtils.getRule[Line|Column]
This is also needed for @keyframe rules for the keyframe inspector https://bugzilla.mozilla.org/show_bug.cgi?id=1030889
Blocks: 1033082
Blocks: 1030889
Assignee: nobody → gabriel.luong
So generally speaking, I think the right way to fix this is:

1)  Move the mLineNumber and  mColumnNumber members (and perhaps the mWasMatched member, to avoid bloating StyleRule by a word) from StyleRule to Rule.

2)  Move SetLineNumberAndColumnNumber, GetLineNumber, GetColumnNumber to Rule.  Possibly replace SetLineNumberAndColumnNumber with constructor arguments to make it easier to track down callers that need to set the line/column.

3)  Update nsCSSParser to pass in the line/column for other rule types.  

4)  Update the inDOMUtils code to work with other Rule subclasses as well.
Attached patch 591303-part-1.patch (obsolete) — Splinter Review
Attachment #8452464 - Flags: review?(bzbarsky)
Comment on attachment 8452464 [details] [diff] [review]
591303-part-1.patch

There's a bunch of changes to StyleRule.cpp and StyleRule.h that don't seem to be changing anything.  Do you know why those are there?  They don't _seem_ to be newline changes, which was my first assumption....

Other than that, looks good.
Attachment #8452464 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #11)
> Comment on attachment 8452464 [details] [diff] [review]
> 591303-part-1.patch
> 
> There's a bunch of changes to StyleRule.cpp and StyleRule.h that don't seem
> to be changing anything.  Do you know why those are there?  They don't
> _seem_ to be newline changes, which was my first assumption....
> 
> Other than that, looks good.

That happened because my editor was trimming the trailing whitespaces. If we don't want that, I can remove those changes.
> That happened because my editor was trimming the trailing whitespaces. 

There aren't any trailing whitespaces that I can see on those lines.
Attached patch 591303-part-1.patch (obsolete) — Splinter Review
Fixed up the first patch.
Attachment #8452464 - Attachment is obsolete: true
Comment on attachment 8453303 [details] [diff] [review]
591303-part-1.patch

Please don't remove the blank line before the "class Rule" line.
Attached patch 591303-part-1.patch (obsolete) — Splinter Review
Fixed.
Attachment #8453303 - Attachment is obsolete: true
Comment on attachment 8454186 [details] [diff] [review]
Part 2: Add getCSSRule function to nsIDOMCSSRule

>+  [noscript, notxpcom] Rule getCSSRule();

You want nostdcall too, and then you don't need the NS_IMETHOD_/NS_IMETHODIMP_ noise.

r=me with that.
Attachment #8454186 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #18)
> Comment on attachment 8454186 [details] [diff] [review]
> Part 2: Add getCSSRule function to nsIDOMCSSRule
> 
> >+  [noscript, notxpcom] Rule getCSSRule();
> 
> You want nostdcall too, and then you don't need the
> NS_IMETHOD_/NS_IMETHODIMP_ noise.

Done, thanks!
Attachment #8454186 - Attachment is obsolete: true
Attachment #8454438 - Flags: review+
Uploading the right patch this time.
Attachment #8454438 - Attachment is obsolete: true
Attachment #8454482 - Flags: review+
Comment on attachment 8454524 [details] [diff] [review]
Part 3: Set line and column number for {Import,Media,Keyframes,Keyframe}Rule in nsCSSParser

I think it would really be much cleaner to move the line/column bits into the rule constructors.  That would also make sure we catch all the relevant callsites.  Page rules, for example.

Past that, I would prefer we pass the line/column to ProcessImport instead of moving the rule creation out of it.
Attachment #8454524 - Flags: review?(bzbarsky) → feedback+
Attachment #8453595 - Attachment is obsolete: true
Attachment #8454822 - Flags: review?(bzbarsky)
The line/column number is initialized to 0 for Rule and GroupRule to avoid changing all the subclasses. This can be saved for a follow up.
Attachment #8454524 - Attachment is obsolete: true
Attachment #8454825 - Flags: review?(bzbarsky)
Comment on attachment 8454822 [details] [diff] [review]
Part 1: Move mLineNumber, mColumnNumber, and mWasMatched from StyleRule to Rule

r=me
Attachment #8454822 - Flags: review?(bzbarsky) → review+
Comment on attachment 8454825 [details] [diff] [review]
Part 3: Set line and column number for {Import,Media,Keyframes,Keyframe}Rule in nsCSSParser

>+  GroupRule(uint32_t aLineNumber = 0, uint32_t aColumnNumber = 0);

Would be better to require them, so we can't accidentally forget to pass them.

>+++ b/layout/style/StyleRule.h
>   StyleRule(nsCSSSelectorList* aSelector,
>-            Declaration *aDeclaration);
>+            Declaration *aDeclaration,
>+            uint32_t aLineNumber = 0, uint32_t aColumnNumber = 0);

This should be in part 1, no?  Please make sure part 1 compiles on its own.

And again, these should be required.

r=me on the rest, though this is still leaving some rule types without line/column numbers, right?
Attachment #8454825 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #26)
> >+++ b/layout/style/StyleRule.h
> >   StyleRule(nsCSSSelectorList* aSelector,
> >-            Declaration *aDeclaration);
> >+            Declaration *aDeclaration,
> >+            uint32_t aLineNumber = 0, uint32_t aColumnNumber = 0);
> 
> This should be in part 1, no?  Please make sure part 1 compiles on its own.
> 

Fixed
Attachment #8454822 - Attachment is obsolete: true
Attachment #8454963 - Flags: review?(bzbarsky)
Attached patch 591303-part-3.patch (obsolete) — Splinter Review
This will get line/column numbers for all the rules in nsCSSParser.
Attachment #8454825 - Attachment is obsolete: true
Attachment #8454964 - Attachment is obsolete: true
Attachment #8454966 - Flags: review?(bzbarsky)
Comment on attachment 8454963 [details] [diff] [review]
Part 1: Move mLineNumber, mColumnNumber, and mWasMatched from StyleRule to Rule

r=me assuming part 3 makes the arguments to StyleRule non-optional.
Attachment #8454963 - Flags: review?(bzbarsky) → review+
Comment on attachment 8454966 [details] [diff] [review]
Part 3: Set line and column number for all rules in nsCSSParser

r=me.  Thank you!
Attachment #8454966 - Flags: review?(bzbarsky) → review+
Moved GetNextTokenLocation for CSSParserImpl::ParseSupportsRule after the scanner starts recording. This resolves test failure in test_supports_rules.html which reported the following error: got color: green), expected (color: green).
This was seen in: https://tbpl.mozilla.org/?tree=Try&rev=5f6754151021

New try: https://tbpl.mozilla.org/?tree=Try&rev=541b65de7181
Attachment #8454966 - Attachment is obsolete: true
Attachment #8455033 - Flags: review?(bzbarsky)
Comment on attachment 8455033 [details] [diff] [review]
Part 3: Set line and column number for all rules in nsCSSParser

r=me
Attachment #8455033 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/89311542468d
https://hg.mozilla.org/mozilla-central/rev/785257a95e36
https://hg.mozilla.org/mozilla-central/rev/8acc3d0aa710
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.