Fire visibilitychange event on unload (behind the flag)

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}

[kinuko, 2016-03-09 15:07:11]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 15:07:22]

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

[commit-bot: I haz the power, 2016-03-09 15:15:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-09 15:15:14]

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

[kinuko, 2016-03-10 09:05:18]

Description was changed from

==========
visibilitychange@unload

BUG=
==========

to

==========
visibilitychange@unload

http://w3c.github.io/page-visibility/#reacting-to-visibility-changes

BUG=554834
==========

[kinuko, 2016-03-10 09:05:54]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 09:06:14]

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

[kinuko, 2016-03-10 09:35:24]

Description was changed from

==========
visibilitychange@unload

http://w3c.github.io/page-visibility/#reacting-to-visibility-changes

BUG=554834
==========

to

==========
visibilitychange@unload

http://w3c.github.io/page-visibility/#reacting-to-visibility-changes

BUG=554834
TEST=to be added
==========

[commit-bot: I haz the power, 2016-03-10 10:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 10:15:21]

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

[kinuko, 2016-03-10 13:56:23]

Description was changed from

==========
visibilitychange@unload

http://w3c.github.io/page-visibility/#reacting-to-visibility-changes

BUG=554834
TEST=to be added
==========

to

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

[kinuko, 2016-03-10 13:56:23]

kinuko@chromium.org changed reviewers:
+ dcheng@chromium.org

[kinuko, 2016-03-10 13:59:25]

Patchset #3 (id:40001) has been deleted

[kinuko, 2016-03-10 13:59:43]

Patchset #2 (id:20001) has been deleted

[kinuko, 2016-03-10 14:00:17]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 14:00:23]

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

[kinuko, 2016-03-10 14:03:05]

Hi dcheng@- this could be probably the minimum change we could start with, could
you take a look if this looks reasonable?  (I'll add more tests and send out
intent to implement once it starts to look good to you)

[kinuko, 2016-03-10 14:06:38]

Patchset #3 (id:80001) has been deleted

[kinuko, 2016-03-10 14:06:46]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 14:06:57]

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

[commit-bot: I haz the power, 2016-03-10 15:49:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 15:49:41]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-03-10 22:46:33]

https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1373: return
PageVisibilityStateHidden;
I think we should update pageDismissalEventBeingDispatched(): otherwise
document.open() in visibilitychange can change m_loadEventProgress back to
LoadEventNotRun.

(Actually the spec says we should completely ignore document.open during unload
but I haven't had time to fix that yet: see https://crbug.com/583586)

[kinuko, 2016-03-11 04:11:52]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 04:12:15]

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

[kinuko, 2016-03-11 04:13:05]

PTAL

https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1778753003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1373: return
PageVisibilityStateHidden;
On 2016/03/10 22:46:33, dcheng wrote:
> I think we should update pageDismissalEventBeingDispatched(): otherwise
> document.open() in visibilitychange can change m_loadEventProgress back to
> LoadEventNotRun.
> 
> (Actually the spec says we should completely ignore document.open during
unload
> but I haven't had time to fix that yet: see https://crbug.com/583586)

Oh yeah we should.  Done.

[dcheng, 2016-03-11 04:20:38]

https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:867: // This array must be
consistent with PageDismissalType enum defined in
Do we need to preserve the relative order? This is currently logged in UMA. That
being said, I think the UMA stat is not very useful at this point. So maybe we
should just remove the UMA recording for this?

[commit-bot: I haz the power, 2016-03-11 04:26:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 04:26:10]

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_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2016-03-11 05:12:50]

https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:867: // This array must be
consistent with PageDismissalType enum defined in
On 2016/03/11 04:20:38, dcheng wrote:
> Do we need to preserve the relative order? This is currently logged in UMA.
That
> being said, I think the UMA stat is not very useful at this point. So maybe we
> should just remove the UMA recording for this?

Hmm, true. I removed the UMA part & changed these arrays into switch/case.  How
does it look to you now?

Also-- assuming that this patch's not looking terrible I'm going to add a few
more tests & start preparing intent process!

[dcheng, 2016-03-11 05:38:50]

On 2016/03/11 at 05:12:50, kinuko wrote:
>
https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/ChromeClientImpl.cpp:867: // This array must be
consistent with PageDismissalType enum defined in
> On 2016/03/11 04:20:38, dcheng wrote:
> > Do we need to preserve the relative order? This is currently logged in UMA.
That
> > being said, I think the UMA stat is not very useful at this point. So maybe
we
> > should just remove the UMA recording for this?
> 
> Hmm, true. I removed the UMA part & changed these arrays into switch/case. 
How does it look to you now?
> 
> Also-- assuming that this patch's not looking terrible I'm going to add a few
more tests & start preparing intent process!

Yep, looks reasonable to me.

I was wondering something actually: I know in the last discussion, we mentioned
restricting things that could run a nested message loop in this event. But
things like removing a plugin element from the DOM can trigger a nested message
loop due to a sync IPC: did you have any thoughts on how to handle that
situation?

