The spec is being updated in https://github.com/whatwg/html/pull/6297 Tests (for confirm/prompt only) in https://github.com/web-platform-tests/wpt/pull/27435
Created attachment 419625 [details] Patch
Created attachment 419631 [details] Patch
Created attachment 419652 [details] Patch
Created attachment 419663 [details] Patch
Created attachment 419665 [details] Patch
Created attachment 419673 [details] Patch
Comment on attachment 419673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419673&action=review r=me > Source/WebCore/page/SecurityOrigin.cpp:471 > +bool SecurityOrigin::isSameOriginDomain(const SecurityOrigin& other) const I'm a little surprised that this needed to be a unique check, instead of just canAccess(). Is this check more strict or less strict? (If this check is more strict, it kinda doesn't make sense, since I could always do otherWindow.alert() as a workaround.)
(In reply to Geoffrey Garen from comment #7) > Comment on attachment 419673 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419673&action=review > > r=me > > > Source/WebCore/page/SecurityOrigin.cpp:471 > > +bool SecurityOrigin::isSameOriginDomain(const SecurityOrigin& other) const > > I'm a little surprised that this needed to be a unique check, instead of > just canAccess(). Is this check more strict or less strict? (If this check > is more strict, it kinda doesn't make sense, since I could always do > otherWindow.alert() as a workaround.) can-access is not really a spec concept. I was trying to stay as close to possible to the spec. I just looked at SecurityOrigin::canAccess() and the concepts are almost identical except that: 1. SecurityOrigin::canAccess() returns true when the m_universalAccess flag is set. 2. SecurityOrigin::canAccess() adds the following extra check for file URLs: (canAccess && isLocal()) canAccess = passesFileCheck(other); Despite those differences. They are basically the same thing. However, I do not like the fact that our naming does not match the spec. Maybe we should consider renaming SecurityOrigin::canAccess() to match the spec naming? Or have SecurityOrigin::isSameOriginDomain() call canAccess() internally?
FWIW in Blink we implement all same-origin domain checks using canAccess. (Which, I believe, has similar exceptional cases.) And, the mismatch between spec-naming and Blink code always bothers me too :).
(In reply to Domenic Denicola from comment #9) > FWIW in Blink we implement all same-origin domain checks using canAccess. > (Which, I believe, has similar exceptional cases.) And, the mismatch between > spec-naming and Blink code always bothers me too :). Thanks for the clarification. I see 3 ways to address this: 1. Rename canAccess() to isSameOriginDomain() 2. Add a new isSameOriginDomain() that calls canAccess() internally 3. Add a comment for canAccess() explaining that it implements the 'same origin-domain' concept in the HTML specification.
Created attachment 419747 [details] Patch
(In reply to Chris Dumez from comment #10) > (In reply to Domenic Denicola from comment #9) > > FWIW in Blink we implement all same-origin domain checks using canAccess. > > (Which, I believe, has similar exceptional cases.) And, the mismatch between > > spec-naming and Blink code always bothers me too :). > > Thanks for the clarification. I see 3 ways to address this: > 1. Rename canAccess() to isSameOriginDomain() > 2. Add a new isSameOriginDomain() that calls canAccess() internally > 3. Add a comment for canAccess() explaining that it implements the 'same > origin-domain' concept in the HTML specification. I would opt for number 1. Matching spec names is super helpful, and have fewer of these predicates is almost always better than having more.
(In reply to Sam Weinig from comment #12) > (In reply to Chris Dumez from comment #10) > > (In reply to Domenic Denicola from comment #9) > > > FWIW in Blink we implement all same-origin domain checks using canAccess. > > > (Which, I believe, has similar exceptional cases.) And, the mismatch between > > > spec-naming and Blink code always bothers me too :). > > > > Thanks for the clarification. I see 3 ways to address this: > > 1. Rename canAccess() to isSameOriginDomain() > > 2. Add a new isSameOriginDomain() that calls canAccess() internally > > 3. Add a comment for canAccess() explaining that it implements the 'same > > origin-domain' concept in the HTML specification. > > I would opt for number 1. Matching spec names is super helpful, and have > fewer of these predicates is almost always better than having more. Yes, I prefer 1. too. I went with 3. in this patch to keep things small and I would be happy to rename the function in a follow-up if everyone agrees.
Committed r272607: <https://commits.webkit.org/r272607> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419747 [details].
<rdar://problem/74206634>
This change was reverted via Bug 229737 due to breakage.