Bug 83493 – Implement Location.ancestorOrigins

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Implement Location.ancestorOrigins

[Adam Barth]

Created attachment 136276 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 136276 [details]
Patch

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

So this would be syntactic sugar if pages could access window.parent.location.origin, correct?  Based on in-person discussion you suggested that although allowing access to such would be possible, it would be an odd security check.

You also mentioned that this approach encouraged developers to check all of them, instead of just randomly parent or top (which I'm not sure I buy).

> Source/WebCore/page/Location.idl:67
> +        DOMStringList ancestorOrigins();

I was under the (possibly false) impression that we were trying to kill the DOMStringList uses and make them all Array instead?

Why would you want this to be a function instead of a readonly property?

[David Levin]

Comment on attachment 136276 [details]
Patch

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

> Source/WebCore/page/Location.cpp:132
> +    for (Frame* frame = m_frame->tree()->parent(); frame; frame = frame->tree()->parent())

Should this be parent(true) ?
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/FrameTree.cpp&type=cs&l=61

(where true means checkForDisconnectedFrame)

And if so, please improve the test to catch this case (-- ideally for places where tree()->parent() is called).

[Adam Barth]

> You also mentioned that this approach encouraged developers to check all of them, instead of just randomly parent or top (which I'm not sure I buy).

See Jonas' email <http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-April/035344.html>.  He also mentions that this aligns with how CORS presents a list of origins.

> > Source/WebCore/page/Location.idl:67
> > +        DOMStringList ancestorOrigins();
> 
> I was under the (possibly false) impression that we were trying to kill the DOMStringList uses and make them all Array instead?

I'm happy to change this to a sequence<DOMString> if you'd prefer.

> Why would you want this to be a function instead of a readonly property?

The issue is that that we don't want this to be a "live" list of DOMStrings, which means you get a fresh copy each time you read the property.  Given that, we'd have location.ancestorOrigins !== location.ancestorOrigins, which is slightly odd.

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 136276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136276&action=review
> 
> > Source/WebCore/page/Location.cpp:132
> > +    for (Frame* frame = m_frame->tree()->parent(); frame; frame = frame->tree()->parent())
> 
> Should this be parent(true) ?

As far as I can tell Frame::setIsDisconnected is never called, so I don't think that does anything anymore.  I'll post a separate patch to remove it.

[David Levin]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 136276 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136276&action=review
> > 
> > > Source/WebCore/page/Location.cpp:132
> > > +    for (Frame* frame = m_frame->tree()->parent(); frame; frame = frame->tree()->parent())
> > 
> > Should this be parent(true) ?
> 
> As far as I can tell Frame::setIsDisconnected is never called, so I don't think that does anything anymore.  I'll post a separate patch to remove it.

fwiw, https://lists.webkit.org/pipermail/webkit-dev/2009-April/007400.html

So I don't think you can write a test after all.

[Adam Barth]

Wow, that's a terrible idea.

[Ojan Vafai]

(In reply to comment #4)
> > > Source/WebCore/page/Location.idl:67
> > > +        DOMStringList ancestorOrigins();
> > 
> > I was under the (possibly false) impression that we were trying to kill the DOMStringList uses and make them all Array instead?
> 
> I'm happy to change this to a sequence<DOMString> if you'd prefer.

Yes please. We're trying to kill all the arbitrary *List types that are just dumbed down arrays. So far, the only one we're consciously keeping is NodeList because we want to add new methods to that interface that operate on the list of Nodes (a la jQuery).

[Ian 'Hixie' Hickson]

Note that it doesn't matter if this array is live or not, since the contents of the array can't change.

[WebKit Review Bot]

Comment on attachment 136276 [details]
Patch

Attachment 136276 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12370461

New failing tests:
fast/dom/Window/window-appendages-cleared.html

[WebKit Review Bot]

Created attachment 136336 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Created attachment 136361 [details]
Addressed feedback except DOMStringList

[David Levin]

Comment on attachment 136361 [details]
Addressed feedback except DOMStringList

It will address a big need to get this landed.

I know there is a desire to have a real array behind this, but it sounds like this can be fixed up later, and it won't be too hard in the near future (Perhaps with something like this http://trac.webkit.org/changeset/113931).

[Eric Seidel (no email)]

I predict that some browser (including possibly WebKit) will eventually make parent.location.origin visible between frames, and then this API will just seem redundant and vestigial.  It's just syntactic sugar that some JS library should provide, IMO.

[Eric Seidel (no email)]

It's also unclear to me what the current "best practice" is for this type of new API.  Should it be webkitAncestorOrigins until it's written down in some spec?

[Adam Barth]

Now you're really trolling!  I think Hixie's going to add it to the HTML spec somewhat soon, and I don't expect it to change much, so I don't think there's much value in adding a vendor prefix.

[WebKit Review Bot]

Comment on attachment 136361 [details]
Addressed feedback except DOMStringList

Clearing flags on attachment: 136361

Committed r113945: <http://trac.webkit.org/changeset/113945>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ian 'Hixie' Hickson]

Yeah, don't prefix, I'll make sure to either match implementations or not conflict on names when I spec this (with preference towards matching implementations).

[Malte Ubl]

Thanks for doing this! It seems it does not fix an important use case which triggers most of the cross domain violation error messages for us:

Imagine you are an iframe and you have siblings. Some of them are on the same origin. You can find them like this

for (var i = 0; i < parent.frames.length; i++) {
  try {
    var siblingDoc = parent.frames[i].document;
  } catch(e) {}
  if (siblingDoc) {
    // Yaihh, found same origin sibling!
  }
}

This triggers the magic v8/JSC error message that I think Location.ancestorOrigins was mostly designed to get around and unless I am missing something Location.ancestorOrigins does not help at all with this, right?

[Adam Barth]

> Location.ancestorOrigins does not help at all with this, right?

Correct.  It doesn't help for siblings.