Window's named properties should be exposed on a WindowProperties object in its prototype

[Chris Dumez]

Window's named properties should be exposed on a WindowProperties object in its prototype:
- http://heycam.github.io/webidl/#named-properties-object

Firefox and Chrome both comply with the specification. However, in WebKit, there is no "WindowProperties" object in the Window prototype chain and the named properties are exposed on the Window object itself.

[Chris Dumez]

Created attachment 284892 [details]
WIP Patch

[Chris Dumez]

Created attachment 284897 [details]
Patch

[Gavin Barraclough]

Comment on attachment 284897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284897&action=review

r- for index getter precedence.

> Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:41
> +static bool jsDOMWindowPropertiesGetOwnPropertySlotNamedItemGetter(JSDOMWindowProperties* thisObject, Frame& frame, ExecState* exec, PropertyName propertyName, PropertySlot& slot)

Given comments below, you may just be able to fold this in to getOwnPropertySlot.

> Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:87
> +    if (thisObject->classInfo() == info() && frame)

I think the info part of this check is redundant – isn't think checking the same thing as ASSERT_GC_OBJECT_INHERITS above?

> Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:99
> +    if (thisObject->classInfo() == info() && frame)

Ditto – info check appears to be redundant.

> Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:100
> +        return jsDOMWindowPropertiesGetOwnPropertySlotNamedItemGetter(thisObject, *frame, state, propertyName, slot);

getOwnPropertySlotByIndex should be the same as getOwnPropertySlot - own properties should take precedence, then you should check for presence on the prototype before allowing named getters.
I'd probably just make getOwnPropertySlotByIndex call getOwnPropertySlot.

> LayoutTests/fast/dom/Window/es52-globals-expected.txt:5
> +FAIL window.hasOwnProperty("div") should be true. Was false.

Maybe should keep these but also add checks for window.__proto__.__proto.hasOwnProperty("f") (I think it's the proto-proto its' now on?)

[ Or more standardsy: Object.getPrototypeOf(Object.getPrototypeOf(window)).hasOwnProperty("f") ]

[Chris Dumez]

Created attachment 284917 [details]
Patch

[Chris Dumez]

Comment on attachment 284917 [details]
Patch

Clearing flags on attachment: 284917

Committed r203935: <http://trac.webkit.org/changeset/203935>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 160596

[Chris Dumez]

Comment on attachment 284917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284917&action=review

> Source/WebCore/bindings/js/JSDOMWindowProperties.h:61
> +    Ref<DOMWindow> m_window;

Assuming the memory regression is real, I suspect the Ref'ing of the DOMWindow here is a bad thing. I don't think we really need it either. We can probably just get the window from the JSGlobalObject when needed.

[Chris Dumez]

Created attachment 285429 [details]
Patch

[Chris Dumez]

Comment on attachment 285429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285429&action=review

> Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:87
> +    auto& window = jsCast<JSDOMWindowBase*>(thisObject->globalObject())->wrapped();

I got rid of the m_window data member (which was a string Ref to DOMWindow) and access the Window via the global object now.

[Chris Dumez]

Comment on attachment 285429 [details]
Patch

Clearing flags on attachment: 285429

Committed r204179: <http://trac.webkit.org/changeset/204179>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.