I would like to add a new API named setAllowNotification in webpage to let client set those allowed domains in notifications.
Created attachment 147240 [details] Patch
Created attachment 147257 [details] Patch
Comment on attachment 147257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review It looks good. I wonder if it fits better to WebSettings instead of WebPage. What you you think? > Source/WebKit/blackberry/Api/WebPage.cpp:6346 > +void WebPage::setAllowNotification(const WebString& domain, bool allow) I think you need UNUSED_PARAM for 'domain' and 'allow' in an #else block
Comment on attachment 147257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review > Source/WebKit/blackberry/ChangeLog:9 > + Add a new API named setAllowNotification in webpage class to > + let client set those allowed domains into notifications. could you tell more about the use cases?
(In reply to comment #4) > (From update of attachment 147257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review > > > Source/WebKit/blackberry/ChangeLog:9 > > + Add a new API named setAllowNotification in webpage class to > > + let client set those allowed domains into notifications. > > could you tell more about the use cases? Antonio, Thanks, please see PR #141125 for more.:)
Comment on attachment 147257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:6349 > + static_cast<NotificationPresenterImpl*>(NotificationPresenterImpl::instance())->onPermission(domain.utf8(), allow); Since this is a singleton, is it a problem that a new page will "inherit" this? > Source/WebKit/blackberry/Api/WebPage.h:269 > + void setAllowNotification(const WebString& domain, bool allow); Would it make sense for the whole method to be wrapped in #if ENABLE(NOTIFICATIONS) ?
(In reply to comment #3) > (From update of attachment 147257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review > > It looks good. I wonder if it fits better to WebSettings instead of WebPage. What you you think? > > > Source/WebKit/blackberry/Api/WebPage.cpp:6346 > > +void WebPage::setAllowNotification(const WebString& domain, bool allow) > > I think you need UNUSED_PARAM for 'domain' and 'allow' in an #else block yes, UNUSED_PARAM is good, :) I am thinking refactor notification, because upstream code has changed a lot since we started implementing notification, I feel notificationClient is a better candidate class by which QT port is using.
(In reply to comment #6) > (From update of attachment 147257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review > > > Source/WebKit/blackberry/Api/WebPage.cpp:6349 > > + static_cast<NotificationPresenterImpl*>(NotificationPresenterImpl::instance())->onPermission(domain.utf8(), allow); > > Since this is a singleton, is it a problem that a new page will "inherit" this? I am thinking refactor our notification implementation. > > > Source/WebKit/blackberry/Api/WebPage.h:269 > > + void setAllowNotification(const WebString& domain, bool allow); > > Would it make sense for the whole method to be wrapped in #if ENABLE(NOTIFICATIONS) ? Both are okay to me, I was searching other ports, I remember that I kept the same with it, Mac or QT probably(not sure) :)
(In reply to comment #4) > (From update of attachment 147257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147257&action=review > > > Source/WebKit/blackberry/ChangeLog:9 > > + Add a new API named setAllowNotification in webpage class to > > + let client set those allowed domains into notifications. > > could you tell more about the use cases? this is very similar with Geoloaction.
Comment on attachment 147257 [details] Patch I will make a new patch.
Created attachment 147468 [details] Patch
Comment on attachment 147468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147468&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:6358 > +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > +void WebPage::setAllowNotification(const WebString& domain, bool allow) > +{ > + static_cast<NotificationPresenterImpl*>(NotificationPresenterImpl::instance())->onPermission(domain.utf8(), allow); > +} > +#endif I am not sure I like the whole method being wrapped by #if. If you disable it, how will the client side build? the #if macro is not visible outside webkit. > Source/WebKit/blackberry/Api/WebPage.h:270 > > + void setAllowNotification(const WebString& domain, bool allow); > + you did not wrap the method in the header.
Created attachment 147469 [details] Patch
(In reply to comment #12) > (From update of attachment 147468 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147468&action=review > > > Source/WebKit/blackberry/Api/WebPage.cpp:6358 > > +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > > +void WebPage::setAllowNotification(const WebString& domain, bool allow) > > +{ > > + static_cast<NotificationPresenterImpl*>(NotificationPresenterImpl::instance())->onPermission(domain.utf8(), allow); > > +} > > +#endif > > I am not sure I like the whole method being wrapped by #if. If you disable it, how will the client side build? the #if macro is not visible outside webkit. > > > Source/WebKit/blackberry/Api/WebPage.h:270 > > > > + void setAllowNotification(const WebString& domain, bool allow); > > + > > you did not wrap the method in the header. we can not wrap this method in header file, because libwebview build fails because it does not know the macro of "ENABLE".
Comment on attachment 147469 [details] Patch Clearing flags on attachment: 147469 Committed r120300: <http://trac.webkit.org/changeset/120300>
All reviewed patches have been landed. Closing bug.