Bug 28930 – Enable switch for notifications in Page Settings

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Gregg]

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.

[John Gregg]

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

[John Gregg]

Created attachment 39030 [details]
same patch w/ style fix

same patch as above with one stray tab expunged

[Eric Seidel (no email)]

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,

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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:

        trunk/WebKit/win/ChangeLog

    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.

[John Gregg]

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.

[Alexey Proskuryakov]

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.

[John Gregg]

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.

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

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.

[Adam Barth]

Comment on attachment 39030 [details]
same patch w/ style fix

Clearing review flag on obsolete patch.

[Peter Kasting]

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

[Alexey Proskuryakov]

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

[John Gregg]

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.

[Alexey Proskuryakov]

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.

[John Gregg]

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 {
  don't
}

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.

[John Gregg]

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.

[Eric Seidel (no email)]

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.

[John Gregg]

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.

[Andrew Wilson]

Committed as r48745.