Summary: | [BlackBerry] Upstream ChromeClientBlackBerry.{h, cpp} | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Leo Yang <leo.yang> | ||||||||||
Component: | Platform | Assignee: | Leo Yang <leo.yang> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, charles.wei, japhet, rwlbuis, staikos, webkit.review.bot, zimmermann | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 73144 | ||||||||||||
Attachments: |
|
Description
Leo Yang
2012-02-05 19:06:01 PST
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> |