Created attachment 125552 [details] Patch

Comment on attachment 125552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125552&action=review the change I will request is easy but should be addressed internally first, and then we generate this patch again for upstreaming. r- due to that. > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:82 > +ChromeClientBlackBerry::ChromeClientBlackBerry(WebPage* page) > + : m_webPage(page) should awebpageprivate, not the public class

Created attachment 125767 [details] Patch v2

Comment on attachment 125767 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=125767&action=review > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:476 > +#if ENABLE(EVENT_MODE_METATAGS) > +void ChromeClientBlackBerry::didReceiveCursorEventMode(Frame* frame, CursorEventMode mode) const > +{ > + if (m_webPagePrivate->m_mainFrame != frame) > + return; > + > + m_webPagePrivate->didReceiveCursorEventMode(mode); > +} > + > +void ChromeClientBlackBerry::didReceiveTouchEventMode(Frame* frame, TouchEventMode mode) const > +{ > + if (m_webPagePrivate->m_mainFrame != frame) > + return; > + > + m_webPagePrivate->didReceiveTouchEventMode(mode); > +} > +#endif the WebCore counter part of these methods are intended to be upstreamed? > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:604 > +void ChromeClientBlackBerry::invalidateContentsForSlowScroll(const IntSize& delta, const IntRect& updateRect, bool immediate, const ScrollView* scrollView) it also requires changes in scrollview I am not sure we will upstream. > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h:2 > + * Copyright (C) 2009, 2010, 2011 Research In Motion Limited. All rights reserved. 2012?

Comment on attachment 125767 [details] Patch v2 I still see some issues, please don't land just yet.

Comment on attachment 125767 [details] Patch v2 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.

Created attachment 125976 [details] Patch v3

(In reply to comment #4) > (From update of attachment 125767 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125767&action=review > > > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:476 > > +#if ENABLE(EVENT_MODE_METATAGS) > > +void ChromeClientBlackBerry::didReceiveCursorEventMode(Frame* frame, CursorEventMode mode) const > > +{ > > + if (m_webPagePrivate->m_mainFrame != frame) > > + return; > > + > > + m_webPagePrivate->didReceiveCursorEventMode(mode); > > +} > > + > > +void ChromeClientBlackBerry::didReceiveTouchEventMode(Frame* frame, TouchEventMode mode) const > > +{ > > + if (m_webPagePrivate->m_mainFrame != frame) > > + return; > > + > > + m_webPagePrivate->didReceiveTouchEventMode(mode); > > +} > > +#endif > > the WebCore counter part of these methods are intended to be upstreamed? We've enabled EVENT_MODE_METATAGS. I think we should upstream the counterpart of WebCore because otherwise we will be divergent with the upstream. > > > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:604 > > +void ChromeClientBlackBerry::invalidateContentsForSlowScroll(const IntSize& delta, const IntRect& updateRect, bool immediate, const ScrollView* scrollView) > > it also requires changes in scrollview I am not sure we will upstream. I think we can change the last parameter to a bool which indicate if this is requested by the main frame view, saying bool isMainFrameView. And then we should upstream the changed API, at least guard it using PLATFORM(BLACKBERRY) > > > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h:2 > > + * Copyright (C) 2009, 2010, 2011 Research In Motion Limited. All rights reserved. > > 2012? Yes.

Comment on attachment 125976 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=125976&action=review Looks good, please have a look at fixing the nits before landing. > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h:2 > + * Copyright (C) 2009, 2010, 2011 Research In Motion Limited. All rights reserved. Still needs 2012. > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.h:95 > + virtual void runOpenPanel(Frame*, WTF::PassRefPtr<FileChooser>); WTF is probably not needed, ChromeClient.h does not need it either.

Created attachment 125985 [details] Patch v4

Comment on attachment 125985 [details] Patch v4 Looks great!

Committed r107033: <http://trac.webkit.org/changeset/107033>