Bug 120849 - Add support for BeforeUnloadEvent interface
Summary: Add support for BeforeUnloadEvent interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: BlinkMergeCandidate, WebExposed
Depends on:
Blocks:
 
Reported: 2013-09-06 05:32 PDT by Chris Dumez
Modified: 2013-09-09 11:39 PDT (History)
11 users (show)

See Also:

Attachments
WIP Patch (25.67 KB, patch)
2013-09-09 03:56 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Attempt to fix mac build (30.17 KB, patch)
2013-09-09 04:40 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Attempt to fix mac build (32.46 KB, patch)
2013-09-09 05:42 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Attempt to fix mac build (32.46 KB, patch)
2013-09-09 06:16 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Attempt to fix mac build (33.22 KB, patch)
2013-09-09 07:02 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Attempt to fix mac build (34.63 KB, patch)
2013-09-09 07:58 PDT, Chris Dumez 		buildbot: commit-queue-
Details | Formatted Diff | Diff
Attempt to fix mac build (34.63 KB, patch)
2013-09-09 08:51 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (36.12 KB, patch)
2013-09-09 11:03 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (7) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-09-06 05:32:59 PDT 
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
Comment 1 Chris Dumez 2013-09-09 03:56:22 PDT 
Created attachment 211027 [details]
WIP Patch
Comment 2 Build Bot 2013-09-09 04:36:15 PDT 
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
Comment 3 Chris Dumez 2013-09-09 04:40:42 PDT 
Created attachment 211029 [details]
Attempt to fix mac build
Comment 4 Build Bot 2013-09-09 05:17:41 PDT 
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
Comment 5 Build Bot 2013-09-09 05:40:22 PDT 
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
Comment 6 Chris Dumez 2013-09-09 05:42:20 PDT 
Created attachment 211031 [details]
Attempt to fix mac build
Comment 7 Build Bot 2013-09-09 06:10:10 PDT 
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
Comment 8 Chris Dumez 2013-09-09 06:16:13 PDT 
Created attachment 211033 [details]
Attempt to fix mac build
Comment 9 Build Bot 2013-09-09 06:57:47 PDT 
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
Comment 10 Chris Dumez 2013-09-09 07:02:54 PDT 
Created attachment 211036 [details]
Attempt to fix mac build
Comment 11 Build Bot 2013-09-09 07:47:21 PDT 
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
Comment 12 Chris Dumez 2013-09-09 07:58:19 PDT 
Created attachment 211043 [details]
Attempt to fix mac build
Comment 13 Build Bot 2013-09-09 08:22:46 PDT 
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
Comment 14 Build Bot 2013-09-09 08:27:31 PDT 
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
Comment 15 Chris Dumez 2013-09-09 08:51:14 PDT 
Created attachment 211046 [details]
Attempt to fix mac build
Comment 16 Chris Dumez 2013-09-09 10:17:26 PDT 
Comment on attachment 211046 [details]
Attempt to fix mac build

Green bubbles, finally.
Comment 17 Darin Adler 2013-09-09 10:29:26 PDT 
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.
Comment 18 Darin Adler 2013-09-09 10:30:07 PDT 
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.
Comment 19 Chris Dumez 2013-09-09 10:58:31 PDT 
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.
Comment 20 Chris Dumez 2013-09-09 11:03:20 PDT 
Created attachment 211058 [details]
Patch
Comment 21 WebKit Commit Bot 2013-09-09 11:39:06 PDT 
Comment on attachment 211058 [details]
Patch

Clearing flags on attachment: 211058

Committed r155367: <http://trac.webkit.org/changeset/155367>
Comment 22 WebKit Commit Bot 2013-09-09 11:39:10 PDT 
All reviewed patches have been landed.  Closing bug.