Bug 85907 – [BlackBerry] Enable PAGE_POPUP in make file, and implement required methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Crystal Zhang]

Enable PAGE_POPUP in make file, and implement required methods.

[Crystal Zhang]

Created attachment 140793 [details]
patch

[WebKit Review Bot]

Attachment 140793 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/blackberry/Api..." exit_code: 1
Source/WebKit/blackberry/Api/WebPage.cpp:6106:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

Comment on attachment 140793 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140793&action=review

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:314
> +    if (!hasOpenedPopup()) {
> +        webPopup = new PagePopupBlackBerry(m_webPagePrivate, client,
> +                rootViewToScreen(originBoundsInRootView));
> +        m_webPagePrivate->m_webPage->popupOpened(webPopup);
> +        webPopup->sendCreatePopupWebViewRequest();
> +    } else {
> +        webPopup = m_webPagePrivate->m_webPage->popup();
> +        webPopup->closeWebPage();
> +        webPopup->sendCreatePopupWebViewRequest();
> +    }

They can share one webPopup->sendCreatePopupWebViewRequest()?

[Yong Li]

Comment on attachment 140793 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140793&action=review

> ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Need more description after this line.

[Yong Li]

Comment on attachment 140793 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140793&action=review

> Source/WebKit/blackberry/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Need detailed information here

[Crystal Zhang]

Comment on attachment 140793 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140793&action=review

>> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:314
>> +    }
> 
> They can share one webPopup->sendCreatePopupWebViewRequest()?

yes, it will check if it already exists on client side.

[Crystal Zhang]

Created attachment 140947 [details]
Fix style issue and add more details in Changelog.

[Rob Buis]

Comment on attachment 140947 [details]
Fix style issue and add more details in Changelog.

View in context: https://bugs.webkit.org/attachment.cgi?id=140947&action=review

Looks good, can be improved some more.

> Source/WebKit/blackberry/Api/WebPage.cpp:378
> +

You don't need the empty line.

> Source/WebKit/blackberry/Api/WebPage.cpp:6106
> +    return (d->m_selectPopup) ? false : true;

It does not need to be this explicit, just say return d->m_selectPopup;

> Source/WebKit/blackberry/Api/WebPage.h:341
> +    WebCore::PagePopupBlackBerry* popup();

Can some be const, like hasOpenedPopup?

> Source/WebKit/blackberry/Api/WebPage_p.h:544
> +

No need for empty line.

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

You can move the webPopup->sendCreatePopupWebViewRequest(); usage to here so you state it once.

[Crystal Zhang]

Created attachment 140954 [details]
fix small issue

[Rob Buis]

Comment on attachment 140954 [details]
fix small issue

Looks good.

[WebKit Review Bot]

Comment on attachment 140954 [details]
fix small issue

Clearing flags on attachment: 140954

Committed r116536: <http://trac.webkit.org/changeset/116536>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.