Bug 88919 – Change Notification.permissionLevel() to Notification.permission

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Jon Lee]

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.

[Radar WebKit Bug Importer]

<rdar://problem/11650319>

[Jon Lee]

Created attachment 153207 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153207 [details]
Patch

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

[Jon Lee]

Created attachment 153210 [details]
Patch

[Kentaro Hara]

Comment on attachment 153210 [details]
Patch

The change looks good. Any test cases?

[WebKit Review Bot]

Comment on attachment 153210 [details]
Patch

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

[Jon Lee]

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

[Jon Lee]

Created attachment 153288 [details]
Patch

[Kentaro Hara]

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?

[Jon Lee]

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

[Kentaro Hara]

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

[Jon Lee]

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

[Jon Lee]

Filed V8 support bug 91764.

[Kentaro Hara]

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

[Jon Lee]

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

[Kentaro Hara]

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

[Jon Lee]

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

[Kentaro Hara]

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

[Kentaro Hara]

Comment on attachment 153288 [details]
Patch

Marking r- since we need to support static attributes in V8 first.

[Kentaro Hara]

Now static attributes are supported in both JSC and V8.

You need to specify [CallWith=ScriptExecutionContext] for Notification.permission.

[Jon Lee]

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

[Jon Lee]

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

[Jon Lee]

Created attachment 154123 [details]
Patch

[Kentaro Hara]

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

[Jon Lee]

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

[Kentaro Hara]

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

[Jon Lee]

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

[Kentaro Hara]

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

[Kentaro Hara]

Comment on attachment 154123 [details]
Patch

For now marking r- due to missing tests.

[Kentaro Hara]

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.

[Jon Lee]

(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

[Jon Lee]

Created attachment 156432 [details]
Patch

[Jon Lee]

Comment on attachment 156432 [details]
Patch

Added test

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Jon Lee]

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

[Jon Lee]

Created attachment 156444 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

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', '...');

[Jon Lee]

Created attachment 156722 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Jon Lee]

I'm not exactly sure why the test is failing on chromium. Running it locally on mac worked ok. Kentaro, any thoughts?

[Kentaro Hara]

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

[Jon Lee]

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

[Kentaro Hara]

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

[Jon Lee]

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

[Kentaro Hara]

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

[Jon Lee]

(In reply to comment #49)
> 
> Yes. Looks like the implementation of static attributes of V8 is wrong. Let me take a look.

Any news?

[Kentaro Hara]

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

[Kentaro Hara]

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?

[Jon Lee]

Created attachment 157659 [details]
Patch

[Jon Lee]

Created attachment 157664 [details]
Patch

[Kentaro Hara]

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.

[Jon Lee]

Committed r125280: <http://trac.webkit.org/changeset/125280>