Bug 116448 – [BlackBerry] Make PagePopup implementation independent from WebCore

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

[Carlos Garcia Campos]

PAGE_POPUP feature will be removed and the whole implementation in BlackBerry can be made independent from WebCore.

[Carlos Garcia Campos]

Created attachment 202295 [details]
Patch

The pickers are now in the  BB::WebKit namespace instead of WebCore, maybe we could move all those files (pickers, pagePopup and pagePopupClient) to WebKitSupport, but I've left them for now in WebCoreSupport to make it easier to review. If you think this should be in WebKitSupport I'll move them in a follow up patch.

[Anders Carlsson]

Comment on attachment 202295 [details]
Patch

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

> Source/WebKit/blackberry/ChangeLog:14
> +        (BlackBerry::WebKit::WebPagePrivate::~WebPagePrivate): Destriy the

Typo, “Destriy"

> Source/WebKit/blackberry/ChangeLog:22
> +        (BlackBerry::WebKit::WebPagePrivate::hasOpenedPopup): Whether

Return whether?

[Arvid Nilsson]

Comment on attachment 202295 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:468
> +    // Destroy popup if we have.

If we have one? I think this is self-explanatory, and doesn't need any comment.

> Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:-153
> -    // Destroy popup if we have.

Now I see where you got that comment from =)

> Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerryClient.h:36
> +class PagePopupBlackBerryClient {

The "BlackBerry" in the name seems redundant, since we're in the BlackBerry::WebKit namespace. However, perhaps you can rename the classes to just PagePopup/PagePopupClient once the cross platform ones have been removed.

> Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerryClient.h:44
> +    //  - window.setValueAndClosePopup(number, string).

Does it really support this, if we dont define ENABLE(PAGE_POPUP)? Don't you need to inject some native function into JavaScript that does this?

[Arvid Nilsson]

Comment on attachment 202295 [details]
Patch

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

>> Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerryClient.h:44
>> +    //  - window.setValueAndClosePopup(number, string).
> 
> Does it really support this, if we dont define ENABLE(PAGE_POPUP)? Don't you need to inject some native function into JavaScript that does this?

I found it, PagePopupBlackBerry::installDOMFunction(Frame* frame)

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 202295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202295&action=review
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:468
> > +    // Destroy popup if we have.
> 
> If we have one? I think this is self-explanatory, and doesn't need any comment.
> 
> > Source/WebKit/blackberry/WebCoreSupport/ChromeClientBlackBerry.cpp:-153
> > -    // Destroy popup if we have.
> 
> Now I see where you got that comment from =)

:-) I'll remove it anyway.

> > Source/WebKit/blackberry/WebCoreSupport/PagePopupBlackBerryClient.h:36
> > +class PagePopupBlackBerryClient {
> 
> The "BlackBerry" in the name seems redundant, since we're in the BlackBerry::WebKit namespace. However, perhaps you can rename the classes to just PagePopup/PagePopupClient once the cross platform ones have been removed.

Exactly, that was my idea. At the moment we would need to use a different #ifndef #define block for the PagePopupClient header if we use the same name. I think it will be easier to rename it when we move the files to WebKitSupport, making this patch as simple as possible.

[Carlos Garcia Campos]

Created attachment 202401 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 202401 [details]:

svg/batik/text/smallFonts.svg bug 115040 (author: [e-mail])
svg/batik/text/textEffect2.svg bug 116286 (authors: [e-mail] and [e-mail])
svg/batik/paints/patternRegions.svg bug 116380 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 202401 [details]
Patch for landing

Clearing flags on attachment: 202401

Committed r150434: <http://trac.webkit.org/changeset/150434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.