Bug 88919 - Change Notification.permissionLevel() to Notification.permission
Summary: Change Notification.permissionLevel() to Notification.permission
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 88920 91764 91924 91925
Blocks: 80472 90603
  Show dependency treegraph
 
Reported: 2012-06-12 14:26 PDT by Jon Lee
Modified: 2012-08-10 08:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.20 KB, patch)
2012-07-19 01:30 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (3.19 KB, patch)
2012-07-19 01:50 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2012-07-19 10:11 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (3.30 KB, patch)
2012-07-24 12:48 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2012-08-03 12:23 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (432.48 KB, application/zip)
2012-08-03 13:07 PDT, WebKit Review Bot
no flags Details
Patch (5.82 KB, patch)
2012-08-03 13:19 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (419.24 KB, application/zip)
2012-08-03 14:10 PDT, WebKit Review Bot
no flags Details
Patch (5.50 KB, patch)
2012-08-06 10:33 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (377.62 KB, application/zip)
2012-08-06 11:32 PDT, WebKit Review Bot
no flags Details
Patch (6.26 KB, patch)
2012-08-10 00:51 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2012-08-10 01:07 PDT, Jon Lee
haraken: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-06-12 14:26:11 PDT
The latest spec draft http://dvcs.w3.org/hg/notifications/raw-file/tip/Overview.html#api utilizes a readonly attribute on Notification instead of a function.
Comment 1 Radar WebKit Bug Importer 2012-06-12 14:27:08 PDT
<rdar://problem/11650319>
Comment 2 Jon Lee 2012-07-19 01:30:20 PDT
Created attachment 153207 [details]
Patch
Comment 3 WebKit Review Bot 2012-07-19 01:41:51 PDT
Comment on attachment 153207 [details]
Patch

Attachment 153207 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13278807
Comment 4 Jon Lee 2012-07-19 01:50:08 PDT
Created attachment 153210 [details]
Patch
Comment 5 Kentaro Hara 2012-07-19 01:52:29 PDT
Comment on attachment 153210 [details]
Patch

The change looks good. Any test cases?
Comment 6 WebKit Review Bot 2012-07-19 01:59:17 PDT
Comment on attachment 153210 [details]
Patch

