http://dom.spec.whatwg.org/#document Make Document constructable so that one can do "new Document" instead of "document.implementation.createHTMLDocument('')"
Created attachment 222642 [details] Patch
Comment on attachment 222642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222642&action=review > Source/WebCore/dom/Document.h:262 > + return adoptRef(new Document(0, context.url())); Please use nullptr instead of 0 here. I don’t see why context.url() would be correct for this document. I think we need to make a document without a URL here. DOMImplementation.createDocument also sets the security origin on the document it creates. I suspect we need to do the same here. I think we need some test coverage that goes beyond “this is a document” and checks these other properties of the document we create.
The things we need to test are the things that the standard requires: - The document has an origin which is an alias to the origin of the global object's associated document. Need to test that either directly or indirectly. - The document has an effective script origin which is an alias to the effective script origin of the global object's associated document. Need to test that either directly or indirectly.
(In reply to comment #3) > The things we need to test are the things that the standard requires: > > - The document has an origin which is an alias to the origin of the global object's associated document. Need to test that either directly or indirectly. > > - The document has an effective script origin which is an alias to the effective script origin of the global object's associated document. Need to test that either directly or indirectly. Thanks for your comments. Can you help me a bit more with testing? I'm not sure how to test these correctly. Should we check these steps? http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#sandboxOrigin
(In reply to comment #4) > (In reply to comment #3) > > The things we need to test are the things that the standard requires: > > > > - The document has an origin which is an alias to the origin of the global object's associated document. Need to test that either directly or indirectly. > > > > - The document has an effective script origin which is an alias to the effective script origin of the global object's associated document. Need to test that either directly or indirectly. > > Thanks for your comments. Can you help me a bit more with testing? I'm not sure how to test these correctly. Should we check these steps? http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#sandboxOrigin No, those steps are the algorithm the engine is supposed to follow. But a test needs to be based on the observable results of the algorithm, not the algorithm itself. We need to figure out how to detect if a document has an incorrect origin or an incorrect effective script origin by the behavior of the engine. I presume that the effect of the effective script origin is to restrict script execution, so that test would be about correct restricting script execution and not incorrectly restricting it. The other test could perhaps be done simply by reading a property that indicates what a document’s origin is. I’m not really sure, because I’m not an expert on this part of the specification. I’d have to study it myself to figure that out.
Created attachment 223958 [details] Patch
Comment on attachment 223958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=223958&action=review Does the test fail if you remove the line of code that says setSecurityOrigin? > Source/WebCore/dom/Document.cpp:565 > + document->setSecurityOrigin(adoptRef(context.securityOrigin())); The adoptRef here is incorrect. This will result in a reference count underflow.
(In reply to comment #7) > (From update of attachment 223958 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223958&action=review > > Does the test fail if you remove the line of code that says setSecurityOrigin? yes
Created attachment 224054 [details] Patch
Comment on attachment 224054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224054&action=review > Source/WebCore/dom/Document.cpp:566 > + return document; Oh, I just realized this line should read document.release() to avoid a tiny bit of needless reference count churn. Probably will let this go into the commit queue anyway. We can follow up with a 1-line patch to fix that.
Comment on attachment 224054 [details] Patch Clearing flags on attachment: 224054 Committed r164036: <http://trac.webkit.org/changeset/164036>
All reviewed patches have been landed. Closing bug.
(In reply to comment #10) > (From update of attachment 224054 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224054&action=review > > > Source/WebCore/dom/Document.cpp:566 > > + return document; > > Oh, I just realized this line should read document.release() to avoid a tiny bit of needless reference count churn. Probably will let this go into the commit queue anyway. We can follow up with a 1-line patch to fix that. It's fixed. Committed in https://trac.webkit.org/r164099
This change added document.origin to WebKit. There's a discussion on blink-dev about removing it from Blink in favor of self.origin that may be of interest: https://groups.google.com/a/chromium.org/d/msg/blink-dev/CO52Bt15cuc/ya6u_devCQAJ
(In reply to comment #14) > This change added document.origin to WebKit. There's a discussion on > blink-dev about removing it from Blink in favor of self.origin that may be > of interest: > https://groups.google.com/a/chromium.org/d/msg/blink-dev/CO52Bt15cuc/ > ya6u_devCQAJ Well, we don't implement self.origin, so I do not think we can get rid of document.origin anytime soon. Also note that we recently aligned our implementation of document.origin with the specification and Blink.