Bug 120849 – Add support for BeforeUnloadEvent interface

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Consider merging the following patch from Blink:
Add support for BeforeUnloadEvent as per the specification:
http://www.whatwg.org/specs/web-apps/current-work/#beforeunloadevent

BeforeUnloadEvent has a returnValue attribute. Setting returnValue to a non-empty
string in an event handler causes the user agent should ask the user to confirm
that they wish to unload the document. This is equivalent to returning a non-empty
string in the EventHandler:
http://www.whatwg.org/specs/web-apps/current-work/#onbeforeunloadeventhandler

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.

Note that this patch also deprecates Event.returnValue. This used to be an IE
extension but this is no longer supported by IE (nor Firefox). The standard
preventDefault() should be used instead (supported in IE >= 9).

Revision:
https://src.chromium.org/viewvc/blink?revision=156916&view=revision

[Chris Dumez]

Created attachment 211027 [details]
WIP Patch

[Build Bot]

Comment on attachment 211027 [details]
WIP Patch

Attachment 211027 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1732350

[Chris Dumez]

Created attachment 211029 [details]
Attempt to fix mac build

[Build Bot]

Comment on attachment 211029 [details]
Attempt to fix mac build

Attachment 211029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1735239

[Build Bot]

Comment on attachment 211029 [details]
Attempt to fix mac build

Attachment 211029 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1732359

[Chris Dumez]

Created attachment 211031 [details]
Attempt to fix mac build

[Build Bot]

Comment on attachment 211031 [details]
Attempt to fix mac build

Attachment 211031 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1732368

[Chris Dumez]

Created attachment 211033 [details]
Attempt to fix mac build

[Build Bot]

Comment on attachment 211033 [details]
Attempt to fix mac build

Attachment 211033 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1729390

[Chris Dumez]

Created attachment 211036 [details]
Attempt to fix mac build

[Build Bot]

Comment on attachment 211036 [details]
Attempt to fix mac build

Attachment 211036 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1733335

[Chris Dumez]

Created attachment 211043 [details]
Attempt to fix mac build

[Build Bot]

Comment on attachment 211043 [details]
Attempt to fix mac build

Attachment 211043 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1734325

[Build Bot]

Comment on attachment 211043 [details]
Attempt to fix mac build

Attachment 211043 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1735274

[Chris Dumez]

Created attachment 211046 [details]
Attempt to fix mac build

[Chris Dumez]

Comment on attachment 211046 [details]
Attempt to fix mac build

Green bubbles, finally.

[Darin Adler]

Comment on attachment 211046 [details]
Attempt to fix mac build

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.

[Darin Adler]

Comment on attachment 211046 [details]
Attempt to fix mac build

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?

Sorry, I meant to delete this comment. I understand this. We are adding the class, not the event. There was a moment where I did not understand.

[Chris Dumez]

Comment on attachment 211046 [details]
Attempt to fix mac build

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?
> 
> Sorry, I meant to delete this comment. I understand this. We are adding the class, not the event. There was a moment where I did not understand.

Renaming to "Add support for BeforeUnloadEvent interface" to make it a little bit clearer but I am adding.

>> Source/WebCore/ChangeLog:19
>> +        a BeforeUnloadEvent.
> 
> Do you mean WebKit here, or are you telling us about Blink for some reason?

My split personality, sorry.

>> 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.

Ok

>> Source/WebCore/dom/BeforeUnloadEvent.h:32
>> +class BeforeUnloadEvent : public Event {
> 
> Should mark this class FINAL.

Ok.

[Chris Dumez]

Created attachment 211058 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 211058 [details]
Patch

Clearing flags on attachment: 211058

Committed r155367: <http://trac.webkit.org/changeset/155367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.