Bug 73586 – Upstream text codec and web string files of BlackBerry API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Jacky Jiang]

Upstream text codec and web string implementation files of BlackBerry API.

[Jacky Jiang]

Created attachment 117484 [details]
Patch

[Alexey Proskuryakov]

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

[Jacky Jiang]

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

[Alexey Proskuryakov]

OK

[Leo Yang]

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.

[Jacky Jiang]

Created attachment 118629 [details]
Updated patch

Updated the patch based on Leo and Dan's review.

[Nikolas Zimmermann]

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.

[Jacky Jiang]

Created attachment 118837 [details]
Updated patch

Updated the patch according to Nikolas' comments.

[Jacky Jiang]

Comment on attachment 118837 [details]
Updated patch

I will update again.

[Jacky Jiang]

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.

[Leo Yang]

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?

[Jacky Jiang]

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.

[Jacky Jiang]

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.

[Jacky Jiang]

Comment on attachment 119152 [details]
Patch

Nikolas, ping.

[Jacky Jiang]

Thanks Dan!

[WebKit Review Bot]

Comment on attachment 119152 [details]
Patch

Clearing flags on attachment: 119152

Committed r102747: <http://trac.webkit.org/changeset/102747>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.