Bug 73586

Created attachment 117484 [details] Patch

This is all implemented in cross platform code, why does Blackberry have its own copy?

(In reply to comment #2) > This is all implemented in cross platform code, why does Blackberry have its own copy? Hi Alexey, they are wrappers for the cross platform code. We have a layer between the WebKit and Browser App, these wrapper APIs are exposed to this layer, we are not going to expose or include the cross platform code to that layer directly.

OK

Comment on attachment 117484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117484&action=review Informal review. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:28 > +#include "Vector.h" #include <wtf/Vector.h> instead of #include "Vector.h". > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:36 > +namespace BlackBerry { > +namespace WebKit { > + > + I would keep only one empty line here. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:47 > + TextEncoding te(encoding); I would use textEncoding instead of te. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:57 > + WTF::String enc(encoding); WTF:: is not needed, should be removed. And I would change enc to encodingName. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:68 > + > + I would remove an extra empty line. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:71 > + TextEncoding teSource(sourceEncoding); I would change teSource to textEncodingSource. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:75 > + TextEncoding teTarget(targetEncoding); I would name the variable textEncodingTarget. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:82 > + WTF::String ucs2 = codecSource.decode(sourceStart, sourceLength, true, true, sawError); Remove WTF::. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:89 > + WTF::CString encoded = codecTarget.encode(ucs2.characters(), ucs2.length(), WebCore::EntitiesForUnencodables); Ditto. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:108 > + WTF::Vector<char> vect; Remove WTF::. I would rename vect to something like result. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:119 > + WTF::Vector<char> vect; Ditto. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:132 > + WTF::String escapedString(escaped.data(), escaped.length()); Remove WTF::. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:133 > + I would remove this empty line. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:135 > + WTF::String urlString = WebCore::decodeURLEscapeSequences(escapedString); > + WTF::CString utf8 = urlString.utf8(); Remove WTF::. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:143 > + WTF::String urlString(url.data(), url.length()); Remove WTF::. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:144 > + I would remove this empty line. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:146 > + WTF::String escapedString = WebCore::encodeWithURLEscapeSequences(urlString); > + WTF::CString utf8 = escapedString.utf8(); Remove WTF::. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:156 > + WTF::String mimeString(mime.data(), mime.length()); > + > + WTF::String ext = WebCore::MIMETypeRegistry::getPreferredExtensionForMIMEType(mimeString); Remove WTF::. I would change ext to extension. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:160 > + WTF::CString utf8 = ext.utf8(); Remove WTF::. > Source/WebKit/blackberry/Api/WebString.cpp:26 > +using WTF::String; Could this line be removed? > Source/WebKit/blackberry/Api/WebString.cpp:31 > +WebString::WebString(const char* latin1) > +: m_impl(static_cast<WebStringImpl*>(WTF::StringImpl::create(latin1).releaseRef())) You should add 4 spaces before the colon. > Source/WebKit/blackberry/Api/WebString.cpp:36 > +: m_impl(static_cast<WebStringImpl*>(WTF::StringImpl::create(latin1, length).releaseRef())) Ditto. > Source/WebKit/blackberry/Api/WebString.cpp:41 > +: m_impl(static_cast<WebStringImpl*>(WTF::StringImpl::create(utf16, length).releaseRef())) Ditto. > Source/WebKit/blackberry/Api/WebString.cpp:59 > +: m_impl(str.m_impl) Ditto. > Source/WebKit/blackberry/Api/WebString.cpp:67 > + return WTF::String::fromUTF8(utf8); Remove WTF::. > Source/WebKit/blackberry/Api/WebString.cpp:87 > + WTF::String str(m_impl); Ditto. > Source/WebKit/blackberry/Api/WebString.h:44 > + WebString& operator=(const WebString&); > + > + std::string utf8() const; > + > + static WebString fromUtf8(const char* utf8); > + I would remove extra empty lines. > Source/WebKit/blackberry/Api/WebString.h:48 > + Ditto. > Source/WebKit/blackberry/Api/WebString.h:51 > + Ditto.

Created attachment 118629 [details] Updated patch Updated the patch based on Leo and Dan's review.

Comment on attachment 118629 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118629&action=review Looks good, r=me! Please fix these before landing, or upload another patch that I could cq+. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:23 > +#include "CString.h" <wtf/text/CString.h> > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:53 > + String lowercasedEncoding = String(encoding).lower(); > + if (lowercasedEncoding.startsWith("iso-8859") We might want to add a comment here, that this is slow and could easily be optimized, by directly inspecting encoding[i]. > Source/WebKit/blackberry/Api/WebString.cpp:26 > +using WTF::String; WTFString.h already declares that, this is unncessary here, no? > Source/WebKit/blackberry/Api/WebString.cpp:109 > + return WTF::equal(m_impl, utf8); doesn't WTF already do "using WTF::equal" in their public headers? > Source/WebKit/blackberry/Api/WebString.cpp:114 > + return WTF::equalIgnoringCase(m_impl, utf8); Ditto.

Created attachment 118837 [details] Updated patch Updated the patch according to Nikolas' comments.

Comment on attachment 118837 [details] Updated patch I will update again.

Created attachment 118926 [details] Updated patch Keep WTF for equal() and equalIgnoringCase(), because the compiler was always trying to use WebString.equal() and WebString.equalIgnoringCase() instead of the WTF functions as the cadidates which caused compiling error when we removed WTF.

Comment on attachment 118926 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118926&action=review Some minor comments. > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:28 > +#include <wtf/Vector.h> > +#include <wtf/text/CString.h> > +#include <wtf/text/WTFString.h> Wouldn't check-webkit-style complain about sorting problem? Sorting is case sensitive? > Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:32 > +using namespace WTF; Is this needed?

Comment on attachment 118926 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118926&action=review >> Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:28 >> +#include <wtf/text/WTFString.h> > > Wouldn't check-webkit-style complain about sorting problem? Sorting is case sensitive? If I remember correctly the ASCII of 'V' should be less than 't' and I don't see any complains. >> Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:32 >> +using namespace WTF; > > Is this needed? String, CString and Vector are using it, however, the header files may already declare it. I can try to remove.

Created attachment 119152 [details] Patch Remove using namespace WTF in WebKitTextCodex.cpp as the WTFString and Vector header files already declares it for String, CString and Vector.

Comment on attachment 119152 [details] Patch Nikolas, ping.

Thanks Dan!

Comment on attachment 119152 [details] Patch Clearing flags on attachment: 119152 Committed r102747: <http://trac.webkit.org/changeset/102747>

All reviewed patches have been landed. Closing bug.