Bug 116988

Summary: window.performance.now operation's property should be configurable
Product: WebKit Reporter: Zan Dobersek <zan>
Component: DOMAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, arv, buildbot, cdumez, commit-queue, darin, esprehn+autocc, ggaren, haraken, joepeck, rniwa
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 116983    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 none

Zan Dobersek
Reported 2013-05-29 12:41:45 PDT
window.performance.now operation's property should be configurable
Attachments
Patch (4.90 KB, patch)
2013-05-29 12:55 PDT, Zan Dobersek
no flags
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (481.30 KB, application/zip)
2013-05-29 19:00 PDT, Build Bot
no flags
Zan Dobersek
Comment 1 2013-05-29 12:55:51 PDT
Zan Dobersek
Comment 2 2013-05-29 12:58:41 PDT
Build Bot
Comment 3 2013-05-29 19:00:30 PDT
Comment on attachment 203262 [details] Patch Attachment 203262 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/714254 New failing tests: fast/dom/location-new-window-no-crash.html
Build Bot
Comment 4 2013-05-29 19:00:32 PDT
Created attachment 203298 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Alexey Proskuryakov
Comment 5 2013-05-29 22:30:09 PDT
Can you please elaborate on how the test verifies the code change?
Zan Dobersek
Comment 6 2013-05-30 03:32:44 PDT
First off, I should have noted this relies on importing the WebIDLParser.js and idlharness.js scripts in bug #116983. The test case provides the IDL interface that's first parsed through WebIDLParser.js. The test then verifies that the tested interface provided by the user agent maches the parsed one in all the exposed declarations and their behavior, as specified by the WebIDL specification. Comment #2 has the links to all the relevant bits.
Alexey Proskuryakov
Comment 7 2013-05-30 08:48:37 PDT
It looks like the IDL in the test doesn't even have "[Deletable]", so how does it verify that we added it to the actual IDL?
Alexey Proskuryakov
Comment 8 2013-05-30 10:14:20 PDT
Also, I'm confused as to why the tests pass if this depends on another patch.
Zan Dobersek
Comment 9 2013-05-30 11:09:16 PDT
The Deletable attribute is WebKitIDL-specific and not supported by WebIDL. http://trac.webkit.org/wiki/WebKitIDL#Deletable (I'm not sure though that the summary in this section is entirely correct - readonly keyword should control the writability while Deletable should control the configurability.) Using the Deletable attribute works around the current shortcomings of the JSC bindings generator regarding the WebIDL support. I reckon the generator should be fixed instead of working around these issues. As for the test passing on EWSs, I guess it's due to the outdated testharness scripts. The up-to-date test, currently failing, is located here: http://w3c-test.org/webperf/tests/approved/HighResolutionTime/idlharness.html
Chris Dumez
Comment 10 2013-06-04 07:45:34 PDT
(In reply to comment #8) > Also, I'm confused as to why the tests pass if this depends on another patch. I think only mac bots run tests and the new test is skipped on mac: LayoutTests/platform/mac/TestExpectations:http/tests/w3c/webperf This is because WEB_TIMING flag is disabled on mac.
Chris Dumez
Comment 11 2013-06-04 07:49:01 PDT
Comment on attachment 203262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203262&action=review > Source/WebCore/page/Performance.idl:66 > + [Deletable] double now(); You are right that *ALL* operations should be configurable according the the Web IDL specification (http://dev.w3.org/2006/webapi/WebIDL/#es-operations). However, your patch is only fixing one particular method in Performance.idl to get one test passing. If we want to comply with the Web IDL specification, this should really be fixed in the JSC bindings generator, for *ALL* the operations, not by adding [Deletable] extended attribute.
Zan Dobersek
Comment 12 2013-06-04 08:33:26 PDT
Comment on attachment 203262 [details] Patch I'll address the problem through modifying the bindings generator. Clearing the flags on the current patch and marking it as obsolete.
Darin Adler
Comment 13 2013-06-04 09:45:00 PDT
(In reply to comment #11) > You are right that *ALL* operations should be configurable according the the Web IDL specification (http://dev.w3.org/2006/webapi/WebIDL/#es-operations). However, your patch is only fixing one particular method in Performance.idl to get one test passing. If we want to comply with the Web IDL specification, this should really be fixed in the JSC bindings generator, for *ALL* the operations, not by adding [Deletable] extended attribute. When we make a change like that, we need to do it in a safe way. In similar cases in the past we have had to add an attribute that gives us the old behavior, then remove it from attributes and operations as we do sufficient testing and research to determine that it’s OK to change that particular one. Once that’s done we may be left with a few that need the old behavior for compatibility reasons or other actions we need to do. Doing it globally all in one step is much easier, but may not give acceptable results.
Chris Dumez
Comment 14 2013-06-04 10:54:47 PDT
(In reply to comment #13) > (In reply to comment #11) > > You are right that *ALL* operations should be configurable according the the Web IDL specification (http://dev.w3.org/2006/webapi/WebIDL/#es-operations). However, your patch is only fixing one particular method in Performance.idl to get one test passing. If we want to comply with the Web IDL specification, this should really be fixed in the JSC bindings generator, for *ALL* the operations, not by adding [Deletable] extended attribute. > > When we make a change like that, we need to do it in a safe way. > > In similar cases in the past we have had to add an attribute that gives us the old behavior, then remove it from attributes and operations as we do sufficient testing and research to determine that it’s OK to change that particular one. Once that’s done we may be left with a few that need the old behavior for compatibility reasons or other actions we need to do. > > Doing it globally all in one step is much easier, but may not give acceptable results. I understand making the change for all operations may seem risky, even though compliant with the Web IDL specification. However, my first impression when looking at this patch was that it looked like a quick fix for a particular W3C test by adding a [Deletable] extended attribute. This approach did not seem very scalable to me considering all operations are actually affected by the same issue. FYI, Firefox seems to comply with the Web IDL specification: Object.getOwnPropertyDescriptor(Window.prototype, "stop"); ({configurable:true, enumerable:true, value:function stop() { [native code] }, writable:true}) I think it is good if we match the Web IDL spec and Firefox eventually. So what is the suggested approach? Gradually add more and more [Deletable] extended attributes to operations? or if I understand your comment, fix the bindings generator but add [NotDeletable] to all operations? (and then gradually remove it).
Darin Adler
Comment 15 2013-06-04 11:04:37 PDT
(In reply to comment #14) > So what is the suggested approach? Gradually add more and more [Deletable] extended attributes to operations? or if I understand your comment, fix the bindings generator but add [NotDeletable] to all operations? (and then gradually remove it). Yes, I think the best way to do this is to fix the bindings generator, add [NotDeletable] everywhere, and remove [NotDeletable] as quickly as we can with sufficient testing and research. We could even add a no-op [NotDeletable] first if that helps us stage the changes.
Chris Dumez
Comment 16 2013-06-04 11:10:54 PDT
(In reply to comment #15) > (In reply to comment #14) > > So what is the suggested approach? Gradually add more and more [Deletable] extended attributes to operations? or if I understand your comment, fix the bindings generator but add [NotDeletable] to all operations? (and then gradually remove it). > > Yes, I think the best way to do this is to fix the bindings generator, add [NotDeletable] everywhere, and remove [NotDeletable] as quickly as we can with sufficient testing and research. We could even add a no-op [NotDeletable] first if that helps us stage the changes. Ok, thank you for the clarification.
Zan Dobersek
Comment 17 2013-09-28 00:36:04 PDT
(In reply to comment #15) > (In reply to comment #14) > > So what is the suggested approach? Gradually add more and more [Deletable] extended attributes to operations? or if I understand your comment, fix the bindings generator but add [NotDeletable] to all operations? (and then gradually remove it). > > Yes, I think the best way to do this is to fix the bindings generator, add [NotDeletable] everywhere, and remove [NotDeletable] as quickly as we can with sufficient testing and research. We could even add a no-op [NotDeletable] first if that helps us stage the changes. There are lots of operation declarations that would be assigned the new NotDeletable attribute -- would it be acceptable to use an interface attribute, for instance OperationsNotDeletable, that would enforce all the operations in the interface to stay non-configurable? This would reduce the amount of changes both before the generator is fixed (when the no-op attribute is introduced) and after (when the operations are made configurable). There still might be specific operations that would have to remain non-configurable, so I'd recommend setting up the no-op NotDeletable attribute as well and use it where necessary. As for the sufficient testing and research -- what exactly, apart from the layout tests, would this include?
Darin Adler
Comment 18 2013-09-28 08:46:39 PDT
(In reply to comment #17) > There are lots of operation declarations that would be assigned the new NotDeletable attribute -- would it be acceptable to use an interface attribute, for instance OperationsNotDeletable, that would enforce all the operations in the interface to stay non-configurable? This would reduce the amount of changes both before the generator is fixed (when the no-op attribute is introduced) and after (when the operations are made configurable). I don’t have a strong feeling either way. > As for the sufficient testing and research -- what exactly, apart from the layout tests, would this include? This is a pretty cosmic question. What is sufficient testing for changing web-facing API? One of the fundamental questions for the WebKit project without a single simple answer. This is like other web-facing API changes, things we do routinely all the time. One thing we look into is whether our old and new behavior matches other browsers. Not just specifications, but the actual browsers that web developers have been testing their websites with. For obscure things that are we have some reason to believe are not used widely on webpages, we sometimes just take the risk of changing behavior with little or no testing. For more widely used things we come up with a way to test for website compatibility and as much as possible, look into other issues, such as iOS and Mac app compatibility. But we get it wrong a lot. Consider, for example, <http://trac.webkit.org/changeset/154614>, which broke the Safari web browser’s reader feature.
Joseph Pecoraro
Comment 19 2016-08-24 13:03:30 PDT
This appears to have been fixed. js> Object.getOwnPropertyDescriptor(performance.__proto__, "now") ... configurable: true enumerable: true writable: true which matches other browsers.
Note You need to log in before you can comment on or make changes to this bug.