Closed Bug 142648 Opened 22 years ago Closed 19 years ago

loose ends in GetSourceSelectorText()

Categories

(Core :: CSS Parsing and Computation, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: rbs, Assigned: domob)

References

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 6 obsolete files)

I had observed two bugs in GetSourceSelectorText() in the past and meant to 
file an entry in bugzilla for them (in expectation of what to do for the MathML 
bug 142592 which was just filed...).

1. GetSourceSelectorText() doesn't reset its output:
   So doing rule->GetSourceSelectorText(aSelector) in a loop does strange things
   if the (presumably nsAutoString) aSelector is declared outside the loop.

2. It doesn't quote its result, e.g., it will return
   *[color=blue] (i.e., no quotes). This is not clean when the value has
    multiple words.
nit while here, s/aIsNegated/isNegated/ in:
nsresult nsCSSSelector::ToString()
  PRBool aIsNegated = PRBool(0 < aNegatedIndex);
Blocks: 142592
Whiteboard: [good first bug]
Note: the code in question is in nsCSSSelector::ToStringInternal in
nsCSSStyleRule.cpp.
The problem is that the string is appended without any quotes or
backslash-escaping, ..., isn't it?
Are there already any methods which do CSS escaping? If not, I'll write one.
Where should I put it (because it may be of interest also for other use cases)
and how to name it?
> The problem is that the string is appended without any quotes or
> backslash-escaping, ..., isn't it?

Absolutely.

> Are there already any methods which do CSS escaping?

nsROCSSPrimitiveValue::GetEscapedURI has some code that does it...

In this case, I would say the way to go is to put quotes around the value and
escape quotes in the value; the sets of allowed characters in a string and in
the thing inside url() are sufficiently different that I'm not sure
GetEscapedURI will be of much help.
Yes, put quotes around and escape. I didn't know if there's already an useable
escaping function (of course for CSS strings), so that I wouldn't have to write
code already present. So I'll write some sort of
nsROCSSPrimitiveValue::GetEscapedString
and then do the fixing.
Putting it on nsROCSSPrimitiveValue wouldn't really help here.  Those are only
used for computed style.
So where to put? As helping function, not visible to others? But I don't think
this is really what we want...
I think a static function in this file would do just fine, actually...
Attached patch First patch (obsolete) — Splinter Review
This is it. But I wasn't really able to test me code further than compiling it,
because I don't know how to make Mozilla serialize a CSS rule to a file.
Attachment #171745 - Flags: review?(rbs)
Create a file with a <style> element with your rule in it, and a script that does:

  alert(document.styleSheets[0].cssRules[0].cssText);

That should let you test this....
It seems to work.
You should ask dbaron for r=. He may have a bigger picture in mind regarding this.

I am sure you can make the string-fu better, such as using AppendLiteral()
instead two calls to Append(ch). And "aReturn=EmptyString()" should be
aReturn.Truncate().
Attached patch Some more changes (obsolete) — Splinter Review
OK, did some more changes.
Attachment #171745 - Attachment is obsolete: true
Attachment #171898 - Flags: review?(dbaron)
Attachment #171745 - Flags: review?(rbs)
Comment on attachment 171898 [details] [diff] [review]
Some more changes

This looks good (I'm glad somebody is fixing this), but:
 * the escaping function needs to escape CR and LF, and may as well escape
everything below 0x20 (convert to numeric escapes).  nsTextFormatter::snprintf
may be helpful.
 * this escaping function should probably go in nsStyleUtil; it's useful in
other places
Attachment #171898 - Flags: review?(dbaron) → review-
There's just one problem: When using snprintf I've to allocate a temporary
buffer (5 characters long), but I can't throw an Out-Of-Memory-Error on failure,
as the "main function", where from the escaping is used, is declared as void!
Or should I use the stack for that buffer and ignore eventual errors in snprintf?
Attached patch patch (obsolete) — Splinter Review
This should escape everthing below 0x20 except of NUL. Should NUL be also
escaped (as special case), or is this not important?
Attachment #171898 - Attachment is obsolete: true
Attachment #172749 - Flags: review?(dbaron)
Comment on attachment 172749 [details] [diff] [review]
patch

>+  if (*in < 0x20)

Actually, you should probably exclude space (" && *in != ' '")

>+   nsTextFormatter::snprintf(buf, 5, NS_LITERAL_STRING("\\%hX ").get(), *in);

use NS_ARRAY_LENGTH(buf) instead of repeating "5".

>+  } else switch (*in)
>+  {

bring the "{" up onto the same line

r=dbaron
Attachment #172749 - Flags: review?(dbaron) → review+
Comment on attachment 172749 [details] [diff] [review]
patch

Oh, and please convert your new function to two-space indentation for
consistency with the rest of the file, and don't add a blank line to the end of
nsCSSStyleRule.cpp
You also need to avoid breakages by keeping existing users in sync:
http://lxr.mozilla.org/mozilla/source/layout/mathml/base/src/nsMathMLFrame.cpp#661

    selector.Assign(NS_LITERAL_STRING("*[") + attrName +
                    NS_LITERAL_STRING("=") + attrValue +
                    NS_LITERAL_STRING("]"));

becomes

    selector.Assign(NS_LITERAL_STRING("*[") + attrName +
                    NS_LITERAL_STRING("=\"") + attrValue +
                    NS_LITERAL_STRING("\"]"));
Attached patch corrected (obsolete) — Splinter Review
Better now? But why should I exclude spaces, they are 0x20 - and *in<0x20 is,
in my opinion, false for spaces. Where's the problem?
Attachment #172749 - Attachment is obsolete: true
oh, right, never mind the spaces thing :-)
Attachment #172790 - Flags: superreview?(bzbarsky)
Comment on attachment 172790 [details] [diff] [review]
corrected

