Closed
Bug 142648
Opened 22 years ago
Closed 19 years ago
loose ends in GetSourceSelectorText()
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: rbs, Assigned: domob)
References
Details
(Whiteboard: [good first bug])
Attachments
(2 files, 6 obsolete files)
10.65 KB,
patch
|
Details | Diff | Splinter Review | |
3.37 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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);
Updated•20 years ago
|
Whiteboard: [good first bug]
Comment 2•20 years ago
|
||
Note: the code in question is in nsCSSSelector::ToStringInternal in nsCSSStyleRule.cpp.
Assignee | ||
Comment 3•20 years ago
|
||
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?
Comment 4•20 years ago
|
||
> 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.
Assignee | ||
Comment 5•20 years ago
|
||
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.
Comment 6•20 years ago
|
||
Putting it on nsROCSSPrimitiveValue wouldn't really help here. Those are only used for computed style.
Assignee | ||
Comment 7•20 years ago
|
||
So where to put? As helping function, not visible to others? But I don't think this is really what we want...
Comment 8•20 years ago
|
||
I think a static function in this file would do just fine, actually...
Assignee | ||
Comment 9•20 years ago
|
||
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)
Comment 10•20 years ago
|
||
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....
Assignee | ||
Comment 11•20 years ago
|
||
It seems to work.
Reporter | ||
Comment 12•20 years ago
|
||
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().
Assignee | ||
Comment 13•20 years ago
|
||
OK, did some more changes.
Attachment #171745 -
Attachment is obsolete: true
Attachment #171898 -
Flags: review?(dbaron)
Assignee | ||
Updated•20 years ago
|
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-
Assignee | ||
Comment 15•19 years ago
|
||
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!
Assignee | ||
Comment 16•19 years ago
|
||
Or should I use the stack for that buffer and ignore eventual errors in snprintf?
Use the stack for the buffer.
Assignee | ||
Comment 18•19 years ago
|
||
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
Reporter | ||
Comment 21•19 years ago
|
||
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("\"]"));
Assignee | ||
Comment 22•19 years ago
|
||
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 :-)
Assignee | ||
Updated•19 years ago
|
Attachment #172790 -
Flags: superreview?(bzbarsky)
Comment 24•19 years ago
|
||
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+
Assignee | ||
Comment 25•19 years ago
|
||
If this is ok, please may someone check this in?
Attachment #172790 -
Attachment is obsolete: true
Updated•19 years ago
|
Assignee: dbaron → domob
Comment 26•19 years ago
|
||
Checked in on trunk for 1.8b. This doesn't address problem 1 from comment 0, does it?
Assignee | ||
Comment 27•19 years ago
|
||
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?
Comment 28•19 years ago
|
||
Yeah, just a .Truncate() would do it (and checking callers to ensure they don't depend on the current behavior).
Assignee | ||
Comment 29•19 years ago
|
||
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 30•19 years ago
|
||
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 31•19 years ago
|
||
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+
Assignee | ||
Comment 32•19 years ago
|
||
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?
Assignee | ||
Updated•19 years ago
|
Attachment #172907 -
Attachment is obsolete: true
Assignee | ||
Comment 33•19 years ago
|
||
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)
Reporter | ||
Comment 34•19 years ago
|
||
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); }
Reporter | ||
Comment 35•19 years ago
|
||
Mis-placed... That conditional truncate should be in |ToStringInternal| instead.
Assignee | ||
Comment 36•19 years ago
|
||
I don't think that would work together with Selector-Lists, would it?
Comment 37•19 years ago
|
||
Yes, that would break for selector lists. I'm going to try to look at the patch tonight.
Comment 38•19 years ago
|
||
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+
Assignee | ||
Comment 39•19 years ago
|
||
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?
Comment 40•19 years ago
|
||
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).
Assignee | ||
Comment 42•19 years ago
|
||
OK, do as you think. This is just what I normally do (in projects only considered to build under cygwin GCC).
Comment 43•19 years ago
|
||
Checked in. I believe this can be marked fixed?
Assignee | ||
Comment 44•19 years ago
|
||
I think so.
Comment 45•19 years ago
|
||
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.
Description
•