Chromium Code Reviews
Help | Chromium Project | Sign in
(18)

Issue 2801813005: Only show a beforeunload dialog if a frame has had a user gesture since its load.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 4 days ago by Avi (concussion NO REVIEWS)
Modified:
2 weeks ago
CC:
chromium-reviews, caseq+blink_chromium.org, sof, creis+watch_chromium.org, blink-reviews-dom_chromium.org, eae+blinkwatch, nasko+codewatch_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, darin-cc_chromium.org, devtools-reviews_chromium.org, blink-reviews, apavlov+blink_chromium.org, rwlbuis, android-webview-reviews_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Only show a beforeunload dialog if a frame has had a user gesture since its load. https://www.chromestatus.com/feature/5082396709879808 BUG=707007 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : cleanup #

Total comments: 2

Patch Set 4 : aw #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -90 lines) Patch
M android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/ModalDialogTest.java View 5 chunks +40 lines, -1 line 0 comments Download
M chrome/browser/devtools/devtools_sanity_browsertest.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/lifetime/browser_close_manager_browsertest.cc View 23 chunks +40 lines, -35 lines 1 comment Download
M chrome/browser/ui/browser_browsertest.cc View 1 6 chunks +36 lines, -0 lines 2 comments Download
M chrome/browser/unload_browsertest.cc View 1 2 11 chunks +28 lines, -50 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl_browsertest.cc View 2 chunks +38 lines, -1 line 1 comment Download
M content/browser/web_contents/web_contents_impl.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
A + content/test/data/render_frame_host/beforeunload.html View 1 chunk +5 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/before-unload-reloads.html View 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/before-unload-returnValue.html View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/loader/form-submission-after-beforeunload-cancel.html View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/loader/resources/iframe-with-beforeunload.html View 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/loader/show-only-one-beforeunload-dialog.html View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/misc/resources/reentrant-beforeunload-helper.html View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/page/javascriptDialogEvents.html View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 1 chunk +9 lines, -0 lines 1 comment Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 30 (22 generated)
Avi (concussion NO REVIEWS)
Ted for chrome/android Bo Liu for aw/ Nico for chrome/* Charlie for content/ Rick for ...
2 weeks, 4 days ago (2017-04-06 21:20:51 UTC) #11
boliu
https://codereview.chromium.org/2801813005/diff/40001/android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java File android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java (right): https://codereview.chromium.org/2801813005/diff/40001/android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java#newcode180 android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java:180: AwTestTouchUtils.simulateTouchCenterOfView(view); should write a comment explaining this?
2 weeks, 4 days ago (2017-04-06 21:30:13 UTC) #12
Avi (concussion NO REVIEWS)
https://codereview.chromium.org/2801813005/diff/40001/android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java File android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java (right): https://codereview.chromium.org/2801813005/diff/40001/android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java#newcode180 android_webview/javatests/src/org/chromium/android_webview/test/WebViewModalDialogOverrideTest.java:180: AwTestTouchUtils.simulateTouchCenterOfView(view); On 2017/04/06 21:30:13, boliu wrote: > should write ...
2 weeks, 4 days ago (2017-04-06 22:29:19 UTC) #15
boliu
lgtm
2 weeks, 4 days ago (2017-04-06 22:30:18 UTC) #16
Ted C
On 2017/04/06 22:30:18, boliu wrote: > lgtm chrome/android - lgtm
2 weeks, 4 days ago (2017-04-06 23:10:24 UTC) #17
Rick Byers
WebKit LGTM
2 weeks, 3 days ago (2017-04-07 18:09:03 UTC) #28
Nico
chrome lgtm, but looks pretty copy-pasta-y https://codereview.chromium.org/2801813005/diff/60001/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/2801813005/diff/60001/chrome/browser/ui/browser_browsertest.cc#newcode627 chrome/browser/ui/browser_browsertest.cc:627: // JavaScript onbeforeunload ...
2 weeks, 3 days ago (2017-04-07 20:05:28 UTC) #29
Charlie Reis
2 weeks ago (2017-04-10 06:20:10 UTC) #30
Thanks for doing this!  It seems like a good change and it's nice that there's
precedent for it.

Generally looks good, but one question to make sure we're tracking this per
document rather than per frame.

https://codereview.chromium.org/2801813005/diff/60001/chrome/browser/lifetime...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/2801813005/diff/60001/chrome/browser/lifetime...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:300: void
PrepareForDialog(content::WebContents* web_contents) {
This might be a good candidate for a utility in content/public/test, so that
other tests can use it?

https://codereview.chromium.org/2801813005/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2801813005/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:627: // JavaScript onbeforeunload
dialogs require a user gesture.
On 2017/04/07 20:05:28, Nico wrote:
> You're adding this exactly in the tests where you added
> DisableBeforeUnloadHangMonitorForTesting(), right? Maybe there can at least be
a
> helper that does DisableBeforeUnloadHangMonitorForTesting() and this loop?
Does
> it ever make sense to have only one of
> DisableBeforeUnloadHangMonitorForTesting() and loop?

It does seem like the majority of cases would benefit from a helper that does
both.

https://codereview.chromium.org/2801813005/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2801813005/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:3074: if
(!frame()->hasReceivedUserGesture()) {
Sanity check: Does this mean the frame has received a user gesture, or the
document?  It's confusing that it's on frame, but the documentation on
WebLocalFrame kind of implies it's about the document.

Can you add a test that beforeunload doesn't fire if there was a user gesture
for a previous document in the same frame?  (Ideally this would be a
content_browsertest that checked both same-process and cross-process, in case
there's some kind of replication that confuses us.)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46