[webkit-reviews] review granted: [Bug 173444] [WebIDL] Remove custom bindings for HTMLDocument : [Attachment 313128] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 173444: [WebIDL] Remove custom bindings for HTMLDocument
https://bugs.webkit.org/show_bug.cgi?id=173444

Attachment 313128: Patch

https://bugs.webkit.org/attachment.cgi?id=313128&action=review




--- Comment #30 from Darin Adler <darin at apple.com> ---
Comment on attachment 313128
  --> https://bugs.webkit.org/attachment.cgi?id=313128
Patch

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

Would be nice to line up our terminology with the specification even closer.
Can we use the term "responsible document" for example?

> Source/WebCore/ChangeLog:109
> +2017-06-15  Sam Weinig  <sam at webkit.org>

Double change log.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5344
> +	   AddToImplIncludes("Document.h");

Not sure I understand why this is required.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5352
> +	   AddToImplIncludes("Document.h");

Ditto.

> Source/WebCore/dom/Document.cpp:2663
> +    return;

Should not add this. I assume it’s a leftover from refactoring.

> Source/WebCore/dom/Document.cpp:7257
> +void Document::clear()
> +{
> +    // Per
https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-clear, this
method does nothing.
> +}
> +
> +void Document::captureEvents()
> +{
> +    // Per
https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-captureevents
, this method does nothing.
> +}
> +
> +void Document::releaseEvents()
> +{
> +    // Per
https://html.spec.whatwg.org/multipage/obsolete.html#dom-document-releaseevents
, this method does nothing.
> +}

inline?

> Source/WebCore/dom/Document.h:601
>      WEBCORE_EXPORT void open(Document* ownerDocument = nullptr);

Maybe rename this at some point? Even possible at that time that we might want
to rename openForBindings back to open.

> Source/WebCore/dom/Document.h:605
>      WEBCORE_EXPORT void close();

Maybe rename this at some point? Even possible at that time that we might want
to rename closeForBindings back to close.

> Source/WebCore/dom/Document.h:1335
> +    WEBCORE_EXPORT const AtomicString& linkColorForBindings() const;
> +    WEBCORE_EXPORT void setLinkColorForBindings(const String&);

Do we really need the existing linkColor and setLinkColor? Can we rename those
instead?