Summary: | [CSSOM] issues with cssText and selectorText | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||
Component: | CSS | Assignee: | Yuzo Fujishima <yuzo> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, annevk, bdakin, darin, ddkilzer, eric, hamaji, webkit.bugzilla, webkit-ews, webkit.review.bot, yuzo | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
URL: | http://glazman.org/forCSSWG/selectorTextBug.html | ||||||||||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2010-02-03 08:09:17 PST
FWIW, http://dev.w3.org/csswg/cssom/ now defines the rules for how to serialize these things. *** Bug 33725 has been marked as a duplicate of this bug. *** Created attachment 56354 [details] Fix for Bug 34529 - [CSSOM] issues with cssText and selectorText Created attachment 56357 [details]
Style fix
Created attachment 56358 [details]
Add another test
Comment on attachment 56358 [details]
Add another test
In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead.
(In reply to comment #6) > In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead. Good point! But for building strings a character at a time, StringBuilder isn't efficient either. Vector<UChar> is probably the type of choice, or even Vector<UChar, 256>. Created attachment 56446 [details]
Use Vector<UChar> for better performance
Created attachment 56447 [details]
Use Vector<UChar> for better performance
Simon, Darin, Thank you for reviewing this. I've changed to use Vector<UChar>. Please take another look. (In reply to comment #7) > (In reply to comment #6) > > In general this looks good, but doing lots of String appends is inefficient. You should use StringBuilder instead. > > Good point! But for building strings a character at a time, StringBuilder isn't efficient either. Vector<UChar> is probably the type of choice, or even Vector<UChar, 256>. Attachment 56447 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2320319 Created attachment 56450 [details]
Try to fix Qt compilation error
Comment on attachment 56450 [details]
Try to fix Qt compilation error
WebCore/css/CSSOMUtils.cpp:56
+ append(appendTo, String::format("\\%x ", c));
Do we have test case which checks this path?
WebCore/css/CSSOMUtils.cpp:76
+ if (c <= 0x1f || (0x30 <= c && c <= 0x39 && (isFirst || (isSecond && isFirstCharHyphen))))
Are there any reason we cannot use character literals such as '0' or '9' (or maybe L'0' or L'9') ?
Thank you for the review. (In reply to comment #13) > (From update of attachment 56450 [details]) > WebCore/css/CSSOMUtils.cpp:56 > + append(appendTo, String::format("\\%x ", c)); > Do we have test case which checks this path? > Yes. Tests 1 through 6 run the path. > WebCore/css/CSSOMUtils.cpp:76 > + if (c <= 0x1f || (0x30 <= c && c <= 0x39 && (isFirst || (isSecond && isFirstCharHyphen)))) > Are there any reason we cannot use character literals such as '0' or '9' (or maybe L'0' or L'9') ? I used character code in hopes of making it easier to see if the function truthfully implements http://dev.w3.org/csswg/cssom/#common-serializing-idioms Created attachment 56457 [details]
Add test descriptions
Comment on attachment 56457 [details] Add test descriptions (In reply to comment #15) > Created an attachment (id=56457) [details] > Add test descriptions Thanks for your description! This change looks good to me. Committed r60880: <http://trac.webkit.org/changeset/60880> http://trac.webkit.org/changeset/60880 might have broken Qt Linux Release |