Summary: | DOMString/DOMStringImpl/AtomicString need enhancements before replacing QString | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||
Component: | WebKit Misc. | Assignee: | Eric Seidel (no email) <eric> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Enhancement | ||||||||||
Priority: | P4 | ||||||||||
Version: | 420+ | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.4 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 6294 | ||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2005-12-24 06:03:37 PST
Created attachment 5266 [details]
Adds find(), contains(), startsWith(), endsWith() to the DOM string classes
Created attachment 5267 [details]
Use new string methods in khtml/css
I separated out khtml/css since that's where the bulk of my changes went.
Created attachment 5268 [details]
Other string uses (including some unrelated cleanup)
Woohoo! Autovicki says these patches yeild another .3ms improvement (out of 133.0)! Comment on attachment 5267 [details]
Use new string methods in khtml/css
+ bool important = priority.find("important", 0, false) != -1;
Replace this with startsWith for clarity maybe?
- refPos = path.find("#", 0);
+ refPos = path.find("#");
r- because of this apparent bug (or possible undocumented fix) otherwise looks
fine.
Comment on attachment 5268 [details]
Other string uses (including some unrelated cleanup)
+ if (str.find("url", 0, false ) == 0)
replace with startsWith?
otherwise r=me
Comment on attachment 5266 [details]
Adds find(), contains(), startsWith(), endsWith() to the DOM string classes
In the future, the startsWith algorithm should not use find, because it only
needs to compare once at the start of the string and find will go on to search
the rest of the string. The things that find does to optimize that search are
not helpful for startsWith.
The patch has a number of tab characters in it. They should be converted to
spaces before it's landed.
I think that dom_string.h's version of contains and startsWith should call
contains and startsWith in the underlying DOMStringImpl and we should have a
contains function in DOMStringImpl. In general, all the functions should be
present in DOMStringImpl and then reflected in DOMString as needed.
DOMStringImpl::find has a comment still mentions QString.
Comment on attachment 5268 [details]
Other string uses (including some unrelated cleanup)
Looking at these searches, it occurs to me that ", false" is a pretty poor way
to indicate a case insensitive search. We should consider alternate API for
that.
+ if (str.find("url", 0, false ) == 0)
The above is a case of "startsWith" and would be clearer if we called it that
way.
+ if (str.length() && str[0] == '=')
And this is startsWith too (although a character, not a string).
Comment on attachment 5267 [details]
Use new string methods in khtml/css
In parseUASheet, we can make the DOMString from the char* without decoding
because we know it has all ASCII characters in it. Unlike QString, that
function does not work for non-ASCII Latin-1 characters. So I think it's worth
a comment and adding an assertion to DOMStringImpl::DOMStringImpl(const char *)
to check for non-ASCII characters.
Landed. |