[webkit-reviews] review denied: [Bug 78275] [BlackBerry] Upstream BlackBerry WebCoreSupport FrameLoaderClientBlackBerry class : [Attachment 126996] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Jacky Jiang <zkjiang008 at gmail.com>'s
request for review:
Bug 78275: [BlackBerry] Upstream BlackBerry WebCoreSupport
FrameLoaderClientBlackBerry class
https://bugs.webkit.org/show_bug.cgi?id=78275

Attachment 126996: Patch
https://bugs.webkit.org/attachment.cgi?id=126996&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=126996&action=review


Almost there, needs a bit more cleaning up.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:72
> +using namespace BlackBerry::WebKit;

This means that below you can remove BlackBerry::WebKit usage. So
BlackBerry::WebKit::WebSettings becomes just WebSettings for example.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:403
> +    // In Frame::createView, Frame's FrameView object is set to 0 and 
recreated.

one space too much between 'and' and 'recreated'.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:427
> +			   true);				  /*lock the
mode*/

It is a bit cleaner to do this style /* lock the mode */ everywhere here.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:468
> +

Remove one empty line here.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:616
> +

Remove on empty line.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:691
> +

Remove empty line here.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:26
> +#include "Widget.h"

This include can probably be removed. Please check whether we need
DocumentLoader and Frame includes.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:38
> +class HTMLFormElement;

These can be removed since FrameLoaderClient does this as well.