Bug 88950 - [Blackberry] add a new Api named setAllowNotification
: [Blackberry] add a new Api named setAllowNotification
Status: RESOLVED FIXED
: WebKit
WebKit BlackBerry
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-06-12 20:01 PST by
Modified: 2012-06-14 02:33 PST (History)


Attachments
Patch (2.19 KB, patch)
2012-06-12 23:08 PST, Chris.Guan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.22 KB, patch)
2012-06-13 01:12 PST, Chris.Guan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2012-06-13 19:28 PST, Chris.Guan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2012-06-13 19:54 PST, Chris.Guan
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-06-12 20:01:47 PST
I would like to add a new API named setAllowNotification in webpage to let client set those allowed domains in notifications.
------- Comment #1 From 2012-06-12 23:08:59 PST -------
Created an attachment (id=147240) [details]
Patch
------- Comment #2 From 2012-06-13 01:12:46 PST -------
Created an attachment (id=147257) [details]
Patch
------- Comment #3 From 2012-06-13 07:46:13 PST -------
(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
------- Comment #4 From 2012-06-13 07:58:10 PST -------
(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?
------- Comment #5 From 2012-06-13 08:00:24 PST -------
(In reply to comment #4)
> (From update of attachment 147257 [details] [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 #6 From 2012-06-13 08:00:58 PST -------
(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?

> 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) ?
------- Comment #7 From 2012-06-13 08:08:31 PST -------
(In reply to comment #3)
> (From update of attachment 147257 [details] [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.
------- Comment #8 From 2012-06-13 08:12:38 PST -------
(In reply to comment #6)
> (From update of attachment 147257 [details] [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) :)
------- Comment #9 From 2012-06-13 08:13:51 PST -------
(In reply to comment #4)
> (From update of attachment 147257 [details] [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 #10 From 2012-06-13 08:36:02 PST -------
(From update of attachment 147257 [details])
I will make a new patch.
------- Comment #11 From 2012-06-13 19:28:19 PST -------
Created an attachment (id=147468) [details]
Patch
------- Comment #12 From 2012-06-13 19:39:09 PST -------
(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.
------- Comment #13 From 2012-06-13 19:54:36 PST -------
Created an attachment (id=147469) [details]
Patch
------- Comment #14 From 2012-06-13 19:57:46 PST -------
(In reply to comment #12)
> (From update of attachment 147468 [details] [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 #15 From 2012-06-14 02:32:55 PST -------
(From update of attachment 147469 [details])
Clearing flags on attachment: 147469

Committed r120300: <http://trac.webkit.org/changeset/120300>
------- Comment #16 From 2012-06-14 02:33:07 PST -------
All reviewed patches have been landed.  Closing bug.