Bug 42258 - Improve coverage of fast/dom/Window/window-properties test
Summary: Improve coverage of fast/dom/Window/window-properties test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Kent Hansen
URL:
Keywords:
Depends on:
Blocks: 28771
  Show dependency treegraph
 
Reported: 2010-07-14 07:10 PDT by Kent Hansen
Modified: 2010-07-19 01:23 PDT (History)
1 user (show)

See Also:


Attachments
Patch (45.10 KB, patch)
2010-07-14 07:27 PDT, Kent Hansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Hansen 2010-07-14 07:10:14 PDT
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).
Comment 1 Kent Hansen 2010-07-14 07:27:09 PDT
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).
Comment 2 Sam Weinig 2010-07-15 22:19:36 PDT
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.
Comment 3 Kent Hansen 2010-07-16 05:39:10 PDT
(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
Comment 4 WebKit Commit Bot 2010-07-19 01:23:51 PDT
Comment on attachment 61516 [details]
Patch

Clearing flags on attachment: 61516

Committed r63645: <http://trac.webkit.org/changeset/63645>
Comment 5 WebKit Commit Bot 2010-07-19 01:23:55 PDT
All reviewed patches have been landed.  Closing bug.