Bug 33948

Created attachment 47129 [details] Proposed patch

Comment on attachment 47129 [details] Proposed patch > + var names = Object.getOwnPropertyNames(o); > + for (var i = 0; i < names.length; ++i) > + propertySet[names[i]] = true; When the test is written this way, the results are unpredictable because the order property names come out of getOwnPropertyNames is unpredictable and depends on hash table ordering. This creates a fragile test with results that get shuffled around any time someone adds or removes a property or changes the hash algorithm. Instead I suggest we have a test that sorts the names so things are in a predictable order. See tests like window-properties.html for an example of how this has been done in the past.

Comment on attachment 47129 [details] Proposed patch r=me for the code, but your test is liekly to be unstable, possibly just do a simple sort on the output of getOwnPropertyNames? I think additionally your test should test to ensure that you still see own properties.

(In reply to comment #3) > (From update of attachment 47129 [details]) > r=me for the code, but your test is liekly to be unstable, possibly just do a > simple sort on the output of getOwnPropertyNames? I think additionally your > test should test to ensure that you still see own properties. Agreed on the sorting, I didn't think of it until I was on the subway home. I did set out to write a "positive" test that uses getOwnPropertyNames() and verifies that those properties have descriptors, but then I'll need a property exclusion list similar to the one in window-properties.html to keep the test robust, and I'd prefer not to copy&paste that code or have to do test refactoring in this change (I have a different task for refactoring those tests already, https://bugs.webkit.org/show_bug.cgi?id=32596). Well, I could create a task "Add getOwnPropertyDescriptor test for window object" and make it a blocker for this task.

Created attachment 47187 [details] Updated patch (more elaborate test) Test properties that should have descriptors as well as those that should not.

Comment on attachment 47187 [details] Updated patch (more elaborate test) Clearing flags on attachment: 47187 Committed r53706: <http://trac.webkit.org/changeset/53706>

All reviewed patches have been landed. Closing bug.

This broke Leopard: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r53706%20(9644)/fast/dom/Window/window-property-descriptors-pretty-diff.html Looks like it needs new baselines.

Looks like there was an attempt to fix this in r53738

(In reply to comment #9) > Looks like there was an attempt to fix this in r53738 Fix is good, sorry for the breakage. Adding "debug vs release" to my Things-to-keep-in-mind-before-submitting-tests checklist. :)

Has this been rolled out? there aren't comments to suggest that it was, but it's also been reopened implying it was rolled out.

(In reply to comment #11) > Has this been rolled out? there aren't comments to suggest that it was, but > it's also been reopened implying it was rolled out. Nope.

The tests aren't failing anymore (the fix Eric pointed to also fixed Windows Debug). Can we close this task again?

I don't see any reason to keep it open.