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

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


Looks good, but I think it can be cleaned up some more.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:68
> +#include <SkBitmap.h>

You probably do not need this.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:72
> +using WTF::String;

I think this means that below you dont have to do WTF::String, but just String.


> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:180
> +void
FrameLoaderClientBlackBerry::dispatchDecidePolicyForResponse(FramePolicyFunctio
n function, const WebCore::ResourceResponse& response, const
WebCore::ResourceRequest& request)

Remove WebCore prefix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:185
> +    if
(WebCore::contentDispositionType(response.httpHeaderField("Content-Disposition"
)) == WebCore::ContentDispositionAttachment

Remove WebCore prefix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:211
> +    // Let the client have a chance to say whether this navigation should
take

take?

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

No need for empty line

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:491
> +void
FrameLoaderClientBlackBerry::dispatchDidReceiveResponse(WebCore::DocumentLoader
*, unsigned long identifier, const WebCore::ResourceResponse& response)

Remove WebCore prefix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:723
> +	   if (WebCore::isBackForwardLoadType(m_frame->loader()->loadType())) {


Remove WebCore prefix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:804
> +	   return WebCore::ObjectContentOtherPlugin;

Remove WebCore prefix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:918
> +    BackForwardListImpl* backForwardList =
static_cast<WebCore::BackForwardListImpl*>(m_webPagePrivate->m_page->backForwar
d()->client());

Remove WebCore prefix.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:997
> +    if (WebCore::isBackForwardLoadType(loader->loadType())) {

Remove WebCore prefix.

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

Remove WebCore prefix.

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

Remove WebCore prefix.

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

Can be a forward reference. For all of the above please check if you can do the
same.

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:33
> +class WebPluginClient;

Is this one needed?

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.h:153
> +    virtual WTF::PassRefPtr<Frame> createFrame(const KURL&, const String&,
HTMLFrameOwnerElement*, const String&, bool, int, int);

You can probably remove WTF:: from PassRefPtr everywhere in the header.