Bug 88950 – [Blackberry] add a new Api named setAllowNotification

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Chris.Guan]

I would like to add a new API named setAllowNotification in webpage to let client set those allowed domains in notifications.

[Chris.Guan]

Created attachment 147240 [details]
Patch

[Chris.Guan]

Created attachment 147257 [details]
Patch

[Antonio Gomes]

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

[Antonio Gomes]

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?

[Chris.Guan]

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

[Rob Buis]

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) ?

[Chris.Guan]

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

[Chris.Guan]

(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) :)

[Chris.Guan]

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

[Chris.Guan]

Comment on attachment 147257 [details]
Patch

I will make a new patch.

[Chris.Guan]

Created attachment 147468 [details]
Patch

[Antonio Gomes]

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.

[Chris.Guan]

Created attachment 147469 [details]
Patch

[Chris.Guan]

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

[WebKit Review Bot]

Comment on attachment 147469 [details]
Patch

Clearing flags on attachment: 147469

Committed r120300: <http://trac.webkit.org/changeset/120300>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.