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

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


Can still be cleaned up some more.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:537
> +	   SecurityPolicy::addOriginAccessWhitelistEntry(*securityOrigin,
String("tel"), String(""), true);

All of the above cases should use emptyString instead of "".

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:591
> +    RefPtr<NodeList> nodeList = headElement->getElementsByTagName("meta");

Can you use HTMLNames::metaTag here?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:618
> +    nodeList = headElement->getElementsByTagName("link");

Can you use HTMLNames::linkTag here?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:815
> +    if (m_pluginView) {

Please do early return here.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1208
> +    Image* img = iconDatabase().synchronousIconForPageURL(url, IntSize(10,
10));

Better add early return here: if (!img || !img->data()) return;

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1209
> +    String iconUrl = iconDatabase().synchronousIconURLForPageURL(url);

This can be moved to below just before it is actually being used.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1211
> +	   NativeImageSkia* bitmap= img->nativeImageForCurrentFrame();

add space before '='

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1224
> +    RefPtr<NodeList> nodeList =
m_frame->document()->getElementsByTagName("video");

Can you use HTMLNames::videoTag here?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1227
> +    nodeList = m_frame->document()->getElementsByTagName("audio");

Can you use HTMLNames::audioTag here?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1233
> +    nodeList = m_frame->document()->getElementsByTagName("img");

Can you use HTMLNames::imgTag here?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:1247
> +    // and then when the user navigates back, it will scroll to the right
position.

I would do s/and then/Then

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:121
> +    virtual ResourceError interruptedForPolicyChangeError(const
ResourceRequest&) { notImplemented(); return ResourceError("", 0, "", ""); }

Please use emptyString for "".

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:122
> +    virtual ResourceError cancelledError(const ResourceRequest&) {
notImplemented(); return ResourceError("", 0, "", ""); }

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:123
> +    virtual ResourceError blockedError(const ResourceRequest&) {
notImplemented(); return ResourceError("", 0, "", ""); }

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:125
> +    virtual ResourceError interruptForPolicyChangeError(const
ResourceRequest&) { notImplemented(); return ResourceError("", 0, "", ""); }

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:126
> +    virtual ResourceError cannotShowMIMETypeError(const ResourceResponse&) {
notImplemented(); return ResourceError("", 0, "", ""); }

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:127
> +    virtual ResourceError fileDoesNotExistError(const ResourceResponse&) {
notImplemented(); return ResourceError("", 0, "", ""); }

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:128
> +    virtual ResourceError pluginWillHandleLoadError(const ResourceResponse&)
{ notImplemented(); return ResourceError("", 0, "", ""); }

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:154
> +    virtual PassRefPtr<Widget> createPlugin(const IntSize&,
HTMLPlugInElement*, const KURL&, const WTF::Vector<String, 0u>&, const
WTF::Vector<String, 0u>&, const String&, bool);

You probably do not need the WTF prefix here. Is the 0u param needed at all?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:156
> +    virtual PassRefPtr<Widget> createJavaAppletWidget(const IntSize&,
HTMLAppletElement*, const KURL&, const WTF::Vector<String, 0u>&, const
WTF::Vector<String, 0u>&) { notImplemented(); return 0; }

Ditto.