120849 – Add support for BeforeUnloadEvent interface

RESOLVED FIXED 120849

Add support for BeforeUnloadEvent interface

https://bugs.webkit.org/show_bug.cgi?id=120849

Summary Add support for BeforeUnloadEvent interface

Chris Dumez

Reported 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

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 attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2013-09-09 03:56:22 PDT

Created attachment 211027 [details] WIP Patch

Build Bot

Comment 2 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

Chris Dumez

Comment 3 2013-09-09 04:40:42 PDT

Created attachment 211029 [details] Attempt to fix mac build

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Chris Dumez

Comment 6 2013-09-09 05:42:20 PDT

Created attachment 211031 [details] Attempt to fix mac build

Build Bot

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

Chris Dumez

Comment 8 2013-09-09 06:16:13 PDT

Created attachment 211033 [details] Attempt to fix mac build

Build Bot

Comment 9 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

Chris Dumez

Comment 10 2013-09-09 07:02:54 PDT

Created attachment 211036 [details] Attempt to fix mac build

Build Bot

Comment 11 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

Chris Dumez

Comment 12 2013-09-09 07:58:19 PDT

Created attachment 211043 [details] Attempt to fix mac build

Build Bot

Comment 13 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

Build Bot

Comment 14 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

Chris Dumez

Comment 15 2013-09-09 08:51:14 PDT

Created attachment 211046 [details] Attempt to fix mac build

Chris Dumez

Comment 16 2013-09-09 10:17:26 PDT

Comment on attachment 211046 [details] Attempt to fix mac build Green bubbles, finally.

Darin Adler

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

Darin Adler

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

Chris Dumez

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

Chris Dumez

Comment 20 2013-09-09 11:03:20 PDT

Created attachment 211058 [details] Patch

WebKit Commit Bot

Comment 21 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>

WebKit Commit Bot

Comment 22 2013-09-09 11:39:10 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Chris Dumez

Reported

2013-09-06 05:32 PDT

Modified

2013-09-09 11:39 PDT History

CC List

11 users Show

URL

Keywords BlinkMergeCandidate, WebExposed

Depends on

Blocks