[Webkit-unassigned] [Bug 85670] [BlackBerry] Implement a popup client for HTML controls
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun May 6 16:40:28 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=85670
Rob Buis <rwlbuis at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #140351|review? |review-
Flag| |
--- Comment #4 from Rob Buis <rwlbuis at gmail.com> 2012-05-06 16:40:28 PST ---
(From update of attachment 140351)
View in context: https://bugs.webkit.org/attachment.cgi?id=140351&action=review
Can be cleaned up some more.
> Source/WebKit/ChangeLog:7
> +
It is best to add at least a short description.
> Source/WebKit/PlatformBlackBerry.cmake:6
> +
Better not add empty lines here.
> Source/WebKit/PlatformBlackBerry.cmake:81
> +
Ditto.
> Source/WebKit/blackberry/ChangeLog:6
> + Reviewed by NOBODY (OOPS!).
It is best to add at least a short description.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:43
> +#include "WindowFeatures.h"
Are all includes needed?
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:48
> +using namespace BlackBerry::WebKit;
Add a newline here.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:51
> +class PagePopupChromeClient: public ChromeClientBlackBerry {
please add a space character before the ':'
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:56
> + 0, 0, 0, 0)
Check if you need to initialize m_rect that way, it may default to 0,0,0,0 anyway.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:81
> +
Remove empty line.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:97
> + new EmptyFrameLoaderClient;
I don't think you have to wrap here, can be on one line.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:105
> + new EmptyContextMenuClient;
Ditto.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:122
> + emptyFrameLoaderClient);
Ditto.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:129
> + frame->loader()->activeDocumentLoader()->writer();
Ditto.
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.cpp:138
> + return false;
Better do early return style:
if (!webpage)
return false;
.....
return true;
> Source/WebKit/blackberry/WebCoreSupport/WebPopupImpl.h:31
> +#include <JavaScriptCore/JSValueRef.h>
Please make sure to only include the ones you need, for example JSC includes can go to the .cpp.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list