Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(439)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 22893051: Add support for BeforeUnloadEvent (Closed)

Created:
7 years, 4 months ago by do-not-use
Modified:
7 years, 3 months ago
CC:
blink-reviews, Nils Barth (inactive), kojih, jsbell+bindings_chromium.org, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, haraken, Nate Chapin
Visibility:
Public.

Description

Add support for BeforeUnloadEvent 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). BUG=277851 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156916

Patch Set 1 #

Total comments: 9

Patch Set 2 : Fix issues #

Total comments: 1

Patch Set 3 : Rebase on master #

Patch Set 4 : Add back Event.returnValue #

Patch Set 5 : Rebaseline test #

Patch Set 6 : Fix test failures #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -45 lines) Patch
A LayoutTests/fast/events/before-unload-returnValue.html View 1 1 chunk +32 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/before-unload-returnValue-expected.txt View 1 chunk +13 lines, -0 lines 0 comments Download
M LayoutTests/fast/xmlhttprequest/xmlhttprequest-get-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/jquery/event-expected.txt View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/transitions/transition-end-event-prefixed-01.html View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/transitions/transition-end-event-prefixed-01-expected.txt View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/transitions/transition-end-event-prefixed-03.html View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/transitions/transition-end-event-prefixed-03-expected.txt View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/virtual/stable/webexposed/global-constructors-listing-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/webexposed/global-constructors-listing-expected.txt View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M Source/bindings/v8/V8AbstractEventListener.cpp View 1 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/BeforeUnloadEvent.h View 1 2 chunks +24 lines, -14 lines 0 comments Download
M Source/core/dom/BeforeUnloadEvent.cpp View 1 2 chunks +4 lines, -7 lines 0 comments Download
A + Source/core/dom/BeforeUnloadEvent.idl View 1 chunk +2 lines, -4 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/dom/Event.h View 1 2 3 3 chunks +4 lines, -5 lines 0 comments Download
M Source/core/dom/Event.cpp View 1 1 chunk +2 lines, -6 lines 0 comments Download
M Source/core/dom/Event.idl View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/UseCounter.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/page/UseCounter.cpp View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
do-not-use
7 years, 4 months ago (2013-08-23 11:52:17 UTC) #1
haraken
LGTM. Needs an approval from an API owner. https://codereview.chromium.org/22893051/diff/1/LayoutTests/fast/events/before-unload-returnValue.html File LayoutTests/fast/events/before-unload-returnValue.html (right): https://codereview.chromium.org/22893051/diff/1/LayoutTests/fast/events/before-unload-returnValue.html#newcode23 LayoutTests/fast/events/before-unload-returnValue.html:23: <body> ...
7 years, 4 months ago (2013-08-23 12:27:00 UTC) #2
arv (Not doing code reviews)
https://codereview.chromium.org/22893051/diff/1/Source/bindings/v8/V8AbstractEventListener.cpp File Source/bindings/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/22893051/diff/1/Source/bindings/v8/V8AbstractEventListener.cpp#newcode161 Source/bindings/v8/V8AbstractEventListener.cpp:161: if (event->type() == eventNames().beforeunloadEvent && !returnValue->IsNull() && !returnValue->IsUndefined()) This ...
7 years, 4 months ago (2013-08-23 13:56:48 UTC) #3
do-not-use
https://codereview.chromium.org/22893051/diff/1/Source/bindings/v8/V8AbstractEventListener.cpp File Source/bindings/v8/V8AbstractEventListener.cpp (right): https://codereview.chromium.org/22893051/diff/1/Source/bindings/v8/V8AbstractEventListener.cpp#newcode161 Source/bindings/v8/V8AbstractEventListener.cpp:161: if (event->type() == eventNames().beforeunloadEvent && !returnValue->IsNull() && !returnValue->IsUndefined()) On ...
7 years, 4 months ago (2013-08-23 14:20:40 UTC) #4
do-not-use
https://codereview.chromium.org/22893051/diff/1/LayoutTests/fast/events/before-unload-returnValue.html File LayoutTests/fast/events/before-unload-returnValue.html (right): https://codereview.chromium.org/22893051/diff/1/LayoutTests/fast/events/before-unload-returnValue.html#newcode23 LayoutTests/fast/events/before-unload-returnValue.html:23: <body> On 2013/08/23 12:27:00, haraken wrote: > > Nit: ...
7 years, 4 months ago (2013-08-23 14:27:54 UTC) #5
arv (Not doing code reviews)
LGTM
7 years, 4 months ago (2013-08-23 14:47:29 UTC) #6
abarth-chromium
https://codereview.chromium.org/22893051/diff/11001/Source/core/dom/Event.idl File Source/core/dom/Event.idl (left): https://codereview.chromium.org/22893051/diff/11001/Source/core/dom/Event.idl#oldcode72 Source/core/dom/Event.idl:72: attribute boolean returnValue; Do other browsers not have this ...
7 years, 4 months ago (2013-08-23 19:32:33 UTC) #7
abarth-chromium
LGTM --- sorry I missed the part about returnValue in the description.
7 years, 4 months ago (2013-08-23 19:33:10 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/22893051/11001
7 years, 4 months ago (2013-08-23 19:46:29 UTC) #9
commit-bot: I haz the power
Change committed as 156654
7 years, 4 months ago (2013-08-23 21:05:43 UTC) #10
do-not-use
Relanding without change as Erik did the necessary clean up on Chrome side after the ...
7 years, 3 months ago (2013-08-27 06:55:44 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/22893051/22001
7 years, 3 months ago (2013-08-27 06:56:16 UTC) #12
commit-bot: I haz the power
Change committed as 156774
7 years, 3 months ago (2013-08-27 08:38:25 UTC) #13
do-not-use
Reuploading the patch. This time, I deprecated Event.returnValue instead of removing it as it seems ...
7 years, 3 months ago (2013-08-28 18:59:32 UTC) #14
arv (Not doing code reviews)
LGTM
7 years, 3 months ago (2013-08-28 19:03:03 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/22893051/39001
7 years, 3 months ago (2013-08-28 19:26:01 UTC) #16
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=3432
7 years, 3 months ago (2013-08-28 22:17:54 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/22893051/55001
7 years, 3 months ago (2013-08-29 07:04:35 UTC) #18
commit-bot: I haz the power
7 years, 3 months ago (2013-08-29 10:58:35 UTC) #19
Message was sent while issue was closed. 
Change committed as 156916

Powered by Google App Engine
This is Rietveld 408576698