[webkit-reviews] review denied: [Bug 85670] [BlackBerry] Implement a popup client for HTML controls : [Attachment 140351] fix style

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Crystal Zhang <haizhang at rim.com>'s
request for review:
Bug 85670: [BlackBerry] Implement a popup client for HTML controls
https://bugs.webkit.org/show_bug.cgi?id=85670

Attachment 140351: fix style
https://bugs.webkit.org/attachment.cgi?id=140351&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
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.