[webkit-reviews] review denied: [Bug 77992] Initial upstreaming of input handling for BlackBerry port : [Attachment 125892] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 7 12:57:50 PST 2012
Rob Buis <rwlbuis at gmail.com> has denied Nima Ghanavatian
<nima.ghanavatian at gmail.com>'s request for review:
Bug 77992: Initial upstreaming of input handling for BlackBerry port
https://bugs.webkit.org/show_bug.cgi?id=77992
Attachment 125892: Patch
https://bugs.webkit.org/attachment.cgi?id=125892&action=review
------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=125892&action=review
Looks good, but still needs some cleanup.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:2
> + * Copyright (C) 2009, 2010, 2011 Research In Motion Limited. All rights
reserved.
Need to add 2012.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:176
> +
Don't need the empty line.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:218
> + return InputTypeTextArea;
This looks a bit strange since right after we always return InputTypeTextArea.
Maybe better to update the comment below and remove the if.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:368
> +
Extra empty line maybe not needed.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:371
> + textInField = textInField.substring(std::max(0,
(int)textInField.length() - MaxLearnTextDataSize), textInField.length());
can use static_cast<int> here.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:388
> +
Why the empty line?
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:583
> + WebCore::IntRect actualScreenRect =
WebCore::IntRect(mainFrameView->scrollPosition(),
m_webPage->actualVisibleSize());
Can be moved into the if section.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:1154
> + bool* selecteds = 0;
Strange name again.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:1405
> + extractedText->text = spannableTextInRange(0, elementText().length(),
flags);
Too many empty lines IMHO.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:1501
> + // Remove all markers.
Maybe not needed.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:1677
> + // the start of the inserted texted.
texted?
> Source/WebKit/blackberry/WebKitSupport/InputHandler.h:2
> + * Copyright (C) 2009, 2010, 2011 Research In Motion Limited. All rights
reserved.
Need to add 2012
> Source/WebKit/blackberry/WebKitSupport/InputHandler.h:41
> +class VisibleSelection;
I dont think you need the above two.
> Source/WebKit/blackberry/WebKitSupport/InputHandler.h:96
> + void setPopupListIndexes(int size, bool* selecteds);
Not so nice param name selecteds
More information about the webkit-reviews
mailing list