[webkit-reviews] review granted: [Bug 227493] Support <dialog> element close event : [Attachment 432575] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Tim Nguyen (:ntim)
<ntim at apple.com>'s request for review:
Bug 227493: Support <dialog> element close event
https://bugs.webkit.org/show_bug.cgi?id=227493

Attachment 432575: Patch

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 432575
  --> https://bugs.webkit.org/attachment.cgi?id=432575
Patch

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

> Source/WebCore/html/HTMLDialogElement.h:28
> +#include "EventSender.h"

You can forward declare EventSender with

template<typename T> class EventSender;

so you don't need this include in the header (slightly reducing header bloat
and so improving compile times). See the other EventSenders.

> Source/WebCore/html/HTMLDialogElement.h:33
> +typedef EventSender<HTMLDialogElement> DialogEventSender;

Modern replacement for typedef is using:

using DialogEventSender = EventSender<HTMLDialogElement>;

I find it more readable.

> Source/WebCore/html/HTMLDialogElement.h:57
> -    void toggleOpen();
> +    void toggleOpen(bool newValue);

This should be renamed to setOpen since it is now a setter rather than a
toggle.
No need for parameter name, it is obvious from context.