Replace WKStringGetCharactersPtr() with WKStringGetCharacters()
Created attachment 88659 [details] Patch
Comment on attachment 88659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88659&action=review I should be encouraging you to write API tests for the new API you're adding. They're pretty easy to write; just add some code to Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp. > Source/WebKit2/Shared/WebString.h:68 > + ASSERT(buffer); > + if (!buffer) > + return 0; I don't think we're normally this defensive in the WK2 API. > Source/WebKit2/Shared/WebString.h:72 > + size_t lengthInBytes = m_string.length() * sizeof(UChar); > + if (bufferSize > lengthInBytes) > + bufferSize = lengthInBytes; > + memcpy(buffer, m_string.characters(), bufferSize); This will cause a buffer overrun if bufferSize is less than lengthInBytes. > Source/WebKit2/Shared/API/c/WKString.cpp:55 > +size_t WKStringGetCharacters(WKStringRef stringRef, WKChar* buffer, size_t bufferSize) It's surprising that this API takes a number bytes to copy rather than a number of characters.
Comment on attachment 88659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88659&action=review >> Source/WebKit2/Shared/WebString.h:72 >> + memcpy(buffer, m_string.characters(), bufferSize); > > This will cause a buffer overrun if bufferSize is less than lengthInBytes. Maybe this was intentional? Perhaps the caller is required to pass in a large enough bufferSize?
Comment on attachment 88659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88659&action=review >>> Source/WebKit2/Shared/WebString.h:72 >>> + memcpy(buffer, m_string.characters(), bufferSize); >> >> This will cause a buffer overrun if bufferSize is less than lengthInBytes. > > Maybe this was intentional? Perhaps the caller is required to pass in a large enough bufferSize? Jeff politely pointed out to me that there is in fact no overrun here.
Created attachment 88670 [details] Patch
Comment on attachment 88670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88670&action=review > Source/WebKit2/Shared/WebString.h:68 > + if (bufferLength > length) > + bufferLength = length; You could use std::min here. > Source/WebKit2/Shared/API/c/WKString.cpp:58 > + return (toImpl(stringRef)->getCharacters(static_cast<UChar*>(buffer), bufferLength)); I'm a little surprised you don't need reinterpret_cast here. > Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:60 > + maxSize = WKStringGetLength(string); > + TEST_ASSERT(maxSize == 5); > + > + WKChar* uniBuffer = new WKChar[maxSize]; > + actualSize = WKStringGetCharacters(string, uniBuffer, maxSize); > + TEST_ASSERT(actualSize == 5); > + > + WKChar helloBuffer[] = { 'h', 'e', 'l', 'l', 'o' }; > + TEST_ASSERT(!memcmp(uniBuffer, helloBuffer, 10)); > + > + delete[] uniBuffer; > + Might be good to have tests that pass in buffers that are larger and smaller than needed.
(In reply to comment #6) > (From update of attachment 88670 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88670&action=review > > > Source/WebKit2/Shared/WebString.h:68 > > + if (bufferLength > length) > > + bufferLength = length; > > You could use std::min here. Will do. > > > Source/WebKit2/Shared/API/c/WKString.cpp:58 > > + return (toImpl(stringRef)->getCharacters(static_cast<UChar*>(buffer), bufferLength)); > > I'm a little surprised you don't need reinterpret_cast here. Yeah, but it works. My typical MO is to try static_cast first, and use reinterpret_cast if the compiler complains. :) > > > Tools/TestWebKitAPI/Tests/WebKit2/WKString.cpp:60 > > + maxSize = WKStringGetLength(string); > > + TEST_ASSERT(maxSize == 5); > > + > > + WKChar* uniBuffer = new WKChar[maxSize]; > > + actualSize = WKStringGetCharacters(string, uniBuffer, maxSize); > > + TEST_ASSERT(actualSize == 5); > > + > > + WKChar helloBuffer[] = { 'h', 'e', 'l', 'l', 'o' }; > > + TEST_ASSERT(!memcmp(uniBuffer, helloBuffer, 10)); > > + > > + delete[] uniBuffer; > > + > > Might be good to have tests that pass in buffers that are larger and smaller than needed. Good idea, I'll add those.
Committed r83198: <http://trac.webkit.org/changeset/83198>
http://trac.webkit.org/changeset/83198 might have broken SnowLeopard Intel Release (Build)