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.

Really need this for @media rules for a Style Editor feature.

This is also needed for @keyframe rules for the keyframe inspector https://bugzilla.mozilla.org/show_bug.cgi?id=1030889

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.

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.

(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.

Fixed up the first patch.

Comment on attachment 8453303 [details] [diff] [review] 591303-part-1.patch Please don't remove the blank line before the "class Rule" line.

Fixed.

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.

(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!

Uploading the right patch this time.

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.

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.

Comment on attachment 8454822 [details] [diff] [review] Part 1: Move mLineNumber, mColumnNumber, and mWasMatched from StyleRule to Rule r=me

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?

(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

This will get line/column numbers for all the rules in nsCSSParser.

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.

Comment on attachment 8454966 [details] [diff] [review] Part 3: Set line and column number for all rules in nsCSSParser r=me. Thank you!

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

Comment on attachment 8455033 [details] [diff] [review] Part 3: Set line and column number for all rules in nsCSSParser r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/89311542468d https://hg.mozilla.org/integration/mozilla-inbound/rev/785257a95e36 https://hg.mozilla.org/integration/mozilla-inbound/rev/8acc3d0aa710

https://hg.mozilla.org/mozilla-central/rev/89311542468d https://hg.mozilla.org/mozilla-central/rev/785257a95e36 https://hg.mozilla.org/mozilla-central/rev/8acc3d0aa710