Bug 28930 - Enable switch for notifications in Page Settings
Summary: Enable switch for notifications in Page Settings
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: John Gregg
Depends on:
Reported: 2009-09-02 17:58 PDT by John Gregg
Modified: 2009-09-24 19:46 PDT (History)
5 users (show)

See Also:

patch which adds [set]desktopNotificationsEnabled() flag to Settings object (18.02 KB, patch)
2009-09-03 19:02 PDT, John Gregg
no flags Details | Formatted Diff | Diff
same patch w/ style fix (18.03 KB, patch)
2009-09-03 19:05 PDT, John Gregg
eric: commit-queue-
Details | Formatted Diff | Diff
same patch, one more tab gone and default false. (18.02 KB, patch)
2009-09-04 09:45 PDT, John Gregg
no flags Details | Formatted Diff | Diff
same patch, rename to "experimental" (18.54 KB, patch)
2009-09-14 16:25 PDT, John Gregg
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Gregg 2009-09-02 17:58:23 PDT
Keeping with the pattern of other new features, notifications needs the ability to hide from javascript at runtime based, e.g. on a command line flag to the browser.

Proposing a new boolean parameter in the page Settings object which controls whether notifications are enabled.
Comment 1 John Gregg 2009-09-03 19:02:15 PDT
Created attachment 39029 [details]
patch which adds [set]desktopNotificationsEnabled() flag to Settings object

Patch covers the following:
 - adds desktopNotificationsEnabled() and setDesktopNotificationsEnabled(bool) to Settings object
 - checks the setting before returning access to notificationCenter
 - adds support for the setting via WebPreferencesPrivate in mac/win WebKit
 - sets the flag to true for DumpRenderTree
Comment 2 John Gregg 2009-09-03 19:05:55 PDT
Created attachment 39030 [details]
same patch w/ style fix

same patch as above with one stray tab expunged
Comment 3 Eric Seidel (no email) 2009-09-04 00:31:16 PDT
Comment on attachment 39030 [details]
same patch w/ style fix

LGTM.  I'm not sure that Mac/Win will want this on by default, but they can always fix that:
 323         [NSNumber numberWithBool:YES],  WebKitDesktopNotificationsEnabledPreferenceKey,
Comment 4 Eric Seidel (no email) 2009-09-04 00:42:45 PDT
Comment on attachment 39030 [details]
same patch w/ style fix

Rejecting patch 39030 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None
Comment 5 Eric Seidel (no email) 2009-09-04 00:44:30 PDT
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:


    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

You'll need to remove the tabs from your patch.
Comment 6 John Gregg 2009-09-04 09:45:19 PDT
Created attachment 39070 [details]
same patch, one more tab gone and default false.

Removed that tab, and I agree it should default off at least until the point the feature and tests are enabled, so I changed that YES to a NO.
Comment 7 Alexey Proskuryakov 2009-09-04 12:39:48 PDT
Most new features cannot and needn't be enabled at runtime. Unless there's a special need for notifications, I'd say that we shouldn't do this at all.
Comment 8 John Gregg 2009-09-04 12:53:20 PDT
Maybe I'm not explaining it precisely, but there are a lot of similar options in WebCore/page/Settings.h:
        bool m_isJavaEnabled : 1;
        bool m_arePluginsEnabled : 1;
        bool m_databasesEnabled : 1;
        bool m_localStorageEnabled : 1;
        bool m_sessionStorageEnabled : 1;
        bool m_offlineWebApplicationCacheEnabled : 1;
        bool m_downloadableBinaryFontsEnabled : 1;
(and _many_ others), which is what I meant by "following the pattern of other new features". 

The goal is to support a command line flag to the browser which causes the javascript object to be exposed or not exposed, in order to roll out this new feature gradually.

Without a change like this the best option is exposing the same javascript API at all times but changing the behavior of the API based on options, which is less ideal.
Comment 9 Eric Seidel (no email) 2009-09-05 01:25:59 PDT
@ap, can you please clarify?

@john: no need to mark me as the requestee.  We really should turn off that field in Bugzilla since we use it so differently than mozilla does.
Comment 10 Alexey Proskuryakov 2009-09-05 23:03:58 PDT
Some of these are runtime settings because clients (such as Safari) want them to be runtime. I'm not sure about downloadable binary fonts, or database-related features.
Comment 11 Adam Barth 2009-09-06 22:01:32 PDT
Comment on attachment 39030 [details]
same patch w/ style fix

Clearing review flag on obsolete patch.
Comment 12 Peter Kasting 2009-09-08 09:57:55 PDT
(In reply to comment #10)
> Some of these are runtime settings because clients (such as Safari) want them
> to be runtime.

Yes, that is precisely the case here: Chromium wants this to be runtime.
Comment 13 Alexey Proskuryakov 2009-09-08 10:16:34 PDT
(In reply to comment #12)
> Yes, that is precisely the case here: Chromium wants this to be runtime.

That's a good reason.

One more question: in this particular case, does a client need any WebCore changes in order to make this a runtime setting? Can it just turn client callbacks into no-ops, and not display any notifications?
Comment 14 John Gregg 2009-09-08 10:20:48 PDT
In this particular case, yes, the client can provide a NotificationPresenter which simply no-ops all calls into it.  

However in that state, a web site would see window.webkitNotifications in JavaScript but not be able to use it successfully, and Chromium would prefer to hide webkitNotifications from script entirely if the command-line switch hasn't been enabled.
Comment 15 Alexey Proskuryakov 2009-09-08 10:53:58 PDT
Even with this patch, "webkitNotifications in window" will be true, I think. We don't have a way to reliably hide a disabled feature, as far as I know.

Can you describe what exactly the use case for this is? Can the same JavaScript code be exposed to both behaviors? We have some precedent with popup window blocking, and sites are not informed about the blocker being active in advance.

In another bug, someone mentioned that Chromium wants all new features to have runtime switches for easier transition between distribution channels. If that's also one of the reasons here, it's perfectly valid, but it needs some webkit-dev discussion, in my opinion.
Comment 16 John Gregg 2009-09-08 11:13:55 PDT
It's true, that window.webkitNotifications === null with this flag off, as opposed to undefined, but the check for

if (window.webkitNotifications) {
  use notifications
} else {

will behave correctly, and the methods themselves (like window.webkitNotification.createNotification) *are* reliably hidden.

As you say, command line switches for managing the rollout of the new feature in Chromium is exactly the reason for this -- I will try to help gather consensus for how to deal with it.
Comment 17 John Gregg 2009-09-14 16:25:29 PDT
Created attachment 39579 [details]
same patch, rename to "experimental"

As discussed on webkit-dev, this patch renames the functions being added to "experimentalNotifications" to indicate that this feature isn't completely implemented.
Comment 18 Eric Seidel (no email) 2009-09-24 14:00:43 PDT
Comment on attachment 39579 [details]
same patch, rename to "experimental"

This patch looks good, except for the fact that notifications are on for Windows DRT and off for Mac DRT.  Is that what you intended?

Please correct that or explain why it's on for one and not the other when landing.
Comment 19 John Gregg 2009-09-24 14:05:14 PDT
This was intentional; the modifications to DRT required to run the notifications tests have already landed for Windows, and the DRT code in this patch is required for those tests to continue to work (although they are disabled in the tree).  I haven't submitted those modifications yet for Mac DRT.
Comment 20 Andrew Wilson 2009-09-24 19:46:02 PDT
Committed as r48745.