856317 - inIDOMUtils should expose the column number of rule definitions

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Creswick]

Attachment: patch v1

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 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[David Creswick]

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

[David Creswick]

Attachment: patch v2, with test and changes from review

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also gave you canconfirm and editbugs privileges in Bugzilla.  See:
https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, when you revise the patch, you can use the checkin-needed keyword.  See:
https://developer.mozilla.org/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").

Comment 8

[David Creswick]

Attachment: patch v3

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/082be152752b

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/082be152752b