Bug 116448

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.

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?

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?

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)

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

Created attachment 202401 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 202401 [details]: svg/batik/text/smallFonts.svg bug 115040 (author: oliver@apple.com) svg/batik/text/textEffect2.svg bug 116286 (authors: darin@apple.com and zimmermann@kde.org) svg/batik/paints/patternRegions.svg bug 116380 (author: zimmermann@kde.org) The commit-queue is continuing to process your patch.

Comment on attachment 202401 [details] Patch for landing Clearing flags on attachment: 202401 Committed r150434: <http://trac.webkit.org/changeset/150434>

All reviewed patches have been landed. Closing bug.