[webkit-reviews] review denied: [Bug 227872] Do not handle open attribute removal the same as HTMLDialogElement::close() : [Attachment 433412] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Tim Nguyen (:ntim) <ntim at apple.com>'s
request for review:
Bug 227872: Do not handle open attribute removal the same as
HTMLDialogElement::close()
https://bugs.webkit.org/show_bug.cgi?id=227872

Attachment 433412: Patch

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




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 433412
  --> https://bugs.webkit.org/attachment.cgi?id=433412
Patch

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

r- due to lack of testing.

> Source/WebCore/html/HTMLDialogElement.cpp:64
> +    // TODO(webkit.org/b/227537): Add steps 3 & 4

No TODO comments in WebKit

> Source/WebCore/html/HTMLDialogElement.cpp:81
> +    // FIXME(webkit.org/b/227907)

Bad format, please see https://webkit.org/code-style-guidelines/#comments.

> Source/WebCore/html/HTMLDialogElement.cpp:84
> +    // TODO(webkit.org/b/227537): Add steps 8 & 9

No TODO comments in WebKit: https://webkit.org/code-style-guidelines/#comments

> Source/WebCore/html/HTMLDialogElement.cpp:89
> +void HTMLDialogElement::close(const String& result)

Please explain in the change log the changes to this function as they seem
non-trivial.

> Source/WebCore/html/HTMLDialogElement.cpp:101
> +    // FIXME(webkit.org/b/227907)

This is not how we write FIXME comments:
https://webkit.org/code-style-guidelines/#comments. Please write a proper
sentence ending with a period.

> Source/WebCore/html/HTMLDialogElement.cpp:104
> +    // TODO(webkit.org/b/227537): Add step 6

No TODO comments in WebKit: https://webkit.org/code-style-guidelines/#comments.

> Source/WebCore/html/HTMLDialogElement.h:43
> +    const String& returnValue() { return m_returnValue; }

should be marked const.