Bug 170014 - [GTK] Remove notifications guards from GTK API layer
Summary: [GTK] Remove notifications guards from GTK API layer
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
Depends on:
Reported: 2017-03-23 11:36 PDT by ManDay
Modified: 2017-05-11 05:54 PDT (History)
8 users (show)

See Also:

Error as produced by cmake ; make (792 bytes, text/plain)
2017-03-23 11:36 PDT, ManDay
no flags Details
Patch (2.15 KB, patch)
2017-05-11 04:57 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ManDay 2017-03-23 11:36:17 PDT
Created attachment 305207 [details]
Error as produced by cmake ; make

Despite USE_LIBNOTIFY=OFF, the following error occurs during compilation of Source/WebKit2/CMakeFiles/WebKit2.dir/UIProcess/API/gtk/WebKitWebContext.cpp:

‘WebKitWebContextPrivate {aka struct _WebKitWebContextPrivate}’ has no member named ‘notificationProvider’

IIRC, I could easily fix this last time by commenting out the according section. I don't know whether this is proper but assume so. I'll submit a patch a.s.a.p.
Comment 1 Michael Catanzaro 2017-03-23 13:03:15 PDT
No that's not right. We support USE_LIBNOTIFY=OFF, but we do *not* support ENABLE_NOTIFICATIONS=OFF. The ENABLE(NOTIFICATIONS) guards should be removed from the GTK API layer. The only use of libnotify is in WebKitWebView.cpp and that is properly guarded.

I wonder how you wound up with ENABLE_NOTIFICATIONS=OFF? If something is setting that in our build system, it needs to be fixed.
Comment 2 Michael Catanzaro 2017-03-23 13:04:15 PDT
To be clear, WebKitNotificationProvider is required for notifications to work properly regardless of whether the default libnotify implementation is compiled in or not.
Comment 3 ManDay 2017-03-23 23:40:34 PDT
I do not understand your answer. What exactly is it that you're suggesting is missing? I configure WK using the following commandline

Comment 4 Michael Catanzaro 2017-03-24 05:59:14 PDT
(In reply to ManDay from comment #3)

This isn't supported, GTK+ port uses ENABLE_NOTIFICATIONS=ON. Remember back when we fixed a bunch of private options mistakenly not being marked as ADVANCED? This was one of them.

There is still a bug though, because we have ENABLE_NOTIFICATIONS guards in our GTK API code. We should either remove them, or make it public and actually allow building with it disabled.
Comment 5 Michael Catanzaro 2017-03-24 06:00:40 PDT
What you can do is build with ENABLE_NOTIFICATIONS=ON USE_LIBNOTIFY=OFF, in which case applications still get callbacks when there is a web notification but have to implement the notifications themselves (or just ignore it). That's also an option for applications when using USE_LIBNOTIFY=ON. libnotify is just a default handler.
Comment 6 Claudio Saavedra 2017-05-11 04:57:09 PDT
Created attachment 309707 [details]
Comment 7 Build Bot 2017-05-11 04:59:10 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 WebKit Commit Bot 2017-05-11 05:54:29 PDT
Comment on attachment 309707 [details]

Clearing flags on attachment: 309707

Committed r216675: <http://trac.webkit.org/changeset/216675>
Comment 9 WebKit Commit Bot 2017-05-11 05:54:30 PDT
All reviewed patches have been landed.  Closing bug.