Bug 85907 - [BlackBerry] Enable PAGE_POPUP in make file, and implement required methods
Summary: [BlackBerry] Enable PAGE_POPUP in make file, and implement required methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-08 12:36 PDT by Crystal Zhang
Modified: 2012-05-09 09:30 PDT (History)
3 users (show)

See Also:


Attachments
patch (13.35 KB, patch)
2012-05-08 15:02 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff
Fix style issue and add more details in Changelog. (13.76 KB, patch)
2012-05-09 08:09 PDT, Crystal Zhang
rwlbuis: review-
Details | Formatted Diff | Diff
fix small issue (13.66 KB, patch)
2012-05-09 08:47 PDT, Crystal Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Crystal Zhang 2012-05-08 12:36:09 PDT
Enable PAGE_POPUP in make file, and implement required methods.
Comment 1 Crystal Zhang 2012-05-08 15:02:55 PDT
Created attachment 140793 [details]
patch
Comment 2 WebKit Review Bot 2012-05-08 15:07:13 PDT
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.
Comment 3 Yong Li 2012-05-09 07:01:07 PDT
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()?
Comment 4 Yong Li 2012-05-09 07:35:23 PDT
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.
Comment 5 Yong Li 2012-05-09 07:35:46 PDT
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
Comment 6 Crystal Zhang 2012-05-09 07:38:00 PDT
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.
Comment 7 Crystal Zhang 2012-05-09 08:09:32 PDT
Created attachment 140947 [details]
Fix style issue and add more details in Changelog.
Comment 8 Rob Buis 2012-05-09 08:32:18 PDT
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.
Comment 9 Crystal Zhang 2012-05-09 08:47:58 PDT
Created attachment 140954 [details]
fix small issue
Comment 10 Rob Buis 2012-05-09 08:53:58 PDT
Comment on attachment 140954 [details]
fix small issue

Looks good.
Comment 11 WebKit Review Bot 2012-05-09 09:30:40 PDT
Comment on attachment 140954 [details]
fix small issue

Clearing flags on attachment: 140954

Committed r116536: <http://trac.webkit.org/changeset/116536>
Comment 12 WebKit Review Bot 2012-05-09 09:30:46 PDT
All reviewed patches have been landed.  Closing bug.