Summary: | Upstream BlackBerry porting of WebCore/editing | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jacky Jiang <jkjiang> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dbates, rwlbuis, staikos, tonikitoo, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 73144 | ||||||||
Attachments: |
|
Description
Jacky Jiang
2011-11-28 19:23:37 PST
Created attachment 116876 [details]
Patch
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. (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. 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 on attachment 116993 [details] Patch Clearing flags on attachment: 116993 Committed r101465: <http://trac.webkit.org/changeset/101465> All reviewed patches have been landed. Closing bug. |