Bug 73534 - upstream BlackBerry porting of MIMETypeRegistry/KeyboardEvent
Summary: upstream BlackBerry porting of MIMETypeRegistry/KeyboardEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (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-11-30 23:58 PST by Mary Wu
Modified: 2011-12-08 20:38 PST (History)
5 users (show)

See Also:


Attachments
Patch (18.84 KB, patch)
2011-12-01 00:35 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (19.13 KB, patch)
2011-12-04 20:08 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2011-12-04 21:14 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (19.10 KB, patch)
2011-12-08 02:01 PST, Mary Wu
no flags Details | Formatted Diff | Diff
Patch (19.07 KB, patch)
2011-12-08 19:16 PST, Mary Wu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mary Wu 2011-11-30 23:58:44 PST
add two files MIMETypeRegistryBlackBerry.cpp and PlatformKeyboardEventBlackBerry.cpp to WebCore/platform/blackberry
Comment 1 Mary Wu 2011-12-01 00:35:56 PST
Created attachment 117370 [details]
Patch
Comment 2 Leo Yang 2011-12-01 02:03:57 PST
Please capitalise the first character of the bug title.
Comment 3 Mary Wu 2011-12-04 20:02:10 PST
(In reply to comment #2)
> Please capitalise the first character of the bug title.

ok, will update...
Comment 4 Mary Wu 2011-12-04 20:08:55 PST
Created attachment 117827 [details]
Patch
Comment 5 Antonio Gomes 2011-12-04 20:38:26 PST
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)
Comment 6 Mary Wu 2011-12-04 21:14:17 PST
Created attachment 117836 [details]
Patch
Comment 7 Leo Yang 2011-12-07 22:10:24 PST
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?
Comment 8 Mary Wu 2011-12-08 01:50:32 PST
accept most part, thanks for your comments, Leo
Comment 9 Mary Wu 2011-12-08 02:01:17 PST
Created attachment 118348 [details]
Patch
Comment 10 Rob Buis 2011-12-08 07:15:28 PST
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.
Comment 11 Mary Wu 2011-12-08 19:16:52 PST
Created attachment 118516 [details]
Patch
Comment 12 Rob Buis 2011-12-08 19:35:57 PST
Comment on attachment 118516 [details]
Patch

Looks good!
Comment 13 WebKit Review Bot 2011-12-08 20:37:56 PST
Comment on attachment 118516 [details]
Patch

Clearing flags on attachment: 118516

Committed r102426: <http://trac.webkit.org/changeset/102426>
Comment 14 WebKit Review Bot 2011-12-08 20:38:01 PST
All reviewed patches have been landed.  Closing bug.