[webkit-reviews] review denied: [Bug 78275] [BlackBerry] Upstream BlackBerry WebCoreSupport FrameLoaderClientBlackBerry class : [Attachment 126818] 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 126818: Patch
https://bugs.webkit.org/attachment.cgi?id=126818&action=review

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


Looks good, still a few things to fix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:52
> +#include "NotImplemented.h"

Not needed, included in header.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:56
> +#include "RenderEmbeddedObject.h"

You don't need this one. Please check all other includes as well.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:329
> +	   return pluginView;

No need for temp var pluginView.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:375
> +	   // received, even for the case of a document with no data (like
about:blank)

Add a period to the end of line to make it a proper sentence.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:386
> +    // (unless it already has a token, in which case the request came from
the client)

Ditto.

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

Might as well try to align the comments.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:523
> +    // Whitelist all known protocols if BrowserField2 requires unrestricted
requests.

What is BrowserField2?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:553
> +	   // SubstituteData in dispatchDidFailProvisionalLoad)

Add a period to the end of line to make it a proper sentence.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:887
> +    // Provide the extension object first in case the client or others want
to use it

Add a period.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:23
> +#include "FormState.h"

Maybe you don't need FormState include.