Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

Issue 1778753003: Fire visibilitychange event on unload (behind the flag) (Closed)

Created:
3 years ago by kinuko
Modified:
3 years ago
Reviewers:
Mike West, dcheng
CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, kinuko+watch, rwlbuis, sof
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fire visibilitychange event on unload (behind the flag) Per recent spec change: http://w3c.github.io/page-visibility/#reacting-to-visibility-changes BUG=554834 TEST=fast/events/page-visibility-iframe-unload.html fast/events/page-visibility-unload.html Committed: https://crrev.com/8909670a89cea02aca51324f6a1b0bd14bf0710a Cr-Commit-Position: refs/heads/master@{#380848}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 2

Patch Set 4 : add dismissal type #

Total comments: 2

Patch Set 5 : switch/case #

Patch Set 6 : fix + one more test #

Total comments: 7

Patch Set 7 : fix tests #

Patch Set 8 : fix tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -27 lines) Patch
M third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-delete-test.html View 1 1 chunk +1 line, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html View 1 2 3 4 5 6 7 1 chunk +40 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload-expected.txt View 1 2 3 4 5 6 7 1 chunk +13 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/events/page-visibility-unload.html View 1 2 3 4 5 1 chunk +22 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/events/page-visibility-unload-expected.txt View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/events/resources/page-visibility-iframe-delete-test-frame.html View 1 1 chunk +0 lines, -6 lines 0 comments Download
A + third_party/WebKit/LayoutTests/fast/events/resources/page-visibility-iframe-with-subframes.html View 1 0 chunks +-1 lines, --1 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 3 chunks +31 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/ChromeClientImpl.cpp View 1 2 3 4 5 6 7 2 chunks +39 lines, -14 lines 0 comments Download

Messages

Total messages: 54 (26 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/1
3 years ago (2016-03-09 15:07:22 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/154841)
3 years ago (2016-03-09 15:15:14 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/20001
3 years ago (2016-03-10 09:06:14 UTC) #7
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/194504)
3 years ago (2016-03-10 10:15:21 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/60001
3 years ago (2016-03-10 14:00:23 UTC) #16
kinuko
Hi dcheng@- this could be probably the minimum change we could start with, could you ...
3 years ago (2016-03-10 14:03:05 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/100001
3 years ago (2016-03-10 14:06:57 UTC) #20
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years ago (2016-03-10 15:49:41 UTC) #22
dcheng
https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp#newcode1373 third_party/WebKit/Source/core/dom/Document.cpp:1373: return PageVisibilityStateHidden; I think we should update pageDismissalEventBeingDispatched(): otherwise ...
3 years ago (2016-03-10 22:46:33 UTC) #23
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/120001
3 years ago (2016-03-11 04:12:15 UTC) #25
kinuko
PTAL https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Source/core/dom/Document.cpp#newcode1373 third_party/WebKit/Source/core/dom/Document.cpp:1373: return PageVisibilityStateHidden; On 2016/03/10 22:46:33, dcheng wrote: > ...
3 years ago (2016-03-11 04:13:05 UTC) #26
dcheng
https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode867 third_party/WebKit/Source/web/ChromeClientImpl.cpp:867: // This array must be consistent with PageDismissalType enum ...
3 years ago (2016-03-11 04:20:38 UTC) #27
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/128818) chromeos_amd64-generic_chromium_compile_only_ng on ...
3 years ago (2016-03-11 04:26:10 UTC) #29
kinuko
https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Source/web/ChromeClientImpl.cpp File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode867 third_party/WebKit/Source/web/ChromeClientImpl.cpp:867: // This array must be consistent with PageDismissalType enum ...
3 years ago (2016-03-11 05:12:50 UTC) #30
dcheng
On 2016/03/11 at 05:12:50, kinuko wrote: > https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Source/web/ChromeClientImpl.cpp > File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right): > > https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Source/web/ChromeClientImpl.cpp#newcode867 ...
3 years ago (2016-03-11 05:38:50 UTC) #31
kinuko
On 2016/03/11 05:38:50, dcheng wrote: > On 2016/03/11 at 05:12:50, kinuko wrote: > > > ...
3 years ago (2016-03-11 06:07:54 UTC) #32
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/160001
3 years ago (2016-03-11 09:02:17 UTC) #34
kinuko
Added one more test and fixed builds etc. PTAL
3 years ago (2016-03-11 09:02:50 UTC) #36
dcheng
LGTM. I think we could slightly simplify the layout test code a little but I ...
3 years ago (2016-03-11 09:13:37 UTC) #37
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/194039)
3 years ago (2016-03-11 10:02:42 UTC) #39
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/180001
3 years ago (2016-03-11 12:37:34 UTC) #41
kinuko
Thanks! https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html File third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html (right): https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html#newcode12 third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:12: function startTest() { On 2016/03/11 09:13:36, dcheng wrote: ...
3 years ago (2016-03-11 12:55:13 UTC) #42
kinuko
Mike: could you stamp for RuntimeEnabledFeatures change?
3 years ago (2016-03-11 12:56:03 UTC) #44
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/129654)
3 years ago (2016-03-11 13:34:57 UTC) #46
Mike West
RS LGTM. :)
3 years ago (2016-03-11 15:46:00 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778753003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778753003/200001
3 years ago (2016-03-12 01:35:27 UTC) #50
commit-bot: I haz the power
Committed patchset #8 (id:200001)
3 years ago (2016-03-12 04:46:05 UTC) #52
commit-bot: I haz the power
3 years ago (2016-03-12 04:47:59 UTC) #54
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/8909670a89cea02aca51324f6a1b0bd14bf0710a
Cr-Commit-Position: refs/heads/master@{#380848}

Powered by Google App Engine
This is Rietveld 408576698