Bug 77843 - [BlackBerry] Upstream ChromeClientBlackBerry.{h, cpp}
Summary: [BlackBerry] Upstream ChromeClientBlackBerry.{h, cpp}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leo Yang
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2012-02-05 19:06 PST by Leo Yang
Modified: 2012-02-07 20:05 PST (History)
7 users (show)

See Also:


Attachments
Patch (35.39 KB, patch)
2012-02-05 19:19 PST, Leo Yang
tonikitoo: review-
Details | Formatted Diff | Diff
Patch v2 (35.91 KB, patch)
2012-02-06 22:02 PST, Leo Yang
rwlbuis: review-
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (35.87 KB, patch)
2012-02-07 18:25 PST, Leo Yang
rwlbuis: review+
Details | Formatted Diff | Diff
Patch v4 (35.87 KB, patch)
2012-02-07 19:02 PST, Leo Yang
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Yang 2012-02-05 19:06:01 PST
This is to upstream the BlackBerry implementation of ChromeClient.
Comment 1 Leo Yang 2012-02-05 19:19:15 PST
Created attachment 125552 [details]
Patch
Comment 2 Antonio Gomes 2012-02-05 19:59:55 PST
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
Comment 3 Leo Yang 2012-02-06 22:02:30 PST
Created attachment 125767 [details]
Patch v2
Comment 4 Antonio Gomes 2012-02-07 06:39:14 PST
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 5 Rob Buis 2012-02-07 07:06:21 PST
Comment on attachment 125767 [details]
Patch v2

I still see some issues, please don't land just yet.
Comment 6 Rob Buis 2012-02-07 07:25:14 PST
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.
Comment 7 Leo Yang 2012-02-07 18:25:16 PST
Created attachment 125976 [details]
Patch v3
Comment 8 Leo Yang 2012-02-07 18:48:42 PST
(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 9 Rob Buis 2012-02-07 18:49:21 PST
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.
Comment 10 Leo Yang 2012-02-07 19:02:17 PST
Created attachment 125985 [details]
Patch v4
Comment 11 Rob Buis 2012-02-07 19:12:13 PST
Comment on attachment 125985 [details]
Patch v4

Looks great!
Comment 12 Leo Yang 2012-02-07 20:05:46 PST
Committed r107033: <http://trac.webkit.org/changeset/107033>