Improve coverage of fast/dom/Window/window-properties test

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kent Hansen]

The fast/dom/Window/window-properties test uses a for..in statement to enumerate the properties of an object. An issue with that approach is that non-enumerable properties (such as the built-in ECMA properties) are not enumerated, and hence not tested, even though they are (quote test description) "reachable from the window object".

Since we now have the ES5 function Object.getOwnPropertyNames implemented, it could be used in place of for..in; Object.getOwnPropertyNames returns both enumerable and non-enumerable properties. This will increase the test coverage, and as a bonus the test won't be sensitive to changes in the enumerable flag of existing properties (see https://bugs.webkit.org/show_bug.cgi?id=28771).
It should be OK to change the test like this because the test is made for checking reachability, not enumerability (i.e. the more, the merrier).

[Kent Hansen]

Created attachment 61516 [details]
Patch

Introduces a helper function, propertiesOnObject(), to compute an object's properties. Properties in the prototype chain are added as well, to correspond with for..in behavior. However, it stops at the built-in prototypes to avoid spewing out a bunch of "redundant" properties (e.g. including properties from Object.prototype for every visited object blows up the expected results immensely).

I've updated the gtk results even though this test is currently skipped on that platform (https://bugs.webkit.org/show_bug.cgi?id=30594).

[Sam Weinig]

Comment on attachment 61516 [details]
Patch

You may have other platforms that need to have there results updated.  We should really try and figure out what the differences are and and move those properties to their own tests.

[Kent Hansen]

(In reply to comment #2)
> (From update of attachment 61516 [details])
> You may have other platforms that need to have there results updated.  We should really try and figure out what the differences are and and move those properties to their own tests.

There are no existing *-expected.txt that I haven't updated, at least. The only port I haven't tested is Windows. The test is skipped on Chromium already (from test_expectations.txt: "This fails because we're missing various useless apple-specific properties on the window object").

The added properties due to the patch can be divided into four groups:
- ECMA DontEnum properties. These should always be there, regardless of features enabled.
- Properties of existing prototype objects (e.g. window.DOMException.prototype.toString). These are marked with attribute DontEnum in the IDL files, they're not apple-specific.
- property "length" on some constructors. This is due to somewhat of a quirk in the JS code generator. A length property is only created if the constructor expects > 0 arguments, so only constructors with e.g. ConstructorParameters=1 in the IDL, or some manually implemented classes (e.g. JSAudioConstructor), will have a length property. This is "apple" (JSC) specific AFAICT.
- styleMedia.constructor property. This is inherited from the StyleMedia prototype object, should always be there.

Regarding differences between ports (relative to Safari):
- Qt is missing some of the SVG constructors, adds TouchEvent constructor and webkitNotifications
- Gtk is missing BeforeLoadEvent, DomStringMap, Element.prototype.webkitMatchesSelector, Event.prototype.stopImmediatePropagation; accounts for a typo ("EventSourceContructor" :D ); a bunch of SVG stuff, ...

It would indeed be nice to split this and other "world tests" into smaller chunks that can be maintained and updated more easily; e.g. a separate test for SVG classes. This would mean using a static (generated?) list of classes rather than relying on property enumeration. There is some relevant discussion in https://bugs.webkit.org/show_bug.cgi?id=32596

[WebKit Commit Bot]

Comment on attachment 61516 [details]
Patch

Clearing flags on attachment: 61516

Committed r63645: <http://trac.webkit.org/changeset/63645>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.