Bug 30409 - Need to turn off notifications properly at runtime
Summary: Need to turn off notifications properly at runtime
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: John Gregg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-15 15:03 PDT by Andrew Wilson
Modified: 2009-10-30 12:37 PDT (History)
2 users (show)

See Also:


Attachments
patch (6.32 KB, patch)
2009-10-20 15:40 PDT, John Gregg
no flags Details | Formatted Diff | Diff
new patch for the new way (5.29 KB, patch)
2009-10-23 15:01 PDT, John Gregg
no flags Details | Formatted Diff | Diff
new patch for the new way (6.11 KB, patch)
2009-10-30 11:21 PDT, John Gregg
levin: review+
Details | Formatted Diff | Diff
patch w/levin's feedback (6.09 KB, patch)
2009-10-30 11:39 PDT, John Gregg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2009-10-15 15:03:55 PDT
This if statement should evaluate to false if notifications are disabled at runtime:

if ('webkitNotifications' in window)

Additionally, we need to disable notifications from worker context as well. This means that we need to move runtime enabling out of Settings and into some static variable/struct that can be called from any context.

You can look at https://bugs.webkit.org/show_bug.cgi?id=30240 for an example of how this was done for window.Audio()
Comment 1 John Gregg 2009-10-20 15:40:23 PDT
Created attachment 41529 [details]
patch

Patch adds support for the [EnabledAtRuntime] flag on DOMWindow.webkitNotifications and WorkerContext.webkitNotifications.  

After I switch chromium over to using this instead of Settings, I'll remove notifications from Settings in a second patch.
Comment 2 Eric Seidel (no email) 2009-10-21 16:10:05 PDT
I'm not sure if this is a two-sided patch needing special commit, or if this can be added to the commit-queue.
Comment 3 John Gregg 2009-10-21 16:13:14 PDT
I already had atwilson commit this for me:
http://trac.webkit.org/changeset/49893
Comment 4 Eric Seidel (no email) 2009-10-21 16:18:05 PDT
So the bug should have been closed. :)
Comment 5 John Gregg 2009-10-23 14:39:59 PDT
And now there's an even newer way of doing this.
Comment 6 John Gregg 2009-10-23 15:01:54 PDT
Created attachment 41750 [details]
new patch for the new way
Comment 7 John Gregg 2009-10-30 10:39:59 PDT
Comment on attachment 41750 [details]
new patch for the new way

d'oh forgot the r? last week.  now the patch is too out of date... will merge and resubmit.
Comment 8 John Gregg 2009-10-30 11:21:32 PDT
Created attachment 42219 [details]
new patch for the new way 

resynced and recreated the patch.
Comment 9 David Levin 2009-10-30 11:32:56 PDT
Comment on attachment 42219 [details]
new patch for the new way 

> Index: WebCore/bindings/v8/RuntimeEnabledFeatures.h
> +    static bool isNotificationsEnabled;

Hmmm, this should have been m_ since these are class members but I see you're following a pattern.

> Index: WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
> +    return WebCore::RuntimeEnabledFeatures::notificationsEnabled();

Since the code is in namespace WebCore, WebCore:: isn't needed.


> Index: WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp
> +    return WebCore::RuntimeEnabledFeatures::notificationsEnabled();

Since the code is in namespace WebCore, WebCore:: isn't needed.


Just remove the WebCore:: from the two places mentioned on landing.
Comment 10 John Gregg 2009-10-30 11:39:41 PDT
Created attachment 42221 [details]
patch w/levin's feedback

removing unnecessary WebCore::'s
Comment 11 WebKit Commit Bot 2009-10-30 12:37:20 PDT
Comment on attachment 42221 [details]
patch w/levin's feedback

Clearing flags on attachment: 42221

Committed r50348: <http://trac.webkit.org/changeset/50348>
Comment 12 WebKit Commit Bot 2009-10-30 12:37:24 PDT
All reviewed patches have been landed.  Closing bug.