>Index: layout/style/nsCSSStyleRule.cpp
>@@ -676,37 +679,37 @@ void nsCSSSelector::ToStringInternal(nsA
>+      aString.Append(NS_LITERAL_STRING("\"]"));

Why not:

  aString.AppendLiteral("\"]");

?

>@@ -1550,8 +1553,9 @@ NS_NewCSSStyleRule(nsICSSStyleRule** aIn
> }
>+

David asked that this line not be added...

>Index: layout/mathml/base/src/nsMathMLFrame.cpp

>     // check for duplicate, if a similar rule is already there, don't bother to add another one
>     // XXX bug 142648 - GetSourceSelectorText is in the format *[color=blue] (i.e., no quotes...) 
>     // XXXrbs need to keep this in sync with the fix for bug 142648

Remove the XXX part of the comment?

And perhaps call nsStyleUtil::EscapeCSSString on the attrValue here, and use
the escaped value in the Assign() call, just in case?

sr=bzbarsky with those fixed.
Attachment #172790 - Flags: superreview?(bzbarsky) → superreview+
Attached patch correctedSplinter Review
If this is ok, please may someone check this in?
Attachment #172790 - Attachment is obsolete: true
Assignee: dbaron → domob
Checked in on trunk for 1.8b.  This doesn't address problem 1 from comment 0,
does it?
No, I did nothing according to comment 1. You thought of something like
aString.Truncate() right at the beginning, or isn't it that simple?
Yeah, just a .Truncate() would do it (and checking callers to ensure they don't
depend on the current behavior).
Attached patch second fix (obsolete) — Splinter Review
This adds the truncate.
I've looked a bit around on lxr, the callers I found (in nsCSSStyleRule.cpp
itself and nsCSSStyleSheet.cpp) don't mind. Although, I don't think I found
every one.
Attachment #172907 - Flags: review?(bzbarsky)
Comment on attachment 172907 [details] [diff] [review]
second fix

r+sr=bzbarsky
Attachment #172907 - Flags: superreview+
Attachment #172907 - Flags: review?(bzbarsky)
Attachment #172907 - Flags: review+
Comment on attachment 172907 [details] [diff] [review]
second fix

Actually, no.  This is wrong, since it'll break linked selectors (see the mNext
call, eg).

You want to truncate in ToString(), methinks.  And test.  ;)
Attachment #172907 - Flags: superreview-
Attachment #172907 - Flags: superreview+
Attachment #172907 - Flags: review-
Attachment #172907 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
In ToString() it isn't possible, either, since SelectorLists aren't serialized
correctly then. So I inserted it for nsCSSSelectorList::ToString. This works,
but if nsCSSSelector::ToString may be called directly, it isn't ensured that
aString is truncated. Should I introduce a new parameter (maybe optional, so we
don't have to change any users), which controls if aString is truncated there?
Attachment #172907 - Attachment is obsolete: true
This one works with things like
h1, h2+h3,
c d > c + b a[abc~='\A ']
Attachment #172945 - Attachment is obsolete: true
Attachment #173147 - Flags: review?(bzbarsky)
Simply do this, and let the recursion voodoo do the rest:

void
nsCSSSelector::ToString(nsAString& aString, nsICSSStyleSheet* aSheet) const
{
+ if (!mNext)
+   aString.Truncate();
+
  ToStringInternal(aString, aSheet, IsPseudoElement(mTag), 0);
}
Mis-placed... That conditional truncate should be in |ToStringInternal| instead.
I don't think that would work together with Selector-Lists, would it?
Yes, that would break for selector lists.

I'm going to try to look at the patch tonight.
Comment on attachment 173147 [details] [diff] [review]
optional argument

r+sr=bzbarsky, but no need for the const on the PRBool
Attachment #173147 - Flags: superreview+
Attachment #173147 - Flags: review?(bzbarsky)
Attachment #173147 - Flags: review+
My style of coding is to put as many const's as possible (because it's very
unlikely that anyone would want to change aAppend). I think this can only help
the compiler, and if not, it can't be a problem. Or am I wrong?
I think, typically, that things that are passed by value are not passed as const
in Mozilla...  I'll make that change before checking in.
In particular, const on function parameters is a little messy because of
compiler inconsistencies -- some compilers consider it part of the signature,
even though it's not supposed to be (i.e., they incorrectly consider void
foo(const int) and void foo (int) to be different signatures).
OK, do as you think. This is just what I normally do (in projects only
considered to build under cygwin GCC).
Checked in.  I believe this can be marked fixed?
I think so.
Fixed.  Thank you for the patches, Daniel!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: