[webkit-reviews] review granted: [Bug 53611] Propagate security origin of parent document into HTML documents created with DOMImplementation : [Attachment 82152] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted anton muhin
<antonm at chromium.org>'s request for review:
Bug 53611: Propagate security origin of parent document into HTML documents
created with DOMImplementation
https://bugs.webkit.org/show_bug.cgi?id=53611

Attachment 82152: Patch
https://bugs.webkit.org/attachment.cgi?id=82152&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82152&action=review

> if you'd prefer to have a separate test, so it'll be.

It's good to have tests in one file when they are clearly independent (output
is clearly separated, there is no conceivable way the tests could affect each
other by being on one page, and you can comment out all subtests but one for
easier debugging). When that's not true, separate tests make it easier to
figure out what happened when they fail.

r=me with comments addressed.

> LayoutTests/fast/dom/gc-9.html:90
> +  impl.createHTMLDocument('');  // May crash or throw an excepiton if we
collect parent document of impl.

Typo: exception.

> Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:5
> + * modify it under the terms of the GNU Library General Public

Please consider using a BSD license for new code.

> Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:34
> +    Document* doc = domImplementation->ownerDocument();

Please don't abbreviate "ownerDocument".

> Source/WebCore/bindings/js/JSDOMImplementationCustom.cpp:39
> +} // namespace WebCore

I'm not a fan of these comments - they don't guarantee that they are true if
you're wondering about that (you'll use your editor to find matching brace if
you really need to check what's wrong with nesting), but most of the time, they
just add visual noise.

> Source/WebCore/dom/DOMImplementation.h:48
> +    PassRefPtr<DocumentType> createDocumentType(const String& qualifiedName,
const String& publicId, const String &systemId, ExceptionCode&);

As you're touching this code already, please move "&" to a correct position.

> Source/WebCore/dom/DOMImplementation.h:72
> +    template <class T>
> +    PassRefPtr<T> setSecurityOrigin(PassRefPtr<T> doc);

I could suggest not using a template, and spelling out "document". But it would
probably be best to just inline this function in the only place it's used.