[webkit-reviews] review denied: [Bug 77843] [BlackBerry] Upstream ChromeClientBlackBerry.{h, cpp} : [Attachment 125767] Patch v2

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied  review:
Bug 77843: [BlackBerry] Upstream ChromeClientBlackBerry.{h, cpp}
https://bugs.webkit.org/show_bug.cgi?id=77843

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

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


r- since I think it can be cleaned up a lot.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:89
> +	   m_webPagePrivate->m_dumpRenderTree->addMessageToConsole(message,
lineNumber, sourceID);

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:108
> +	   return
m_webPagePrivate->m_dumpRenderTree->runJavaScriptConfirm(message);

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:113
> +

Why empty line?

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:118
> +    WebString clientResult;

This can be removed to where it is actually used.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:123
> +    }

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:142
> +    return; // The window dimensions are fixed in the RIM port.

Why return; ?

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:159
> +    return 1.0f;

Should be just 1.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:279
> +    }

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:311
> +	   m_webPagePrivate->m_dumpRenderTree->windowCreated(webPage);

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:411
> +	   return
m_webPagePrivate->m_dumpRenderTree->runBeforeUnloadConfirmPanel(message);

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:426
> +	   m_webPagePrivate->m_dumpRenderTree->setStatusText(status);

We need #if ENABLE_DRT here.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:539
> +void ChromeClientBlackBerry::runOpenPanel(WebCore::Frame*,
PassRefPtr<WebCore::FileChooser> chooser)

Do we really need WebCore:: prefix everywhere?

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:607
> +	   m_webPagePrivate->m_backingStore->d->repaint(updateRect, true
/*contentChanged*/, true /*immediate*/);

Could call invalidateContentsAndWindow to indicate what we are actually doing.