[webkit-reviews] review granted: [Bug 73275] Upstream BlackBerry porting of WebCore/editing : [Attachment 116876] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jacky Jiang
<zkjiang008 at gmail.com>'s request for review:
Bug 73275: Upstream BlackBerry porting of WebCore/editing
https://bugs.webkit.org/show_bug.cgi?id=73275

Attachment 116876: Patch
https://bugs.webkit.org/attachment.cgi?id=116876&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116876&action=review


> Source/WebCore/editing/blackberry/EditorBlackBerry.cpp:23
> +#include "ClipboardAccessPolicy.h"

This include is unnecessary since ClipboardBlackBerry.h (below) includes
Clipboard.h, which includes this file.

> Source/WebCore/editing/blackberry/EditorBlackBerry.cpp:25
> +#include "NotImplemented.h"

Remove this include since we aren't using its functionality in this file.

> Source/WebCore/editing/blackberry/EditorBlackBerry.cpp:26
> +#include "PassRefPtr.h"

This should be #include <wtf/PassRefPtr.h>.

> Source/WebCore/editing/blackberry/EditorBlackBerry.cpp:30
> +WTF::PassRefPtr<Clipboard> Editor::newGeneralClipboard(ClipboardAccessPolicy
policy, Frame*)

Do we need the WTF:: prefix?

> Source/WebCore/editing/blackberry/SmartReplaceBlackBerry.cpp:26
> +bool isCharacterSmartReplaceExempt(UChar32 c, bool isPreviousCharacter)

Neither of these parameters are used in this function. If we want to keep the
names of these parameters, don't we need to use UNUSED_PARAM() (*) here so as
to avoid compiler warnings? The name of the first parameter doesn't seem very
meaningful so I suggest omitting its name. You may want to consider using
UNUSED_PARAM(isPreviousCharacter) so as to keep the parameter name
isPreviousCharacter even though we don't use it.

(*) Defined in JavaScriptCore/wtf/UnusedParam.h,
<http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/UnusedParam.h?r
ev=74855#L32>.

> Source/WebCore/editing/blackberry/SmartReplaceBlackBerry.cpp:32
> +}

Nit: This is a short file. It's convention to put an inline comment here, //
namespace WebCore, to demarcate the end of the WebCore namespace.