[BlackBerry] Upstream ChromeClientBlackBerry.{h, cpp}

[Leo Yang]

This is to upstream the BlackBerry implementation of ChromeClient.

[Leo Yang]

Created attachment 125552 [details]
Patch

[Antonio Gomes]

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

[Leo Yang]

Created attachment 125767 [details]
Patch v2

[Antonio Gomes]

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?

[Rob Buis]

Comment on attachment 125767 [details]
Patch v2

I still see some issues, please don't land just yet.

[Rob Buis]

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.

[Leo Yang]

Created attachment 125976 [details]
Patch v3

[Leo Yang]

(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.

[Rob Buis]

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.

[Leo Yang]

Created attachment 125985 [details]
Patch v4

[Rob Buis]

Comment on attachment 125985 [details]
Patch v4

Looks great!

[Leo Yang]

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