Summary: | Avoid type-coercion into AtomicString in AtomicString methods | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Belshe <mbelshe> | ||||||
Component: | Platform | Assignee: | Mike Belshe <mbelshe> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Mike Belshe
2009-03-31 20:04:24 PDT
Created attachment 29155 [details] bug24979 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. 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 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. 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 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.
(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. Landed as: http://trac.webkit.org/changeset/42245 (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. |