Last Comment Bug 98765 - CSS parser produces multiple rules for comma-separated selectors
: CSS parser produces multiple rules for comma-separated selectors
Status: RESOLVED FIXED
[patch]
: helpwanted
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.5alpha
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 183038 77705 188803 209433
  Show dependency treegraph
 
Reported: 2001-09-07 13:19 PDT by Boris Zbarsky [:bz]
Modified: 2014-04-26 03:11 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress (49.84 KB, patch)
2003-06-07 19:57 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
work in progress (49.77 KB, patch)
2003-06-07 20:06 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (55.56 KB, patch)
2003-06-07 21:59 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (60.91 KB, patch)
2003-06-07 22:24 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (62.80 KB, patch)
2003-06-13 20:59 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch (64.49 KB, patch)
2003-06-14 16:42 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2001-09-07 13:19:25 PDT
Given a rule like:

foo, bar { decls }

The CSS parser produces two nsICSSRules (which share the decls object).  These
both go in the stylesheet and are both visible through the CSSOM.  Doing
GetCssText() on them returns:

"foo { decls }" and "bar { decls } "

This should not affect round-trippability too much, but seems like behavior that
should be fixed at some point....
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-09-07 13:43:57 PDT
We need them to be treated as two separate rules for the cascade, so I'm not
sure how to fix this.
Comment 2 Boris Zbarsky [:bz] 2001-09-07 14:37:10 PDT
Right.  I'm just filing this so that it's filed and people are aware of the
issue.  Please feel free to future it (I would, at the moment).  I'm fairly
certain that there is no simple solution that does not involve changes to the
way we do cascading and the like....
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2001-09-09 08:39:11 PDT
->pierre.  Probably should be Future.
Comment 4 Pierre Saslawsky 2001-09-18 21:51:42 PDT
I'm becoming reluctant to keep bugs as Future is there is only a very very remote 
chance for them to get fixed sometime within the next 5 years.  I also regret 
that LATER and REMIND have been deprecated.  So... it's going to be WONTFIX.
Comment 5 Hixie (not reading bugmail) 2001-09-19 09:29:02 PDT
Reopening...
Comment 6 Hixie (not reading bugmail) 2001-09-19 09:30:09 PDT
...and taking. It's a valid issue and should not be forgotten, and SHOULD be
fixed. The WONTFIX resolution indicates that the bug *should not* be fixed.
Comment 7 Hixie (not reading bugmail) 2002-11-08 09:06:03 PST
->default owner
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-07 19:57:42 PDT
Created attachment 125167 [details] [diff] [review]
work in progress

This almost compiles.  However, I need to rewrite a good bit of the cascade
stuff at the end of nsCSSStyleSheet.cpp to do the last bit.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-07 20:00:37 PDT
Comment on attachment 125167 [details] [diff] [review]
work in progress

>Index: content/html/style/src/nsCSSStyleRule.cpp
>-  virtual nsCSSSelector* FirstSelector(void);
>+  virtual nsCSSSelectorList* Selector(void);
>   virtual void AddSelector(const nsCSSSelector& aSelector);
>   virtual void DeleteSelector(nsCSSSelector* aSelector);
>-  virtual void SetSourceSelectorText(const nsString& aSelectorText);
>-  virtual void GetSourceSelectorText(nsString& aSelectorText) const;

The middle two lines should have been removed as well.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-07 20:06:43 PDT
Created attachment 125168 [details] [diff] [review]
work in progress

Attaching corrected patch (one other thing, too), since I may not come back to
this for a while.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-07 21:59:41 PDT
Created attachment 125170 [details] [diff] [review]
patch

I realized the easy way to do what I wanted (create |RuleValue| objects
earlier), so here's a complete patch.

I still need to check that what I did to the MathML code (eek, odd dependency!)
will work.

