Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making Location.ancestorOrigins a FrozenArray is not Web-Compatible #2179

Closed
cdumez opened this issue Dec 14, 2016 · 5 comments
Closed

Making Location.ancestorOrigins a FrozenArray is not Web-Compatible #2179

cdumez opened this issue Dec 14, 2016 · 5 comments

Comments

@cdumez
Copy link

cdumez commented Dec 14, 2016

https://html.spec.whatwg.org/#the-location-interface:dom-location-reload

[Unforgeable, SameObject] readonly attribute FrozenArray<USVString> ancestorOrigins;

Using a FrozenArray instead of a DOMStringList here does not seem to be Web-Compatible. It breaks notifications on Google.com: If you have notifications to view and you click on the notifications area, they do not show.

Google.com uses the following JS code:
(c = c.location.ancestorOrigins) && 0 < c.length && (c = c.item(0), !/\/plus.google.com$/.test(c) && Vaa.test(c) && 1 == ne && (this.M5a.orig = c));

Error: “Exception with thrown value: TypeError: c.item is not a function. (In 'c.item(0)', 'c.item' is undefined)”
-> c.item() works on a DOMStringList but not on a JS Array.

Note that Google Chrome is still using a DOMStringList. We had switched to a FrozenArray in WebKit but we will have to revert to a DOMStringList.

@cdumez
Copy link
Author

cdumez commented Dec 14, 2016

@domenic @rniwa

@domenic
Copy link
Member

domenic commented Dec 14, 2016

Wow, thanks for taking the bullet and finding that for us. This speaks to the need for FrozenArrayWithItemAndContains, which we've contemplated for a while, but probably we should just switch back to DOMStringList for now? Dang, I thought we'd confined that to IndexedDB.

@cdumez
Copy link
Author

cdumez commented Dec 14, 2016

FYI, reverting in WebKit via https://bugs.webkit.org/show_bug.cgi?id=165872

hubot pushed a commit to WebKit/WebKit-http that referenced this issue Dec 15, 2016
REGRESSION (204679): Google notifications never load (expecting DOMStringList rather than JS array for Location.ancestorOrigins)
<rdar://problem/29573563>
https://bugs.webkit.org/show_bug.cgi?id=165872

Reviewed by Chris Dumez.

Revert the Location.ancestorOrigins part of r204679 because google.com is relying on
it returning a DOMStringList (or at least something with a .item() function), rather
than a frozen javascript array.
        
Spec changes are tracked with whatwg/html#2179.

* page/Location.cpp:
(WebCore::Location::ancestorOrigins):
* page/Location.h:
* page/Location.idl:
Change Location.ancestorOrigins back to returning a DOMStringList.

Source/WebKit2:
[WebIDL] Add support for converting dictionaries to JS
https://bugs.webkit.org/show_bug.cgi?id=165367

Reviewed by Chris Dumez.

* CMakeLists.txt:
Add missing directories to look in for headers.

LayoutTests:
REGRESSION (204679): Google notifications never load (expecting DOMStringList rather than JS array for Location.ancestorOrigins)
<rdar://problem/29573563>
https://bugs.webkit.org/show_bug.cgi?id=165872

Reviewed by Chris Dumez.

* fast/dom/Window/Location/ancestor-origins-expected.txt:
* fast/dom/Window/Location/ancestor-origins.html:
Change back to test that Location.ancestorOrigins returns a DOMStringList.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@209841 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@annevk
Copy link
Member

annevk commented Dec 15, 2016

Rather than FrozenArrayWithItemAndContains should we just admit defeat and add DOMStringList back as a thing? (And just say folks ought not to use it, like we do for NodeList.)

@domenic
Copy link
Member

domenic commented Dec 15, 2016

Yeah, that's probably easier. It's not as good for authors, since they can't use any array methods (e.g. forEach). But it's not the most important rough edge in the platform to fix, I guess.

annevk added a commit that referenced this issue Dec 17, 2016
This requires several follow-up changes:

* To DOM, so it’s no longer marked obsolete.
* To Indexed DB, to start making use of this definition and remove its
own version.
* To Service Workers, which can no longer use “ancestor origins array”.

Fixes #2179.
zcorpan pushed a commit that referenced this issue Jan 13, 2017
This requires several follow-up changes:

* To DOM, so it’s no longer marked obsolete.
* To Indexed DB, to start making use of this definition and remove its
own version.
* To Service Workers, which can no longer use “ancestor origins array”.

Fixes #2179.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This requires several follow-up changes:

* To DOM, so it’s no longer marked obsolete.
* To Indexed DB, to start making use of this definition and remove its
own version.
* To Service Workers, which can no longer use “ancestor origins array”.

Fixes whatwg#2179.
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
REGRESSION (204679): Google notifications never load (expecting DOMStringList rather than JS array for Location.ancestorOrigins)
<rdar://problem/29573563>
https://bugs.webkit.org/show_bug.cgi?id=165872

Reviewed by Chris Dumez.

Revert the Location.ancestorOrigins part of r204679 because google.com is relying on
it returning a DOMStringList (or at least something with a .item() function), rather
than a frozen javascript array.

Spec changes are tracked with whatwg/html#2179.

* page/Location.cpp:
(WebCore::Location::ancestorOrigins):
* page/Location.h:
* page/Location.idl:
Change Location.ancestorOrigins back to returning a DOMStringList.

Source/WebKit2:
[WebIDL] Add support for converting dictionaries to JS
https://bugs.webkit.org/show_bug.cgi?id=165367

Reviewed by Chris Dumez.

* CMakeLists.txt:
Add missing directories to look in for headers.

LayoutTests:
REGRESSION (204679): Google notifications never load (expecting DOMStringList rather than JS array for Location.ancestorOrigins)
<rdar://problem/29573563>
https://bugs.webkit.org/show_bug.cgi?id=165872

Reviewed by Chris Dumez.

* fast/dom/Window/Location/ancestor-origins-expected.txt:
* fast/dom/Window/Location/ancestor-origins.html:
Change back to test that Location.ancestorOrigins returns a DOMStringList.


Canonical link: https://commits.webkit.org/183485@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@209841 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants