inIDOMUtils should expose the column number of rule definitions

RESOLVED FIXED in mozilla23

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: David Creswick, Assigned: David Creswick)

Tracking

(Blocks: 1 bug)

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 731476 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Blocks: 733752
(Assignee)

Updated

5 years ago
Attachment #731476 - Flags: review?(dbaron)
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
Attachment #731476 - Flags: review?(dbaron) → review+
(Though you should probably ask me for review again for the test.)
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 732428 [details] [diff] [review]
patch v2, with test and changes from review
Attachment #731476 - Attachment is obsolete: true
Attachment #732428 - Flags: review?(dbaron)
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).
Attachment #732428 - Flags: review?(dbaron) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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").
(Assignee)

Comment 8

5 years ago
Created attachment 734273 [details] [diff] [review]
patch v3
Attachment #732428 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/082be152752b
Assignee: nobody → dcrewi
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/082be152752b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.