I also intentionally broke a small piece of inspector (I made
|inDOMUtils::GetRuleWeight| always say that the weight was zero.  Rules don't
have a unique weight anymore.  I also may have unintentionally broken other
things in inspector.  I need to look more closely, and probably file a bug on
the inspector module owner about changing the UI in response to these changes.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-07 22:24:49 PDT
Created attachment 125171 [details] [diff] [review]
patch

I think the MathML change is right (although I cc:ed rbs just in case).

I decided to just remove the "Weight" column from inspector -- I suspect it's
not all that useful anyway (and the rules are sorted already).	(I think having
the full selector there, which this causes, is more useful than "Weight",
anyway.)  Someone who knows inspector (caillon?) should probably double-check
that I removed the right things, and also that this is what you want to do.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-07 22:31:39 PDT
A few further notes on the patch:
 * I think the original design was based on nsIStyleRule having methods like
|GetWeight|, that aren't needed and have been removed.  I don't see any need for
separate |nsIStyleRule| objects to match the different parts of the selector --
and there shouldn't be any problem with an element matching a single rule
multiple times.
 * I removed |mWeightedRules| from |RuleCascadeData|, which was unused after the
construction of the object, except that it owned references to the rules.  Since
the sheets own references to the rules (as do the rule nodes), I don't think
these references are needed.

I think it's otherwise pretty clear -- I just changed CSSStyleRuleImpl objects
so they own a nsCSSSelectorList (renamed from the SelectorList struct inside
nsCSSParser.cpp -- which already had the right destructor, so I don't need to
worry about ownership very much).  This means that there's a loop removed from
CSSParserImpl::ParseRuleSet and a loop added to InsertRuleByWeight (in
nsCSSStyleSheet.cpp) and a lot of changes to function signatures and member
variables.  In order to hold the rule / selector pair across the three parts of
CSSRuleProcessor::GetRuleCascade, I started creating the RuleValue objects
(half-initialized) in the first part (CascadeSheetRulesInto) rather than the
third (AddRule).
Comment 14 Christopher Aillon (sabbatical, not receiving bugmail) 2003-06-07 22:56:49 PDT
Comment on attachment 125171 [details] [diff] [review]
patch

r=caillon on the inspector portion of this patch, if you also remove the
olcWeight stuff in the themes:

http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/skin/class
ic/viewers/styleRules/styleRules.css#52 and
http://lxr.mozilla.org/mozilla/source/extensions/inspector/resources/skin/moder
n/viewers/styleRules/styleRules.css#52
Comment 15 Boris Zbarsky [:bz] 2003-06-09 18:49:02 PDT
I'll review this patch this coming weekend (I have exams until then).
Comment 16 jag (Peter Annema) 2003-06-13 19:12:59 PDT
Comment on attachment 125171 [details] [diff] [review]
patch

+nsCSSSelectorList::Clone()

...

+    nsCSSSelector **sel_cur = &l->mSelectors;


That should be

nsCSSSelector **sel_cur =  &lcopy->mSelectors;
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-13 20:59:46 PDT
Created attachment 125617 [details] [diff] [review]
patch

Fixes caillon's comment and jag's comment.
Comment 18 Boris Zbarsky [:bz] 2003-06-14 16:24:48 PDT
Comment on attachment 125617 [details] [diff] [review]
patch

>Index: content/html/style/src/nsCSSParser.cpp

>@@ -688,25 +632,25 @@ CSSParserImpl::ParseStyleAttribute(const
...
>   if (nsnull != declaration) {
>     // Create a style rule for the delcaration
>     nsICSSStyleRule* rule = nsnull;
>-    NS_NewCSSStyleRule(&rule, nsCSSSelector());
>+    NS_NewCSSStyleRule(&rule, nsnull);
>     rule->SetDeclaration(declaration);
>     *aResult = rule;
>   }

As long as you're here, could we change the test to 
|if (declration)|, remove the "rule" temporary (just do
NS_NewCSSStyleRule(aResult, nsnull)), and maybe do something sane on OOM?

>@@ -1516,75 +1460,59 @@ PRBool CSSParserImpl::ParseRuleSet(PRInt
>+  nsICSSStyleRule* rule = nsnull;
>+  NS_NewCSSStyleRule(&rule, slist);
>+  if (nsnull != rule) {
>+    rule->SetLineNumber(linenum);
>+    rule->SetDeclaration(declaration);
>+    (*aAppendFunc)(rule, aData);
>+    NS_RELEASE(rule);
>   }

I know you just copied this code, but that may make more sense as an nsCOMPtr
(and the check could be |if (rule)|).

What if "rule" ends up null here (OOM)?  It looks like we will leak slist in
that case...

>Index: content/html/style/src/nsCSSStyleRule.cpp
>-    mNext->ToString(aString, aSheet, IsPseudoElement(mTag), PR_FALSE);
>+    mNext->ToStringInternal(aString, aSheet, IsPseudoElement(mTag), PR_FALSE);

I know you just renamed this, but please pass '0' instead of 'PR_FALSE' (that
arg is a PRInt8, not a PRBool).

As a separate matter, I see no reason to use a PRInt8 for that state var; a
PRInt32 or PRIntn may be better...

A bunch of the nsCSSSelectorList code could do null-checks of pointers without
comparing to nsnull... (I know you just copied this code; either way is fine
with me).

>+nsCSSSelectorList*
>+nsCSSSelectorList::Clone()
>+{
>+  nsCSSSelectorList *list;
>+  nsCSSSelectorList **list_cur = &list;
>+  for (nsCSSSelectorList *l = this; l; l = l->mNext) {
>+    nsCSSSelectorList *lcopy = new nsCSSSelectorList();
>+    if (!lcopy) {
>+      delete list;

I think you want to init |list| to nsnull before entering the loop -- otherwise
you could end up deleting a garbage pointer here.

Also, you want to assign into *list_cur before entering the selector-copying
inner loop.  That way if a selector allocation fails, you won't leak lcopy and
whatever selectors you've allocated up to now.	Alternately, you need to delete
both |list| and |lcopy| in that inner error case.

All the mWeight stuff can be removed, right?

>Index: content/html/style/src/nsCSSStyleSheet.cpp
>+      mRuleArrays(nsnull, nsnull, RuleArraysDestroy, nsnull, 64),

Hmmm... I guess this is OK as long as we don't try to Clone() mRuleArrays.  I'd
prefer to pass in a non-null function pointer for that first arg and just make
it assert (NS_NOTREACHED) or something...

>+  nsObjectHashtable mRuleArrays; // of nsISupportsArray

You mean of nsAutoVoidArray, right?

>Index: content/html/style/src/nsICSSStyleRule.h
>+ * A selector group is the unit of selectors that each style rule has.

Why bother mentioning the term "group"?  The only place that refers to
nsCSSSelectorList as a "selector group" is the ParseSelectorGroup code in
nsCSSParser.... I'd just call it a "selector list" here.

>+  /**
>+   * Push a copy of |aSelector| on to the beginning of |mSelectors|,
>+   * setting its |mNext| to the current value of |mSelectors|.
>+   */
>+  void AddSelector(const nsCSSSelector& aSelector);

Make it clear that this does NOT update mWeight?

r+sr=me with those changes.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-14 16:42:17 PDT
Created attachment 125663 [details] [diff] [review]
patch

Patch with bz's comments addressed, except for:

> As long as you're here, could we change the test to 
> |if (declration)|, remove the "rule" temporary (just do
> NS_NewCSSStyleRule(aResult, nsnull)), and maybe do something sane on OOM?

I didn't remove the temporary.	It's even more useful when dealing with OOM.

> Hmmm... I guess this is OK as long as we don't try to Clone() mRuleArrays. 
I'd
> prefer to pass in a non-null function pointer for that first arg and just
make
> it assert (NS_NOTREACHED) or something...

I left this.  It's a very common pattern.
Comment 20 Boris Zbarsky [:bz] 2003-06-14 16:53:58 PDT
Comment on attachment 125663 [details] [diff] [review]
patch

OK, looks good.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-06-14 17:02:13 PDT
Fix checked in to trunk, 2003-06-14 16:50 -0700.

Note You need to log in before you can comment on or make changes to this bug.