Bug 73275 - Upstream BlackBerry porting of WebCore/editing
Summary: Upstream BlackBerry porting of WebCore/editing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (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-28 19:23 PST by Jacky Jiang
Modified: 2011-11-30 01:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.81 KB, patch)
2011-11-28 20:01 PST, Jacky Jiang
no flags Details | Formatted Diff | Diff
Patch (3.72 KB, patch)
2011-11-29 10:19 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-11-28 19:23:37 PST
Blackberry's implementation of WebCore/editing.
Comment 1 Jacky Jiang 2011-11-28 20:01:24 PST
Created attachment 116876 [details]
Patch
Comment 2 Daniel Bates 2011-11-28 20:26:56 PST
Comment on attachment 116876 [details]
Patch

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?rev=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.
Comment 3 Jacky Jiang 2011-11-28 20:42:06 PST
(In reply to comment #2)
> (From update of attachment 116876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116876&action=review
> 
Thanks for your review Dan, will update the patch.
Comment 4 Jacky Jiang 2011-11-29 10:19:50 PST
Created attachment 116993 [details]
Patch

Update patch according to Dan's suggestions, remove parameter names of isCharacterSmartReplaceExempt just as Source/WebCore/platform/android/TemporaryLinkStubs.cpp does.
Comment 5 WebKit Review Bot 2011-11-30 01:41:51 PST
Comment on attachment 116993 [details]
Patch

Clearing flags on attachment: 116993

Committed r101465: <http://trac.webkit.org/changeset/101465>
Comment 6 WebKit Review Bot 2011-11-30 01:41:56 PST
All reviewed patches have been landed.  Closing bug.