There's a lot going on here, mixed up in an inconsistent order, with duplication between (& within!) functions. Organizing this a little better will make further behavioral changes easier.
Created attachment 271303 [details] WIP
Comment on attachment 271303 [details] WIP Attachment 271303 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/829020 New failing tests: fast/dom/Window/get-set-properties.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Created attachment 271304 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271303 [details] WIP Attachment 271303 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/829034 New failing tests: fast/dom/Window/get-set-properties.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Created attachment 271305 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 271303 [details] WIP Attachment 271303 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/829026 New failing tests: fast/dom/Window/get-set-properties.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Created attachment 271306 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 271307 [details] Fix
Comment on attachment 271307 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271307&action=review Nice clean up. > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:96 > + // Allow access to toString() cross-domain, but always Object.prototype.toString. We now allow toString() for frameless windows, is this intentional? > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:214 > + Document* document = frame.document(); auto* > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:216 > + HTMLDocument& htmlDocument = downcast<HTMLDocument>(*document); auto& > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:217 > + AtomicStringImpl* atomicPropertyName = propertyName.publicName(); auto* > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:223 > + namedItem = toJS(exec, thisObject->globalObject(), WTF::getPtr(collection)); We prefer collection.ptr(). WTF::getPtr() is mostly used in the generated bindings where we don't know what type we are getting as input. > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:242 > + Optional<unsigned> index = parseIndex(propertyName); Could be inside the if() > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:246 > + JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(object); auto* > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247 > + Frame* frame = thisObject->wrapped().frame(); auto* > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:323 > + Frame* frame = thisObject->wrapped().frame(); auto*
(In reply to comment #9) > Comment on attachment 271307 [details] > Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271307&action=review > > Nice clean up. > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:96 > > + // Allow access to toString() cross-domain, but always Object.prototype.toString. > > We now allow toString() for frameless windows, is this intentional? Yes –there was this old FIXME: // FIXME: This doesn't fully match Firefox, which allows at least toString in addition to those. So I made it so. Would be easy switch if we wanted. In general, it seems crazy to me that we have a whole third path handling frameless access, and that the properties just magically disappear off the global object when the frame is closed. We probably don't gain anything anyway (since functions & accessors can now all be pulled off into local vars, so I doubt this buys us anything anymore. If not spec defined, I think we should probably consider removing this behviour. > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:214 > > + Document* document = frame.document(); > > auto* > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:216 > > + HTMLDocument& htmlDocument = downcast<HTMLDocument>(*document); > > auto& > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:217 > > + AtomicStringImpl* atomicPropertyName = propertyName.publicName(); > > auto* > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:223 > > + namedItem = toJS(exec, thisObject->globalObject(), WTF::getPtr(collection)); > > We prefer collection.ptr(). WTF::getPtr() is mostly used in the generated > bindings where we don't know what type we are getting as input. > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:242 > > + Optional<unsigned> index = parseIndex(propertyName); > > Could be inside the if() > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:246 > > + JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(object); > > auto* > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247 > > + Frame* frame = thisObject->wrapped().frame(); > > auto* > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:323 > > + Frame* frame = thisObject->wrapped().frame(); > > auto* All fixed. Committed revision 196583.