Bug 24979 - Avoid type-coercion into AtomicString in AtomicString methods
Summary: Avoid type-coercion into AtomicString in AtomicString methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Mike Belshe
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-31 20:04 PDT by Mike Belshe
Modified: 2009-04-06 09:11 PDT (History)
1 user (show)

See Also:


Attachments
bug24979 (3.98 KB, patch)
2009-03-31 20:56 PDT, Mike Belshe
mbelshe: review-
Details | Formatted Diff | Diff
updated patch (2.54 KB, patch)
2009-04-05 15:24 PDT, Mike Belshe
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Belshe 2009-03-31 20:04:24 PDT
When using
  AtomicString.find()
  AtomicStirng.contains()
  AtomicString.startsWith()
  AtomicString.endsWith()

if you pass a const char* string literal, the argument will be coerced into a String, then into an AtomicString, and then ripped out. This causes unnecessary churn on the AtomicString table.
Comment 1 Mike Belshe 2009-03-31 20:56:50 PDT
Created attachment 29155 [details]
bug24979
Comment 2 Mike Belshe 2009-03-31 20:58:34 PDT
Two notes on the patch:

a. Document.cpp and FrameTree.cpp needed small changes because they use AtomicString methods which are ambiguous now that we have the new methods plumbed in.

b. I could make these methods chain - e.g. have the one which takes the AtomicString argument in turn call the one which takes the string argument.  I don't know which style is preferred on the WebKit side.  Just let me know if you have a preference.
Comment 3 Alexey Proskuryakov 2009-04-01 00:10:09 PDT
My suggestion was to replace AtomicString methods with String ones, not to add new methods. Unless we're using them with AtomicStrings very often, a string() call here and there should probably be better than having to disambiguate similar methods. What do you think?
Comment 4 Darin Adler 2009-04-01 13:33:17 PDT
Comment on attachment 29155 [details]
bug24979

>      for (frame = m_thisFrame; frame; frame = frame->tree()->parent()) {
> -        if (frame->tree()->name().startsWith(framePathPrefix))
> +        if (frame->tree()->name().startsWith(String(framePathPrefix)))

It would be much better to overload for const char* too rather than adding the explicit casts to String. We don't want to create any destroy String objects either.
Comment 5 Mike Belshe 2009-04-05 15:24:17 PDT
Created attachment 29268 [details]
updated patch

Removed the versions which take AtomicStrings.  startsWith() and endsWith() have no versions which take const char* anyway, so they now take Strings.  find() and contains() now offer both const char* and String variants.
Comment 6 Alexey Proskuryakov 2009-04-06 04:04:09 PDT
Comment on attachment 29268 [details]
updated patch

r=me

We should eventually change these methods to take named enum constants instead of booleans for caseSensitive  - a call like str.contains("foo", true) is quite confusing, str.contains("foo", CaseSensitive) would be much better. But that's definitely a separate issue.
Comment 7 Darin Adler 2009-04-06 09:01:40 PDT
(In reply to comment #5)
> startsWith() and endsWith()
> have no versions which take const char* anyway, so they now take Strings.

Side note: In my opinion that's a temporary situation. Any time we find ourselves doing a string operation on a const char* constant, I think it's a good optimization technique to add the const char* override to avoid the memory allocation. Similarly, I think we need to come up with an approach that will allow us to do this for substrings and other UChar*/length pairs.
Comment 8 Darin Fisher (:fishd, Google) 2009-04-06 09:11:12 PDT
Landed as:  http://trac.webkit.org/changeset/42245
Comment 9 Darin Adler 2009-04-06 09:11:53 PDT
(In reply to comment #6)
> We should eventually change these methods to take named enum constants instead
> of booleans for caseSensitive

Or use two separate functions for the two variants.