Created attachment 188275 [details] Patch

Comment on attachment 188275 [details] Patch Attachment 188275 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16580248 New failing tests: http/tests/security/cross-frame-access-location-get.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/security/cross-frame-access-location-get-override.html http/tests/security/xss-DENIED-defineProperty.html http/tests/misc/location-replace-crossdomain.html

Created attachment 188296 [details] Patch

Comment on attachment 188296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188296&action=review > Source/WebCore/page/Location.idl:54 > [Custom, V8Unforgeable] void assign(in [Optional=DefaultIsUndefined] DOMString url); > [Custom, V8Unforgeable] void replace(in [Optional=DefaultIsUndefined] DOMString url); > [Custom, V8Unforgeable] void reload(); > +#if defined(V8_BINDING) && V8_BINDING > + [Custom, V8Unforgeable, DoNotCheckSecurity] readonly attribute any assign; > + [Custom, V8Unforgeable, DoNotCheckSecurity] readonly attribute any replace; > + [Custom, V8Unforgeable, DoNotCheckSecurity] readonly attribute any reload; > +#endif It's really strange to have both functions and attributes with the same name....

I'm not sure having both functions and attributes with the same name is the right thing to do. If you think it's the right thing, can you add more explanation as to why?

(In reply to comment #5) > I'm not sure having both functions and attributes with the same name is the right thing to do. If you think it's the right thing, can you add more explanation as to why? I don't think it's a right thing to do, but V8 currently support it (i.e. my patch doesn't change the current behavior of V8). For example, http/tests/security/cross-frame-access-location-get-override.html depends on the fact that assign/reload/replace work as attributes. JSC has only functions for these names, which looks correct to me.

What is the difference between having an attribute and having a function with the same name? I though that they both are implemented as properties... Maybe one is on the prototype and one is on the instance?

(In reply to comment #7) > What is the difference between having an attribute and having a function with the same name? Maybe one is on the prototype and one is on the instance? No. Given that [V8Unforgeable] is set on the reload() method, both the method and the attribute are defined on the instance. (In terms of implementation, a method is registered by Set(), and an attribute is registered by SetAccessor().) I have no idea about why we need both. Let me try to remove the attribute and see what test fails.

Created attachment 189161 [details] Patch

dglazkov, mike: Attributes for Location::reload, Location::assign and Location::replace were introduced in http://trac.webkit.org/changeset/41693. It looks strange to have both attributes and methods for these Location properties. Would it make sense to remove the attributes?

Comment on attachment 189161 [details] Patch Attachment 189161 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16640014 New failing tests: http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/security/cross-frame-access-location-put.html http/tests/security/cross-frame-access-location-get.html http/tests/security/xss-DENIED-defineProperty.html http/tests/misc/location-replace-crossdomain.html http/tests/security/cross-frame-access-location-get-override.html

Comment on attachment 189161 [details] Patch Looks like I need to work through more.

V8 is gone from WebKit.