[webkit-reviews] review granted: [Bug 120849] Add support for BeforeUnloadEvent : [Attachment 211046] Attempt to fix mac build

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Christophe Dumez <dchris at gmail.com>'s
request for review:
Bug 120849: Add support for BeforeUnloadEvent
https://bugs.webkit.org/show_bug.cgi?id=120849

Attachment 211046: Attempt to fix mac build
https://bugs.webkit.org/attachment.cgi?id=211046&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211046&action=review


> LayoutTests/ChangeLog:3
> +	   Add support for BeforeUnloadEvent

I don’t understand the name of this patch. We already have a beforeunload
event. I’m assuming this patch fixes something about it. But it’s titled “add
support”. Could you clarify?

> Source/WebCore/ChangeLog:19
> +	   BeforeUnloadEvent and returnValue are already supported by IE and
Firefox. Previously,
> +	   Blink was passing a base Event type to the beforeunload event
handlers instead of
> +	   a BeforeUnloadEvent.

Do you mean WebKit here, or are you telling us about Blink for some reason?

> Source/WebCore/dom/BeforeUnloadEvent.cpp:6
> + * Copyright (C) 2013 Samsung Electronics

Seems strange to add copyright when the change to the file is so small.

> Source/WebCore/dom/BeforeUnloadEvent.h:32
> +class BeforeUnloadEvent : public Event {

Should mark this class FINAL.