Summary: | Enable switch for notifications in Page Settings | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Gregg <johnnyg> | ||||||||||
Component: | WebCore Misc. | Assignee: | John Gregg <johnnyg> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, atwilson, eric, jmalonzo, pkasting | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
John Gregg
2009-09-02 17:58:23 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
Created attachment 39030 [details]
same patch w/ style fix
same patch as above with one stray tab expunged
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 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
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. 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.
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. 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. @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. 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 on attachment 39030 [details]
same patch w/ style fix
Clearing review flag on obsolete patch.
(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. (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? 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. 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. 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. 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 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.
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. Committed as r48745. |