Bug 73534 – upstream BlackBerry porting of MIMETypeRegistry/KeyboardEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mary Wu]

add two files MIMETypeRegistryBlackBerry.cpp and PlatformKeyboardEventBlackBerry.cpp to WebCore/platform/blackberry

[Mary Wu]

Created attachment 117370 [details]
Patch

[Leo Yang]

Please capitalise the first character of the bug title.

[Mary Wu]

(In reply to comment #2)
> Please capitalise the first character of the bug title.

ok, will update...

[Mary Wu]

Created attachment 117827 [details]
Patch

[Antonio Gomes]

Comment on attachment 117827 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        Initial upstream, no new tests.
> +
> +
> +        * platform/blackberry/MIMETypeRegistryBlackBerry.cpp: Added.

extra new line.

> Source/WebCore/platform/blackberry/MIMETypeRegistryBlackBerry.cpp:135
> +bool MIMETypeRegistry::isApplicationPluginMIMEType(const String& mimeType)

Either do

bool MIMETypeRegistry::isApplicationPluginMIMEType(const String& /*mimeType*/)

or add a UNUSED_PARAM(mimeType) line.

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:2
> + * Copyright (C) 2009, 2010 Research In Motion Limited. All rights reserved.

2011?

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:485
> +void PlatformKeyboardEvent::getCurrentModifierState(bool& shiftKey, bool& ctrlKey, bool& altKey, bool& metaKey)

UNNEEDED_PARAM(xxx)

[Mary Wu]

Created attachment 117836 [details]
Patch

[Leo Yang]

Comment on attachment 117836 [details]
Patch

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

Informal review.

> Source/WebCore/platform/blackberry/MIMETypeRegistryBlackBerry.cpp:107
> +String MIMETypeRegistry::getMIMETypeForExtension(const String &ext)
> +{
> +    String s = ext.lower();
> +

ext--> extension?
s--> lowerExtension?

> Source/WebCore/platform/blackberry/MIMETypeRegistryBlackBerry.cpp:113
> +    const ExtensionMap* e = extensionMap;
> +    while (e->extension) {
> +        if (s == e->extension)
> +            return e->mimeType;
> +        ++e;
> +    }

e --> extensionMap?

> Source/WebCore/platform/blackberry/MIMETypeRegistryBlackBerry.cpp:123
> +    String s = type.lower();

s --> lowerMIMEType?

> Source/WebCore/platform/blackberry/MIMETypeRegistryBlackBerry.cpp:130
> +    const ExtensionMap* e = extensionMap;
> +    while (e->mimeType) {
> +        if (s == e->mimeType)
> +            return e->extension;
> +        ++e;
> +    }

e --> extensionMap?

> Source/WebCore/platform/blackberry/MIMETypeRegistryBlackBerry.cpp:136
> +bool MIMETypeRegistry::isApplicationPluginMIMEType(const String& /*mimeType*/)
> +{

remove the comment

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:22
> +#include "BlackBerryPlatformLog.h"

Use angle brackets for BlackBerryPlatformLog.h and move it down after #include <BlackBerryPlatformKeyboardEvent.h>

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:427
> +static inline WebCore::PlatformKeyboardEvent::Type toWebCorePlatformKeyboardEventType(const BlackBerry::Platform::KeyboardEvent::Type type)
> +{

Is namespace WebCore necessary here?

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:435
> +    default:
> +        return WebCore::PlatformKeyboardEvent::Char;

Ditto.

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:457
> +    m_type = toWebCorePlatformKeyboardEventType(event.type());
> +    m_unmodifiedCharacter = event.character();
> +    m_keyIdentifier = keyIdentifierForBlackBerryCharacter(event.character());
> +    m_windowsVirtualKeyCode = windowsKeyCodeForBlackBerryCharacter(event.character());
> +    unsigned short character = adjustCharacterFromOS(event.character());
> +    m_text = String(&character, 1);
> +    m_unmodifiedText = m_text;
> +
> +    if (event.character() == KEYCODE_BACK_TAB)
> +        m_shiftKey = true; // BackTab should be treated as Shift + Tab.
> +

Why can't all these data members be initialised in the initialisation list of the constructor?

[Mary Wu]

accept most part, thanks for your comments, Leo

[Mary Wu]

Created attachment 118348 [details]
Patch

[Rob Buis]

Comment on attachment 118348 [details]
Patch

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

Almost done, but since this needs cq+ the nit needs to be fixed first.

> Source/WebCore/platform/blackberry/PlatformKeyboardEventBlackBerry.cpp:482
> +void PlatformKeyboardEvent::getCurrentModifierState(bool& shiftKey, bool& ctrlKey, bool& altKey, bool& metaKey)

The parameter names can be best removed or marked as unused.

[Mary Wu]

Created attachment 118516 [details]
Patch

[Rob Buis]

Comment on attachment 118516 [details]
Patch

Looks good!

[WebKit Review Bot]

Comment on attachment 118516 [details]
Patch

Clearing flags on attachment: 118516

Committed r102426: <http://trac.webkit.org/changeset/102426>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.