[kinuko, 2016-03-11 06:07:54]

On 2016/03/11 05:38:50, dcheng wrote:
> On 2016/03/11 at 05:12:50, kinuko wrote:
> >
>
https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1778753003/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/web/ChromeClientImpl.cpp:867: // This array must
be
> consistent with PageDismissalType enum defined in
> > On 2016/03/11 04:20:38, dcheng wrote:
> > > Do we need to preserve the relative order? This is currently logged in
UMA.
> That
> > > being said, I think the UMA stat is not very useful at this point. So
maybe
> we
> > > should just remove the UMA recording for this?
> > 
> > Hmm, true. I removed the UMA part & changed these arrays into switch/case. 
> How does it look to you now?
> > 
> > Also-- assuming that this patch's not looking terrible I'm going to add a
few
> more tests & start preparing intent process!
> 
> Yep, looks reasonable to me.
> 
> I was wondering something actually: I know in the last discussion, we
mentioned
> restricting things that could run a nested message loop in this event. But
> things like removing a plugin element from the DOM can trigger a nested
message
> loop due to a sync IPC: did you have any thoughts on how to handle that
> situation?

Yeah we need think about that, and honestly I haven't given a deeper thought on
this yet =(  Ojan was saying that architecture team is planning to add more
restrictions once this one's in so I'm hoping we could continue discussion /
adding restrictions while landing this.

[kinuko, 2016-03-11 09:01:59]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 09:02:17]

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

[kinuko, 2016-03-11 09:02:21]

Description was changed from

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

to

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

[kinuko, 2016-03-11 09:02:50]

Added one more test and fixed builds etc. PTAL

[dcheng, 2016-03-11 09:13:37]

LGTM.

I think we could slightly simplify the layout test code a little but I leave it
up to you.

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html
(right):

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:12:
function startTest() {
window.addEventListener('load', function () {
  /* body of startTest */
});

Then we won't need to count docsLoaded (and iframe onload won't have to call
back up here)

Oh… this shares the test files with page-visibility-iframe-delete-test.html, I
see.

Well, it might be nice cleanup, but I guess let's just do it in a followup CL
since it involves more changes to
fast/events/page-visibility-iframe-delete-test.html.

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:18:
frames.push(document.getElementById("frame1"));
window[0]

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:19:
frames.push(frame1.contentDocument.getElementById("subIframe1"));
window[0][0]

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:20:
frames.push(frame1.contentDocument.getElementById("subIframe2"));
window[0][1]

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:23:
frames[i].contentDocument.addEventListener(
Then you can just use frames[i].document

[commit-bot: I haz the power, 2016-03-11 10:02:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 10:02:42]

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

[kinuko, 2016-03-11 12:37:16]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 12:37:34]

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

[kinuko, 2016-03-11 12:55:13]

Thanks!

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html
(right):

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:12:
function startTest() {
On 2016/03/11 09:13:36, dcheng wrote:
> window.addEventListener('load', function () {
>   /* body of startTest */
> });
> 
> Then we won't need to count docsLoaded (and iframe onload won't have to call
> back up here)
> 
> Oh… this shares the test files with page-visibility-iframe-delete-test.html, I
> see.
> 
> Well, it might be nice cleanup, but I guess let's just do it in a followup CL
> since it involves more changes to
> fast/events/page-visibility-iframe-delete-test.html.

Yup, I figured out that this wouldn't be the simplest code to do this. I'll make
a follow-up CL

https://codereview.chromium.org/1778753003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/page-visibility-iframe-unload.html:23:
frames[i].contentDocument.addEventListener(
On 2016/03/11 09:13:36, dcheng wrote:
> Then you can just use frames[i].document

cool, done.

[kinuko, 2016-03-11 12:55:36]

kinuko@chromium.org changed reviewers:
+ mkwst@chromium.org

[kinuko, 2016-03-11 12:56:03]

Mike: could you stamp for RuntimeEnabledFeatures change?

[commit-bot: I haz the power, 2016-03-11 13:34:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 13:34:57]

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

[Mike West, 2016-03-11 15:46:00]

RS LGTM. :)

[kinuko, 2016-03-12 01:34:44]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-03-12 01:34:45]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1778753003/#ps200001
(title: "fix tests")

[commit-bot: I haz the power, 2016-03-12 01:35:27]

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

[commit-bot: I haz the power, 2016-03-12 04:46:04]

Description was changed from

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

to

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

[commit-bot: I haz the power, 2016-03-12 04:46:05]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-03-12 04:47:58]

Description was changed from

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

to

==========
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}
==========

[commit-bot: I haz the power, 2016-03-12 04:47:59]

Patchset 8 (id:??) landed as
https://crrev.com/8909670a89cea02aca51324f6a1b0bd14bf0710a
Cr-Commit-Position: refs/heads/master@{#380848}