WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
28930
Enable switch for notifications in Page Settings
https://bugs.webkit.org/show_bug.cgi?id=28930
Summary
Enable switch for notifications in Page Settings
John Gregg
Reported
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.
Attachments
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
John Gregg
Comment 1
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
John Gregg
Comment 2
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
Eric Seidel (no email)
Comment 3
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,
Eric Seidel (no email)
Comment 4
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
Eric Seidel (no email)
Comment 5
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: 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
Comment 6
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.
Alexey Proskuryakov
Comment 7
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.
John Gregg
Comment 8
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.
Eric Seidel (no email)
Comment 9
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.
Alexey Proskuryakov
Comment 10
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.
Adam Barth
Comment 11
2009-09-06 22:01:32 PDT
Comment on
attachment 39030
[details]
same patch w/ style fix Clearing review flag on obsolete patch.
Peter Kasting
Comment 12
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.
Alexey Proskuryakov
Comment 13
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?
John Gregg
Comment 14
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.
Alexey Proskuryakov
Comment 15
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.
John Gregg
Comment 16
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 { 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
Comment 17
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.
Eric Seidel (no email)
Comment 18
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.
John Gregg
Comment 19
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.
Andrew Wilson
Comment 20
2009-09-24 19:46:02 PDT
Committed as
r48745
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug