Bug 74383 – Upstream PlatformMouseEvent and LocalizedStrings into WebCore/platform/blackberry

• Component: Platform
• Status: RESOLVED FIXED

[Mary Wu]

Upstream PlatformMouseEvent and LocalizedString into WebCore/platform/blackberry: 
LocalizedStringsBlackBerry.cpp, PlatformMouseEventBlackBerry.cpp

[Mary Wu]

Created attachment 118968 [details]
Patch

[Rob Buis]

Comment on attachment 118968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118968&action=review

> Source/WebCore/ChangeLog:10
> +        Rob Buis  <[e-mail]>

I think there were more contributors, for instance I dont think I touched PlatformMouseEvent.

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:72
> +    // however, WebCore wants a '-' (e.g. en_us should read en-us)

Comment should be a sentence.

> Source/WebCore/platform/blackberry/PlatformMouseEventBlackBerry.cpp:36
> +    , m_timestamp(WTF::currentTime())

Is the WTF prefix needed?

[Rob Buis]

Comment on attachment 118968 [details]
Patch

Clearing r? flag for now, see my comments.

[Mary Wu]

Created attachment 119125 [details]
Patch

[Daniel Bates]

Comment on attachment 119125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119125&action=review

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:24
> +

Nit: I suggest that we remove this empty line as I don't feel it improves the readability of this file.

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:68
> +String platformDefaultLanguage()

Would it make sense to re-write this function using WTF::String functions instead of using the std::string routines? That is, can we just convert BlackBerry::Platform::Client::get()->getLocale() to a WTF::String then manipulate it as as such? I suggest taking this approach since the WTF::String functions tend to be a bit more concise and at the end of the day we want to return a WTF::String.

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:71
> +    // getLocale() returns a POSIX lcoale which uses '_' to separate language and country

lcoale => locale.

country => country.

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:72
> +    // however, we use '-' instead of '_' in WebCore (e.g. en_us should read en-us)

however => However

Missing period at the end of this line.

[Mary Wu]

Created attachment 119170 [details]
Patch

[Mary Wu]

Created attachment 119381 [details]
Patch

[Daniel Bates]

Comment on attachment 119381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119381&action=review

Thanks Mary for updating the patch. Notice that most of these these methods build a WTF::String object from a C-string. We should probably leak the WTF::String objects if these methods are called frequently instead of building a WTF::String object on each method call.

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:587
> +}

Do we need to use String::fromUTF8 here? It seems like it would be sufficient to write this as:

return String(", ...");

[Daniel Bates]

Comment on attachment 119381 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119381&action=review

> Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp:586
> +    return String::fromUTF8(", ...");

I meant to add the "return String(", ...");" remark to this line.

[Mary Wu]

(In reply to comment #8)
> (From update of attachment 119381 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119381&action=review
> 
> Thanks Mary for updating the patch. Notice that most of these these methods build a WTF::String object from a C-string. We should probably leak the WTF::String objects if these methods are called frequently instead of building a WTF::String object on each method call.
> 


yes, thanks for the comments...those are used in file buttons and context menu which we seldom used, and looks other portings didn't leak WTF::String objects either. So I think maybe we could keep it is here for now.

[Mary Wu]

Created attachment 120135 [details]
Patch

[Daniel Bates]

Comment on attachment 120135 [details]
Patch

Thanks for the updated patch.

[WebKit Review Bot]

Comment on attachment 120135 [details]
Patch

Clearing flags on attachment: 120135

Committed r103391: <http://trac.webkit.org/changeset/103391>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.