Bug 88950 - [Blackberry] add a new Api named setAllowNotification
: [Blackberry] add a new Api named setAllowNotification
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 20:01 PDT by Chris.Guan
Modified: 2012-06-14 02:33 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris.Guan 2012-06-12 20:01:47 PDT
I would like to add a new API named setAllowNotification in webpage to let client set those allowed domains in notifications.
Comment 1 Chris.Guan 2012-06-12 23:08:59 PDT
Created attachment 147240 [details]
Patch
Comment 2 Chris.Guan 2012-06-13 01:12:46 PDT
Created attachment 147257 [details]
Patch
Comment 3 Antonio Gomes 2012-06-13 07:46:13 PDT
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 4 Antonio Gomes 2012-06-13 07:58:10 PDT
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?
Comment 5 Chris.Guan 2012-06-13 08:00:24 PDT
(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 6 Rob Buis 2012-06-13 08:00:58 PDT
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) ?
Comment 7 Chris.Guan 2012-06-13 08:08:31 PDT
(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.
Comment 8 Chris.Guan 2012-06-13 08:12:38 PDT
(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) :)
Comment 9 Chris.Guan 2012-06-13 08:13:51 PDT
(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 10 Chris.Guan 2012-06-13 08:36:02 PDT
Comment on attachment 147257 [details]
Patch

I will make a new patch.
Comment 11 Chris.Guan 2012-06-13 19:28:19 PDT
Created attachment 147468 [details]
Patch
Comment 12 Antonio Gomes 2012-06-13 19:39:09 PDT
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.
Comment 13 Chris.Guan 2012-06-13 19:54:36 PDT
Created attachment 147469 [details]
Patch
Comment 14 Chris.Guan 2012-06-13 19:57:46 PDT
(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 15 WebKit Review Bot 2012-06-14 02:32:55 PDT
Comment on attachment 147469 [details]
Patch

Clearing flags on attachment: 147469

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