Bug 6228 - DOMString/DOMStringImpl/AtomicString need enhancements before replacing QString
Summary: DOMString/DOMStringImpl/AtomicString need enhancements before replacing QString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Enhancement
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 6294
  Show dependency treegraph
 
Reported: 2005-12-24 06:03 PST by Eric Seidel (no email)
Modified: 2005-12-29 19:27 PST (History)
0 users

See Also:


Attachments
Adds find(), contains(), startsWith(), endsWith() to the DOM string classes (10.12 KB, patch)
2005-12-24 06:05 PST, Eric Seidel (no email)
mjs: review+
Details | Formatted Diff | Diff
Use new string methods in khtml/css (18.15 KB, patch)
2005-12-24 06:16 PST, Eric Seidel (no email)
mjs: review-
Details | Formatted Diff | Diff
Other string uses (including some unrelated cleanup) (6.11 KB, patch)
2005-12-24 06:16 PST, Eric Seidel (no email)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-12-24 06:03:37 PST
DOMString/DOMStringImpl/AtomicString need enhancements before replacing QString

The attached patch adds find(), contains(), startsWith() and endsWith() functionality to the string classes.  I 
have another patch which uses these throughout CSSStyleSelector and other locations in WebKit which I'll 
attach separately.
Comment 1 Eric Seidel (no email) 2005-12-24 06:05:04 PST
Created attachment 5266 [details]
Adds find(), contains(), startsWith(), endsWith() to the DOM string classes
Comment 2 Eric Seidel (no email) 2005-12-24 06:16:08 PST
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.
Comment 3 Eric Seidel (no email) 2005-12-24 06:16:48 PST
Created attachment 5268 [details]
Other string uses (including some unrelated cleanup)
Comment 4 Eric Seidel (no email) 2005-12-24 06:48:44 PST
Woohoo!  Autovicki says these patches yeild another .3ms improvement (out of 133.0)!
Comment 5 Maciej Stachowiak 2005-12-25 01:15:35 PST
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 6 Maciej Stachowiak 2005-12-25 02:13:00 PST
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 7 Darin Adler 2005-12-25 08:12:12 PST
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 8 Darin Adler 2005-12-25 08:14:29 PST
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 9 Darin Adler 2005-12-25 08:19:46 PST
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.
Comment 10 Eric Seidel (no email) 2005-12-29 19:27:51 PST
Landed.