Attachment 153210 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13282844
Comment 7 Jon Lee 2012-07-19 09:12:38 PDT
(In reply to comment #5)
> (From update of attachment 153210 [details])
> The change looks good. Any test cases?

Not at this time, because the DRT/WTR for Mac still lacks support of notifications.

I am going to have to add stubs for V8. It needs an implementation for static attributes.
Comment 8 Jon Lee 2012-07-19 10:11:26 PDT
Created attachment 153288 [details]
Patch
Comment 9 Kentaro Hara 2012-07-19 10:21:09 PDT
Comment on attachment 153288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153288&action=review

> Source/WebCore/ChangeLog:18
> +        * bindings/scripts/CodeGeneratorV8.pm: Alter generation script to ignore static attributes until
> +        static attributes are supported.

Let me confirm: Currently both JSC and V8 do not support static attributes, right? Then how will Notification.permission work?
Comment 10 Jon Lee 2012-07-19 10:51:06 PDT
(In reply to comment #9)
> (From update of attachment 153288 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153288&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        * bindings/scripts/CodeGeneratorV8.pm: Alter generation script to ignore static attributes until
> > +        static attributes are supported.
> 
> Let me confirm: Currently both JSC and V8 do not support static attributes, right? Then how will Notification.permission work?

JSC currently does (see bug 88920)
Comment 11 Kentaro Hara 2012-07-19 10:59:10 PDT
(In reply to comment #10)
> > Let me confirm: Currently both JSC and V8 do not support static attributes, right? Then how will Notification.permission work?
> 
> JSC currently does (see bug 88920)

Ah, thanks for the clarification. Then we might want to first support static attributes in V8 before landing the patch. Avoiding Chromium build error by skipping binding code would not be a good idea. (If you really want to land the patch first, at least please file a bug and add FIXME in CodeGeneratorV8.pm.)
Comment 12 Jon Lee 2012-07-19 11:06:15 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > > Let me confirm: Currently both JSC and V8 do not support static attributes, right? Then how will Notification.permission work?
> > 
> > JSC currently does (see bug 88920)
> 
> Ah, thanks for the clarification. Then we might want to first support static attributes in V8 before landing the patch. Avoiding Chromium build error by skipping binding code would not be a good idea. (If you really want to land the patch first, at least please file a bug and add FIXME in CodeGeneratorV8.pm.)

Yes, I'd like to land this now, and I can add the FIXME, and file the bug when I check it in. Is the patch ok, otherwise?
Comment 13 Jon Lee 2012-07-19 11:11:34 PDT
Filed V8 support bug 91764.
Comment 14 Kentaro Hara 2012-07-19 11:21:03 PDT
(In reply to comment #12)
> Yes, I'd like to land this now, and I can add the FIXME, and file the bug when I check it in. Is the patch ok, otherwise?

The NOTIFICATIONS flag is already enabled on Chromium too. So your patch will make Notification.permission behavior different between Safari and Chromium, won't it?
Comment 15 Jon Lee 2012-07-19 11:35:47 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > Yes, I'd like to land this now, and I can add the FIXME, and file the bug when I check it in. Is the patch ok, otherwise?
> 
> The NOTIFICATIONS flag is already enabled on Chromium too. So your patch will make Notification.permission behavior different between Safari and Chromium, won't it?

With this patch, I believe that Notification.permission will not exist on Chromium until support for static attributes has been added. Once V8 supports static attributes, everything should work, and no additional changes are needed in WebCore.
Comment 16 Kentaro Hara 2012-07-19 12:32:53 PDT
(In reply to comment #15)
> With this patch, I believe that Notification.permission will not exist on Chromium until support for static attributes has been added. Once V8 supports static attributes, everything should work, and no additional changes are needed in WebCore.

Yes, so we want to support static attributes in V8 first, to keep the behavior of Safari and Chromium the same.
Comment 17 Jon Lee 2012-07-19 13:56:47 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > With this patch, I believe that Notification.permission will not exist on Chromium until support for static attributes has been added. Once V8 supports static attributes, everything should work, and no additional changes are needed in WebCore.
> 
> Yes, so we want to support static attributes in V8 first, to keep the behavior of Safari and Chromium the same.

Who's best to implement it?

Because the Notification spec is still under development, would it be better for Chromium to remove the NOTIFICATIONS flag until the implementation has matured? (see http://lists.webkit.org/pipermail/webkit-dev/2012-March/019948.html)
Comment 18 Kentaro Hara 2012-07-19 23:44:22 PDT
(In reply to comment #17)
> Because the Notification spec is still under development, would it be better for Chromium to remove the NOTIFICATIONS flag until the implementation has matured? (see http://lists.webkit.org/pipermail/webkit-dev/2012-March/019948.html)

It sounds like you are saying "for now let's disable NOTIFICATIONS in V8 because upcoming patches don't work well in V8. Let's implement NOTIFICATIONS in JSC first. Once someone fixes all V8 errors in the future, then V8 will also be able to enable NOTIFICATIONS", which would not be a good idea. Committers are responsible to keep all ports work as much as possible.

> > Yes, so we want to support static attributes in V8 first, to keep the behavior of Safari and Chromium the same.
> 
> Who's best to implement it?

If you can, it's best. If it's difficult, I will implement it (but please wait until next week).
Comment 19 Kentaro Hara 2012-07-19 23:45:51 PDT
Comment on attachment 153288 [details]
Patch

Marking r- since we need to support static attributes in V8 first.
Comment 20 Kentaro Hara 2012-07-24 02:00:27 PDT
Now static attributes are supported in both JSC and V8.

You need to specify [CallWith=ScriptExecutionContext] for Notification.permission.
Comment 21 Jon Lee 2012-07-24 12:16:13 PDT
(In reply to comment #20)
> Now static attributes are supported in both JSC and V8.
> 
> You need to specify [CallWith=ScriptExecutionContext] for Notification.permission.

Patch for 91924 does not take into account this attribute, for either platform.
Comment 22 Jon Lee 2012-07-24 12:17:30 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > Now static attributes are supported in both JSC and V8.
> > 
> > You need to specify [CallWith=ScriptExecutionContext] for Notification.permission.
> 
> Patch for 91924 does not take into account this attribute, for either platform.

Sorry I misspoke, I put the attribute in the wrong place.
Comment 23 Jon Lee 2012-07-24 12:48:33 PDT
Created attachment 154123 [details]
Patch
Comment 24 Kentaro Hara 2012-07-24 14:59:21 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 153210 [details] [details])
> > The change looks good. Any test cases?
> 
> Not at this time, because the DRT/WTR for Mac still lacks support of notifications.

Specifically, what prevents you from writing a test for this?
Comment 25 Jon Lee 2012-07-24 16:31:29 PDT
(In reply to comment #24)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (From update of attachment 153210 [details] [details] [details])
> > > The change looks good. Any test cases?
> > 
> > Not at this time, because the DRT/WTR for Mac still lacks support of notifications.
> 
> Specifically, what prevents you from writing a test for this?

From being able to test them for the Mac port.
Comment 26 Kentaro Hara 2012-07-24 16:36:16 PDT
(In reply to comment #25)
> > Specifically, what prevents you from writing a test for this?
> 
> From being able to test them for the Mac port.

What happens if you test the following code? Otherwise, how are you checking that your change is correct?

  var notification = new Notification("title");
  shouldBe('notification.permission', ...);

I think the code will work on Chromium. Then it would be reasonable to add the test and skip the test on Mac port.
Comment 27 Jon Lee 2012-07-24 16:44:08 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > > Specifically, what prevents you from writing a test for this?
> > 
> > From being able to test them for the Mac port.
> 
> What happens if you test the following code? Otherwise, how are you checking that your change is correct?
> 
>   var notification = new Notification("title");
>   shouldBe('notification.permission', ...);
> 
> I think the code will work on Chromium. Then it would be reasonable to add the test and skip the test on Mac port.

Currently all notifications tests are skipped on the mac port. It's not just this test. There will need to be a duplicate suite of tests written based on what is already at fast/notifications, which only uses the legacy API.

I'm working on trying to get it to work with WTR (and DRT), but it's a much bigger project, and until then, writing these tests will be prohibitively slow because I have to go through EWS. If there's a bug in the test, I have to go through that (long) cycle.

I'd also prefer that the tests exist in http/tests, because we can test it against http, instead of over file, although tests for that protocol should also exist.

There are already separate bugs tracking both the need for a new test suite based on the new API, and WRT support. I know this is atypical, but I'd like for the test for this to be considered as part of that bug.
Comment 28 Kentaro Hara 2012-07-24 16:55:02 PDT
(In reply to comment #27)
> Currently all notifications tests are skipped on the mac port. It's not just this test. There will need to be a duplicate suite of tests written based on what is already at fast/notifications, which only uses the legacy API.
> 
> I'm working on trying to get it to work with WTR (and DRT), but it's a much bigger project, and until then, writing these tests will be prohibitively slow because I have to go through EWS. If there's a bug in the test, I have to go through that (long) cycle.

I do not think that's a good idea. Given that a patch is testable with reasonable effort, you should add a test for each patch. "Let's land a bunch of patches without tests, and then fix a bunch of tests and bugs later" sounds unreasonable.

Looks like there is no test for Notification.permission (or Notification.permissionLevel) in fast/notifications nor http/tests.
Comment 29 Kentaro Hara 2012-07-24 17:18:53 PDT
Comment on attachment 154123 [details]
Patch

For now marking r- due to missing tests.
Comment 30 Kentaro Hara 2012-07-25 00:57:10 PDT
We might have made a mistake... There is no syntax "static attribute" in the Web IDL spec. static can be used for operations only. (http://www.w3.org/TR/WebIDL/#idl-static-operations)

Why is 'static' needed for Notification permission? If this is a bug of the Notification spec, I'll roll out patches for static attributes.
Comment 31 Jon Lee 2012-07-25 10:33:52 PDT
(In reply to comment #30)
> We might have made a mistake... There is no syntax "static attribute" in the Web IDL spec. static can be used for operations only. (http://www.w3.org/TR/WebIDL/#idl-static-operations)
> 
> Why is 'static' needed for Notification permission? If this is a bug of the Notification spec, I'll roll out patches for static attributes.

The lack of static attributes was an oversight in the WebIDL spec that was unearthed as a result of the development of the Notifications spec. You'll see it in the editor's draft:

http://dev.w3.org/2006/webapi/WebIDL/#idl-static-attributes-and-operations
Comment 32 Jon Lee 2012-08-03 12:23:03 PDT
Created attachment 156432 [details]
Patch
Comment 33 Jon Lee 2012-08-03 12:41:15 PDT
Comment on attachment 156432 [details]
Patch

Added test
Comment 34 WebKit Review Bot 2012-08-03 13:07:45 PDT
Comment on attachment 156432 [details]
Patch

Attachment 156432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13437005

New failing tests:
fast/notifications/notifications-standard-check-permission.html
Comment 35 WebKit Review Bot 2012-08-03 13:07:50 PDT
Created attachment 156443 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 36 Jon Lee 2012-08-03 13:12:34 PDT
(In reply to comment #34)
> (From update of attachment 156432 [details])
> Attachment 156432 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13437005
> 
> New failing tests:
> fast/notifications/notifications-standard-check-permission.html

The test failed for the same reason that notifications-check-permission.html failed. (That test is skipped on Chromium, tracked by bug 81697.)

I can simplify the test to just check for the attribute.
Comment 37 Jon Lee 2012-08-03 13:19:24 PDT
Created attachment 156444 [details]
Patch
Comment 38 WebKit Review Bot 2012-08-03 14:10:25 PDT
Comment on attachment 156444 [details]
Patch

Attachment 156444 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13437019

New failing tests:
fast/notifications/notifications-permission.html
Comment 39 WebKit Review Bot 2012-08-03 14:10:30 PDT
Created attachment 156454 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 40 Kentaro Hara 2012-08-04 20:02:37 PDT
Comment on attachment 156444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156444&action=review

> LayoutTests/fast/notifications/notifications-permission.html:28
> +            if (!window.Notification) {
> +                log("FAIL: No Notification interface!");
> +            }
> +            if (!window.Notification.permission) {
> +                log("FAIL: static permission attribute not found");
> +            }
> +            
> +            var N = window.Notification.permission;
> +            if (N != 0) {
> +                log("PASS: Permission not granted.");
> +            } else {
> +                log("FAIL: Permission shouldn't be granted.");
> +            }

Let's use fast/js/resources/js-test-pre.js. Then you can write these tests like this:

  shouldBe('window.Notification', '...');
  shouldBe('window.Notification.permission', '...');
Comment 41 Jon Lee 2012-08-06 10:33:19 PDT
Created attachment 156722 [details]
Patch
Comment 42 WebKit Review Bot 2012-08-06 11:32:07 PDT
Comment on attachment 156722 [details]
Patch

Attachment 156722 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13449067

New failing tests:
fast/notifications/notifications-permission.html
Comment 43 WebKit Review Bot 2012-08-06 11:32:11 PDT
Created attachment 156731 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 44 Jon Lee 2012-08-06 11:44:59 PDT
I'm not exactly sure why the test is failing on chromium. Running it locally on mac worked ok. Kentaro, any thoughts?
Comment 45 Kentaro Hara 2012-08-06 16:51:37 PDT
(In reply to comment #44)
> I'm not exactly sure why the test is failing on chromium. Running it locally on mac worked ok. Kentaro, any thoughts?

I experimented, and it looks like that not only permission but also all other attributes are not exposed to JavaScript in Chrome. Let me check when I have time.
Comment 46 Jon Lee 2012-08-06 16:53:40 PDT
(In reply to comment #45)
> (In reply to comment #44)
> > I'm not exactly sure why the test is failing on chromium. Running it locally on mac worked ok. Kentaro, any thoughts?
> 
> I experimented, and it looks like that not only permission but also all other attributes are not exposed to JavaScript in Chrome. Let me check when I have time.

What do you mean all other attributes? Other static attributes?
Comment 47 Kentaro Hara 2012-08-06 16:56:36 PDT
(In reply to comment #46)
> > I experimented, and it looks like that not only permission but also all other attributes are not exposed to JavaScript in Chrome. Let me check when I have time.
> 
> What do you mean all other attributes? Other static attributes?

All other attributes of Notification. e.g. onshow, ondisplay, onerror, dir, tag, show(), cancel() etc are not exposed to window.Notification (They return an undefined). But only requestPermission() is correctly exposed. Sounds strange.
Comment 48 Jon Lee 2012-08-06 17:11:42 PDT
(In reply to comment #47)
> (In reply to comment #46)
> > > I experimented, and it looks like that not only permission but also all other attributes are not exposed to JavaScript in Chrome. Let me check when I have time.
> > 
> > What do you mean all other attributes? Other static attributes?
> 
> All other attributes of Notification. e.g. onshow, ondisplay, onerror, dir, tag, show(), cancel() etc are not exposed to window.Notification (They return an undefined). But only requestPermission() is correctly exposed. Sounds strange.

The attributes you cited should not be exposed on the constructor because they only exist when you make a notification instance, so that is correct. requestPermission() should be available from the constructor object, however, because it is a _static_ function. Likewise, this permission attribute should be accessible from the constructor object, not an instance object.

For example, if you do

var n = new Notification("title")

do you then see a n.permission? If so, that's wrong.
Comment 49 Kentaro Hara 2012-08-06 17:13:31 PDT
(In reply to comment #48)
> The attributes you cited should not be exposed on the constructor because they only exist when you make a notification instance, so that is correct. requestPermission() should be available from the constructor object, however, because it is a _static_ function. Likewise, this permission attribute should be accessible from the constructor object, not an instance object.

Ah, sorry, you're right.

> var n = new Notification("title")
> 
> do you then see a n.permission? If so, that's wrong.

Yes. Looks like the implementation of static attributes of V8 is wrong. Let me take a look.
Comment 50 Jon Lee 2012-08-08 08:34:00 PDT
(In reply to comment #49)
> 
> Yes. Looks like the implementation of static attributes of V8 is wrong. Let me take a look.

Any news?
Comment 51 Kentaro Hara 2012-08-08 08:40:12 PDT
(In reply to comment #50)
> (In reply to comment #49)
> > 
> > Yes. Looks like the implementation of static attributes of V8 is wrong. Let me take a look.
> 
> Any news?

Filed bug 93488. Hopefully I can work on it on Friday.
Comment 52 Kentaro Hara 2012-08-09 19:08:42 PDT
jonlee: As I mentioned in bug 93488, at present CodeGeneratorV8.pm cannot support static attributes due to a V8 bug. It might take time to fix the V8 bug. So would you skip the test in Chromium and re-upload your patch?
Comment 53 Jon Lee 2012-08-10 00:51:08 PDT
Created attachment 157659 [details]
Patch
Comment 54 Jon Lee 2012-08-10 01:07:33 PDT
Created attachment 157664 [details]
Patch
Comment 55 Kentaro Hara 2012-08-10 04:04:59 PDT
Comment on attachment 157664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157664&action=review

> LayoutTests/platform/chromium/TestExpectations:3479
> +BUGWK93488 SKIP : fast/notifications/notifications-permission.html = TEXT

Please add a comment why this test is skipped.
Comment 56 Jon Lee 2012-08-10 08:20:53 PDT
Committed r125280: <http://trac.webkit.org/changeset/125280>