Bug 116988 – window.performance.now operation's property should be configurable

• Component: DOM
• Status: RESOLVED FIXED

[Zan Dobersek]

window.performance.now operation's property should be configurable

[Zan Dobersek]

Created attachment 203262 [details]
Patch

[Zan Dobersek]

WebIDL specificaton on the problem:
http://www.w3.org/TR/WebIDL/#es-operations

The original test:
https://dvcs.w3.org/hg/webperf/file/577503353c71/tests/approved/HighResolutionTime/idlharness.html

Location of the configurability test in idlharness.js:
https://dvcs.w3.org/hg/resources/file/06cab30bcee2/idlharness.js#l1421

[Build Bot]

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]

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]

Can you please elaborate on how the test verifies the code change?

[Zan Dobersek]

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]

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]

Also, I'm confused as to why the tests pass if this depends on another patch.

[Zan Dobersek]

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]

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

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

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

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

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

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

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

This appears to have been fixed.

    js> Object.getOwnPropertyDescriptor(performance.__proto__, "now")
    ...
    configurable: true
    enumerable: true
    writable: true

which matches other browsers.