Bug 73586 - Upstream text codec and web string files of BlackBerry API
Summary: Upstream text codec and web string files of BlackBerry API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-12-01 13:36 PST by Jacky Jiang
Modified: 2011-12-14 00:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (14.92 KB, patch)
2011-12-01 14:05 PST, Jacky Jiang
no flags Details | Formatted Diff | Diff
Updated patch (16.34 KB, patch)
2011-12-09 13:51 PST, Jacky Jiang
zimmermann: review+
Details | Formatted Diff | Diff
Updated patch (16.40 KB, patch)
2011-12-12 13:45 PST, Jacky Jiang
no flags Details | Formatted Diff | Diff
Updated patch (16.41 KB, patch)
2011-12-12 17:58 PST, Jacky Jiang
no flags Details | Formatted Diff | Diff
Patch (14.87 KB, patch)
2011-12-13 21:24 PST, Jacky Jiang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jacky Jiang 2011-12-01 13:36:50 PST
Upstream text codec and web string implementation files of BlackBerry API.
Comment 1 Jacky Jiang 2011-12-01 14:05:45 PST
Created attachment 117484 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-12-01 17:23:36 PST
This is all implemented in cross platform code, why does Blackberry have its own copy?
Comment 3 Jacky Jiang 2011-12-02 08:21:09 PST
(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.
Comment 4 Alexey Proskuryakov 2011-12-02 08:47:07 PST
OK
Comment 5 Leo Yang 2011-12-08 23:52:36 PST
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.
Comment 6 Jacky Jiang 2011-12-09 13:51:31 PST
Created attachment 118629 [details]
Updated patch

Updated the patch based on Leo and Dan's review.
Comment 7 Nikolas Zimmermann 2011-12-10 01:21:12 PST
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.
Comment 8 Jacky Jiang 2011-12-12 13:45:33 PST
Created attachment 118837 [details]
Updated patch

Updated the patch according to Nikolas' comments.
Comment 9 Jacky Jiang 2011-12-12 15:14:15 PST
Comment on attachment 118837 [details]
Updated patch

I will update again.
Comment 10 Jacky Jiang 2011-12-12 17:58:58 PST
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 11 Leo Yang 2011-12-13 18:03:18 PST
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 12 Jacky Jiang 2011-12-13 18:37:31 PST
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.
Comment 13 Jacky Jiang 2011-12-13 21:24:22 PST
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 14 Jacky Jiang 2011-12-13 21:40:52 PST
Comment on attachment 119152 [details]
Patch

Nikolas, ping.
Comment 15 Jacky Jiang 2011-12-13 23:10:32 PST
Thanks Dan!
Comment 16 WebKit Review Bot 2011-12-14 00:26:11 PST
Comment on attachment 119152 [details]
Patch

Clearing flags on attachment: 119152

Committed r102747: <http://trac.webkit.org/changeset/102747>
Comment 17 WebKit Review Bot 2011-12-14 00:26:17 PST
All reviewed patches have been landed.  Closing bug.