Bug 24979 – Avoid type-coercion into AtomicString in AtomicString methods

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mike Belshe]

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.

[Mike Belshe]

Created attachment 29155 [details]
bug24979

[Mike Belshe]

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.

[Alexey Proskuryakov]

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?

[Darin Adler]

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.

[Mike Belshe]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

(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.

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42245

[Darin Adler]

(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.