| Summary: | Change Notification.permissionLevel() to Notification.permission | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||||||||||||||||||||||||
| Component: | DOM | Assignee: | Jon Lee <jonlee> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, haraken, japhet, jochen, kihong.kwon, ojan, tasak, webkit-bug-importer, webkit.review.bot | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||||||||||
| OS: | OS X 10.7 | ||||||||||||||||||||||||||||
| Bug Depends on: | 88920, 91764, 91924, 91925 | ||||||||||||||||||||||||||||
| Bug Blocks: | 80472, 90603 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Jon Lee
2012-06-12 14:26:11 PDT
Created attachment 153207 [details]
Patch
Comment on attachment 153207 [details] Patch Attachment 153207 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13278807 Created attachment 153210 [details]
Patch
Comment on attachment 153210 [details]
Patch
The change looks good. Any test cases?
Comment on attachment 153210 [details] Patch Attachment 153210 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13282844 (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. Created attachment 153288 [details]
Patch
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? (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) (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.) (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? Filed V8 support bug 91764. (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? (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. (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. (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) (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 on attachment 153288 [details]
Patch
Marking r- since we need to support static attributes in V8 first.
Now static attributes are supported in both JSC and V8. You need to specify [CallWith=ScriptExecutionContext] for Notification.permission. (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. (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. Created attachment 154123 [details]
Patch
(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? (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. (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. (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. (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 on attachment 154123 [details]
Patch
For now marking r- due to missing tests.
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. (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 Created attachment 156432 [details]
Patch
Comment on attachment 156432 [details]
Patch
Added test
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 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
(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. Created attachment 156444 [details]
Patch
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 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 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', '...'); Created attachment 156722 [details]
Patch
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 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
I'm not exactly sure why the test is failing on chromium. Running it locally on mac worked ok. Kentaro, any thoughts? (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. (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? (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. (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. (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. (In reply to comment #49) > > Yes. Looks like the implementation of static attributes of V8 is wrong. Let me take a look. Any news? (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. 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? Created attachment 157659 [details]
Patch
Created attachment 157664 [details]
Patch
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. Committed r125280: <http://trac.webkit.org/changeset/125280> |