Suppressing modal JavaScript/HTML dialogs (i.e., calls to alert(), confirm(), prompt() and showModalDialog()) during unload events (onbeforeunload() and onunload()) can reduce annoyance and help speed up the appearance of closing a tab. The onbeforeunload() event handler offers an opportunity for a page to request user confirmation before a page is unloaded (by returning a non-NULL value), so the other dialogs are not really needed. References: http://code.google.com/p/chromium/issues/detail?id=68780 https://bugzilla.mozilla.org/show_bug.cgi?id=391834
Created attachment 86035 [details] Patch
Do other browsers block these?
(In reply to comment #2) > Do other browsers block these? None that I know of. The patch is conservative; only blocks these dialogs in Chromium for now. Firefox seems to want to do it as well (https://bugzilla.mozilla.org/show_bug.cgi?id=391834).
ping?
Comment on attachment 86035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86035&action=review > LayoutTests/fast/loader/unload-alert.html:9 > + // Once dialogs are disabled on all platforms, change these to "FAIL" so that > + // if the dialogs appear, it's obvious that the test failed. > + alert("PASS"); > + confirm("PASS"); > + prompt("PASS", "PASS"); This test doesn't make sense on chromium, does it? If any of PASS appear on chromium platform, it actually failed, right? I don't think it's clear at all from the test output or from the comment here. > Source/WebCore/page/Chrome.cpp:62 > +static inline bool canRunModalDuringUnload() > +{ > +#if PLATFORM(CHROMIUM) > + return false; > +#else > + return true; > +#endif > +} I don't think this is the right way to do it if we want to make decisions per port. We should add a new callback to WebView or some other class instead.
Comment on attachment 86035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86035&action=review > Source/WebCore/page/Chrome.cpp:66 > + return page->mainFrame()->loader()->pageDismissalEventBeingDispatched(); Does this mean that we will disable modal dialogs only when the main frame is unloading? Why do we allow modal dialogs during unload event in subframes? I'd like to see some explanation as to why such a decision was made in the change log entry.
Comment on attachment 86035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86035&action=review >> LayoutTests/fast/loader/unload-alert.html:9 >> + prompt("PASS", "PASS"); > > This test doesn't make sense on chromium, does it? If any of PASS appear on chromium platform, it actually failed, right? I don't think it's clear at all from the test output or from the comment here. Right, it's misleading for chromium. What would be the preferred way of doing this? Should I: 1. Change the text to read something like "FAIL on chromium; PASS on other platforms"? 2. Somehow fork the test itself for chromium? (I only saw a way to specify a platform-specific expected.ext.) Other ways? >> Source/WebCore/page/Chrome.cpp:62 >> +} > > I don't think this is the right way to do it if we want to make decisions per port. We should add a new callback to WebView or some other class instead. Darin (Fisher) explicitly didn't want this (decision to suppress alerts) to be something platform-specific. Though the current patch only enables it for chromium, the goal is to convince other platforms, and make it the default behaviour eventually. >> Source/WebCore/page/Chrome.cpp:66 >> + return page->mainFrame()->loader()->pageDismissalEventBeingDispatched(); > > Does this mean that we will disable modal dialogs only when the main frame is unloading? Why do we allow modal dialogs during unload event in subframes? I'd like to see some explanation as to why such a decision was made in the change log entry. My mistake. After chancing upon "allowsBeforeUnloadListeners()" in page/DOMWindow.cpp, I assumed that unload/beforeunload listeners were not allowed for subframes. I created some test pages just now, and I see that in fact, that's not the case; subframes can register listeners for those events. I'll change the patch to disallow alerts in these events for all frames. Meanwhile, before I create that patch, could we get agreement on the approach (i.e., global or platform-specific implementation)?
Comment on attachment 86035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86035&action=review >>> LayoutTests/fast/loader/unload-alert.html:9 >>> + prompt("PASS", "PASS"); >> >> This test doesn't make sense on chromium, does it? If any of PASS appear on chromium platform, it actually failed, right? I don't think it's clear at all from the test output or from the comment here. > > Right, it's misleading for chromium. What would be the preferred way of doing this? Should I: > 1. Change the text to read something like "FAIL on chromium; PASS on other platforms"? > 2. Somehow fork the test itself for chromium? (I only saw a way to specify a platform-specific expected.ext.) > Other ways? Looking closer, it appears that LayoutTests/platform can also contain .html files, and not just expected.txt files. I'll use that to fork the test for chromium (and add the overridden one to Skipped, if needed).
(In reply to comment #7) > Right, it's misleading for chromium. What would be the preferred way of doing this? Should I: > 1. Change the text to read something like "FAIL on chromium; PASS on other platforms"? > 2. Somehow fork the test itself for chromium? (I only saw a way to specify a platform-specific expected.ext.) > Other ways? Put a test in LayoutTests/platform/chromium/. IMO, there's no need to add a test for other ports. > >> Source/WebCore/page/Chrome.cpp:62 > >> +} > > > > I don't think this is the right way to do it if we want to make decisions per port. We should add a new callback to WebView or some other class instead. > > Darin (Fisher) explicitly didn't want this (decision to suppress alerts) to be something platform-specific. Though the current patch only enables it for chromium, the goal is to convince other platforms, and make it the default behaviour eventually. If we're trying to convince other platforms to do the same on other ports, then the right approach should be to convince others first, then implement this patch. It's generally a bad idea to introduce a behavioral difference between ports that is visible to JavaScript.
Created attachment 88926 [details] Patch
Comment on attachment 88926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88926&action=review Please separate changes to the existing tests because there's no reason they should be in the same patch. > Source/WebCore/page/Chrome.cpp:209 > +static inline bool canRunModalDuringPageDismissal() > +{ > +#if PLATFORM(CHROMIUM) > + return false; > +#else > + return true; > +#endif > +} I don't think this is the right way to do it. Since we already have willRunModalDialogDuringPageDismissal on chrome client, we should rename it to shouldRunModalDialogDuringPageDismissal and have it return a boolean value. I know that we want all ports to eventually disallow modal dialogs but that's not the case at least for now so it seems more natural to have chrome client decide it.
One case where this change would be a problem is for people trying to debug their JavaScript code with alerts.
(In reply to comment #12) > One case where this change would be a problem is for people trying to debug their JavaScript code with alerts. That's a fair point. I think most cases where you'd want this could be addressed by adding a "debugger;" line and breaking in the debugger.
(In reply to comment #13) > (In reply to comment #12) > > One case where this change would be a problem is for people trying to debug their JavaScript code with alerts. > > That's a fair point. I think most cases where you'd want this could be addressed by adding a "debugger;" line and breaking in the debugger. Agreed. Easier than a debugger is console.log() which is as easy as alert(), especially if we can persist the console output across navigations. It's not as portable as alert(), however it does have an advantage over alert(): It doesn't steal the focus, which is very handy if you are trying to debug something in onblur(), for example.
Please don't forget to report your findings to the WHATWG mailing list when you've done this, so I can update the spec to make this conforming behaviour (assuming it isn't found to be ill-advised). This seems like a great idea.
FWIW, I was just debugging something in onload, and had a thought that it would have been a problem for me if we were suppressing these alerts.
(In reply to comment #16) > FWIW, I was just debugging something in onload, and had a thought that it would have been a problem for me if we were suppressing these alerts. Did you mean unload? There's no plan to suppress alerts during onload.
Yes, sorry for the typo.
(In reply to comment #18) > Yes, sorry for the typo. How about if we add a console message whenever an alert in unload is blocked? Since console messages can be made persistent across page loads, would that be an acceptable compromise?
You might want to check the behavior in Firefox 4. Because their alerts are no longer modal, they might well be effectively suppressed in unload also.
Created attachment 97187 [details] Adds a console message for blocked alerts. Adds Chromium-specific test expectations.
Comment on attachment 97187 [details] Adds a console message for blocked alerts. Adds Chromium-specific test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=97187&action=review > Source/WebCore/page/Chrome.cpp:210 > +static inline bool isDuringPageDismissal(const Frame* frame) > +{ > + return frame->loader()->pageDismissalEventBeingDispatched(); > +} I don't see any point in adding this inline function. It needlessly adds new indirection. We can just call frame->loader()->pageDismissalEventBeingDispatched() directly. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:959 > + logMessage += " during page dismissal (beforeunload, pagehide or unload)."; Can't we identify which one it is?
Comment on attachment 97187 [details] Adds a console message for blocked alerts. Adds Chromium-specific test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=97187&action=review > Source/WebKit/chromium/src/ChromeClientImpl.cpp:942 > + String logMessage = "Blocked "; StringBuilder plz.
Comment on attachment 97187 [details] Adds a console message for blocked alerts. Adds Chromium-specific test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=97187&action=review >> Source/WebCore/page/Chrome.cpp:210 >> +} > > I don't see any point in adding this inline function. It needlessly adds new indirection. We can just call frame->loader()->pageDismissalEventBeingDispatched() directly. Done. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:942 >> + String logMessage = "Blocked "; > > StringBuilder plz. Done. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:959 >> + logMessage += " during page dismissal (beforeunload, pagehide or unload)."; > > Can't we identify which one it is? Sure, if you are okay with instrumenting stuff in WebCore/loader/FrameLoader.cpp. Instead of that, it might be better (but more hacky) to pass along the JS source file and line number here.
Comment on attachment 97187 [details] Adds a console message for blocked alerts. Adds Chromium-specific test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=97187&action=review >>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:959 >>> + logMessage += " during page dismissal (beforeunload, pagehide or unload)."; >> >> Can't we identify which one it is? > > Sure, if you are okay with instrumenting stuff in WebCore/loader/FrameLoader.cpp. Instead of that, it might be better (but more hacky) to pass along the JS source file and line number here. Oh, that's not what I meant. Can we say whether it was beforeunload, pagehide, or unload?
Comment on attachment 97187 [details] Adds a console message for blocked alerts. Adds Chromium-specific test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=97187&action=review >>>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:959 >>>> + logMessage += " during page dismissal (beforeunload, pagehide or unload)."; >>> >>> Can't we identify which one it is? >> >> Sure, if you are okay with instrumenting stuff in WebCore/loader/FrameLoader.cpp. Instead of that, it might be better (but more hacky) to pass along the JS source file and line number here. > > Oh, that's not what I meant. Can we say whether it was beforeunload, pagehide, or unload? Yeah, I understood you. To differentiate between those, we need to add some hooks in FrameLoader.cpp. Say by changing m_pageDismissalEventBeingDispatched from a bool to an enum or such. As an alternative, instead of saying which type of unload handler it was, we might as well just emit the JS source details (file and line number), though I think passing along that information to this spot will be much hackier.
(In reply to comment #20) > You might want to check the behavior in Firefox 4. Because their alerts are no longer modal, they might well be effectively suppressed in unload also. I tested Firefox 4.0.1 on Mac. Alerts showed up in all unload handlers (beforeunload, pagehide and unload). Moreover, they were app-modal. I couldn't switch to any other tab.
Created attachment 97200 [details] Removes indirection. Uses StringBuilder.
Comment on attachment 97200 [details] Removes indirection. Uses StringBuilder. Attachment 97200 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8836831
Created attachment 97206 [details] Adds #include for StringBuilder
Comment on attachment 97206 [details] Adds #include for StringBuilder View in context: https://bugs.webkit.org/attachment.cgi?id=97206&action=review > LayoutTests/ChangeLog:25 > + * platform/chromium/fast/dom/Geolocation/notimer-after-unload-expected.txt: Added. > + * platform/chromium/fast/events/onbeforeunload-focused-iframe-expected.txt: Added. > + * platform/chromium/fast/events/onunload-clears-onbeforeunload-expected.txt: Added. > + * platform/chromium/fast/events/onunload-expected.txt: Added. > + * platform/chromium/fast/events/onunload-not-on-body-expected.txt: Added. > + * platform/chromium/fast/events/onunload-window-property-expected.txt: Added. > + * platform/chromium/fast/events/pageshow-pagehide-on-back-uncached-expected.txt: Added. > + * platform/chromium/fast/history/timed-refresh-in-cached-frame-expected.txt: Added. > + * platform/chromium/fast/loader/frames-with-unload-handlers-in-page-cache-expected.txt: Added. > + * platform/chromium/fast/loader/page-dismissal-modal-dialogs-expected.txt: Added. > + * platform/chromium/fast/loader/recursive-before-unload-crash-expected.txt: Added. Please use svn mv first. It's hard to see what changes you're making to expected results because you're adding new files.
Oops, I meant to say "svn cp".
Created attachment 97368 [details] Diff of test expectations from current files to newly added chromium-specific files
Comment on attachment 97206 [details] Adds #include for StringBuilder View in context: https://bugs.webkit.org/attachment.cgi?id=97206&action=review > Source/WebKit/chromium/src/ChromeClientImpl.cpp:966 > + logMessage.append(" during page dismissal (beforeunload, pagehide or unload)."); As I commented earlier, we should identify the event from which the modal dialog was open (beforeunload, pagehide, or unload.
Comment on attachment 97206 [details] Adds #include for StringBuilder Removing r- per IRC discussion.
If I remember correctly, Firefox solves the perceived performance problem much easier. You could just hide the tab immediately, and re-display it if it shows an alert in onunload (which would be a very rare thing in practice, basically only for debugging). Or even if Firefox doesn't do that, we could.
> If I remember correctly, Firefox solves the perceived performance problem much easier. You could just hide the tab immediately, and re-display it if it shows an alert in onunload (which would be a very rare thing in practice, basically only for debugging). We considered that approach but it was rejected by the user experience folks as being an unpleasant user experience.
(In reply to comment #34) > (From update of attachment 97206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97206&action=review > > Source/WebKit/chromium/src/ChromeClientImpl.cpp:966 > > + logMessage.append(" during page dismissal (beforeunload, pagehide or unload)."); > > As I commented earlier, we should identify the event from which the modal dialog was open (beforeunload, pagehide, or unload. This doesn't seem worth complicating the WebKit code over. Web developers who encounter this can easily add the appropriate logging to figure this out.
> We considered that approach but it was rejected by the user experience folks as being an unpleasant user experience. Did they consider that there is no user experience to affect? Web pages aren't displaying alerts from unload, so regular users aren't going to see any difference either way. I'm not sure if we can have a meaningful discussion here if there is a third party that mandates such behavior for Chrome.
(In reply to comment #39) > > We considered that approach but it was rejected by the user experience folks as being an unpleasant user experience. > > Did they consider that there is no user experience to affect? Web pages aren't displaying alerts from unload, so regular users aren't going to see any difference either way. > > I'm not sure if we can have a meaningful discussion here if there is a third party that mandates such behavior for Chrome. There is no mandate. However, folks on the Chrome team have discussed this issue, and we all agreed that we didn't like hiding and re-showing a tab. Adam was summarizing that, and it is true that our "user experience folks" agree. Regardless, we can certainly talk about this topic here. My take: I have long argued that hiding a tab and showing it again would be a jarring experience. I also contend that if it is the last tab, then it will be really wonky to hide the entire window and then show it again. Keep in mind that it can take seconds before the unload handler runs as you may need to swap a child process back into memory. Meanwhile the user has gone to work on something else. It is not good UI to then pop back up the window they previously dismissed, possibly forcing them away from what they were doing. Moreover, because it is rare that unload handlers call alert, I believe that we would avoid a great deal of complexity (extra state) by NOT having to code support for it at all. The complexity of hiding and re-showing the tab or browser window would be avoided too. We already have a lot of complex code for allowing alerts to be generated as a child process is being torn down to be replaced by a different child process all housed within the same tab. I would love to delete that code!
Created attachment 98623 [details] Identifies page dismissal event more specifically
Created attachment 98624 [details] Updated test expectation
[Copy paste from https://lists.webkit.org/pipermail/webkit-dev/2011-June/017296.html] Briefly, from a histogram we added a while ago, I find that 2.3% of all users encounter at least one such modal dialog every week. This is much larger than I expected. I think it means that it's not just web developers who encounter them while debugging their pages. To me, this makes a very strong case for disallowing them. Here are some examples of sites that show an alert when their pages are unloaded: http://ddl.vidplayer.net/ http://www.buenosprofesionales.com/ http://www.watchsaleshop.com/ http://52-teurseu.blogspot.com/ http://aaz-armor.blogspot.com/ The last two sites above are just two examples out of hundreds of such blogs.
Created attachment 98925 [details] Diff of test expectations from current files to newly added chromium-specific files
Created attachment 98931 [details] Now logging page dismissal type explicitly in the histograms
Created attachment 98932 [details] Diff of test expectations from current files to newly added chromium-specific files
Comment on attachment 98931 [details] Now logging page dismissal type explicitly in the histograms View in context: https://bugs.webkit.org/attachment.cgi?id=98931&action=review > Source/WebCore/loader/FrameLoader.h:280 > + NumDismissalTypes = 4, Please remove this enum value. It's not necessary. > Source/WebCore/page/Chrome.cpp:209 > -bool Chrome::canRunModalNow() const > +bool Chrome::canRunModalNow(const Frame* frame) const We don't use "const Frame", just "Frame". Shouldn't this function be per-Page? If one of the frames is processing unload, that should probably suppress alerts in the whole Page. > Source/WebCore/page/Chrome.cpp:311 > - willRunModalDialog(frame, ChromeClient::ConfirmDialog, m_client); > + FrameLoader::PageDismissalType dismissal = frame->loader()->pageDismissalEventBeingDispatched(); > + if (dismissal != FrameLoader::NoDismissal && !m_client->shouldRunModalDialogDuringPageDismissal(ChromeClient::ConfirmDialog, message, dismissal)) > + return false; Can we factor this copy/paste code into a comment method? Also, these functions have the same Frame-or-Page issue. > Source/WebCore/page/ChromeClient.h:330 > - virtual void willRunModalDialogDuringPageDismissal(const DialogType&) const { } > + virtual bool shouldRunModalDialogDuringPageDismissal(const DialogType&, const String&, FrameLoader::PageDismissalType) const { return true; } What is this string argument? We probably need a parameter name to know. > Source/WebCore/page/DOMWindow.cpp:393 > - return page->chrome()->canRunModalNow(); > + return page->chrome()->canRunModalNow(frame); Notice that this caller has a nice page for you. :) > Source/WebKit/chromium/src/ChromeClientImpl.cpp:949 > + int numDismissalTypes = static_cast<int>(FrameLoader::NumDismissalTypes); > + int numDialogTypes = static_cast<int>(NumDialogTypes); > + int intDialog = static_cast<int>(dialogType); > + int intDismissal = static_cast<int>(dismissal); > + PlatformBridge::histogramEnumeration("Renderer.ModalDialogsDuringPageDismissal", intDialog * numDismissalTypes + intDismissal, numDismissalTypes * numDialogTypes); This is a giant hack. Can we please not have these sorts of hacks? > Source/WebKit/chromium/src/ChromeClientImpl.cpp:957 > + logMessage.append("Blocked alert('"); > + logMessage.append(message); > + logMessage.append("')"); > + break; This code is all super redundant. Consider using String::format to make this code prettier. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:972 > + default: > + logMessage.append("Blocked modal dialog"); Please remove these default cases and explicitly handle all the enum values.
Comment on attachment 98931 [details] Now logging page dismissal type explicitly in the histograms View in context: https://bugs.webkit.org/attachment.cgi?id=98931&action=review >> Source/WebCore/loader/FrameLoader.h:280 >> + NumDismissalTypes = 4, > > Please remove this enum value. It's not necessary. Done (I needed it for computing the largest value for the histogram, but I've hardcoded that number now). >> Source/WebCore/page/Chrome.cpp:209 >> +bool Chrome::canRunModalNow(const Frame* frame) const > > We don't use "const Frame", just "Frame". > > Shouldn't this function be per-Page? If one of the frames is processing unload, that should probably suppress alerts in the whole Page. If a frame is navigating somewhere, it doesn't necessarily mean that the page is, right? It could just be a subframe alone that's navigating, with no change to the top-level page URL. I don't understand this fully; am just adding on top of what is already there. Specifically, the current m_pageDismissalEventBeingDispatched variable is in FrameLoader, and not at the Page level, so that's why I'm passing a Frame (to get to its loader). I'll fix the const part in the next patch. >> Source/WebCore/page/Chrome.cpp:311 >> + return false; > > Can we factor this copy/paste code into a comment method? Also, these functions have the same Frame-or-Page issue. Done (for the common part). Again, not sure about the Frame/Page thing. >> Source/WebCore/page/ChromeClient.h:330 >> + virtual bool shouldRunModalDialogDuringPageDismissal(const DialogType&, const String&, FrameLoader::PageDismissalType) const { return true; } > > What is this string argument? We probably need a parameter name to know. Done. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:949 >> + PlatformBridge::histogramEnumeration("Renderer.ModalDialogsDuringPageDismissal", intDialog * numDismissalTypes + intDismissal, numDismissalTypes * numDialogTypes); > > This is a giant hack. Can we please not have these sorts of hacks? Do you mean that the histogram as a whole is a hack, or some specific part of this code? The histogram is needed; it's what let us collect the 2.3% stat we discussed on webkit-dev. I'd like to keep this around for a few more months, to see if there are any shifts in web developers' behaviour due to disallowing these modal dialogs. I have previously promised to remove the histogram once the data collection experiment is over; I certainly intend to honour that. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:957 >> + break; > > This code is all super redundant. Consider using String::format to make this code prettier. Done. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:972 >> + logMessage.append("Blocked modal dialog"); > > Please remove these default cases and explicitly handle all the enum values. I am handling all the enum cases explicitly (well, all except for the NumFoo enum value which is not expected to be used). The default is just a catch all, just in case somebody adds a new enum value and forgets to update this code.
Created attachment 99024 [details] Addressed most of abarth's comments
Comment on attachment 99024 [details] Addressed most of abarth's comments Attachment 99024 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8958058
Comment on attachment 99024 [details] Addressed most of abarth's comments Attachment 99024 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8958061
Comment on attachment 98931 [details] Now logging page dismissal type explicitly in the histograms View in context: https://bugs.webkit.org/attachment.cgi?id=98931&action=review >>> Source/WebCore/page/Chrome.cpp:209 >>> +bool Chrome::canRunModalNow(const Frame* frame) const >> >> We don't use "const Frame", just "Frame". >> >> Shouldn't this function be per-Page? If one of the frames is processing unload, that should probably suppress alerts in the whole Page. > > If a frame is navigating somewhere, it doesn't necessarily mean that the page is, right? It could just be a subframe alone that's navigating, with no change to the top-level page URL. > > I don't understand this fully; am just adding on top of what is already there. Specifically, the current m_pageDismissalEventBeingDispatched variable is in FrameLoader, and not at the Page level, so that's why I'm passing a Frame (to get to its loader). > > I'll fix the const part in the next patch. I had to put back the const, because DOMWindow.cpp only gets a const pointer (line 386). >>> Source/WebCore/page/ChromeClient.h:330 >>> + virtual bool shouldRunModalDialogDuringPageDismissal(const DialogType&, const String&, FrameLoader::PageDismissalType) const { return true; } >> >> What is this string argument? We probably need a parameter name to know. > > Done. I had to remove the parameter names because otherwise they give an "unused variable" error (since this is not a pure virtual function, but completely defined inline here). I've added a comment naming the params; if that's not proper webkit style, do let me know.
Created attachment 99029 [details] Addressed most of abarth's comments; fixed build errors
> (From update of attachment 98931 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98931&action=review > > >> Source/WebCore/page/Chrome.cpp:209 > >> +bool Chrome::canRunModalNow(const Frame* frame) const > > > > We don't use "const Frame", just "Frame". > > > > Shouldn't this function be per-Page? If one of the frames is processing unload, that should probably suppress alerts in the whole Page. > > If a frame is navigating somewhere, it doesn't necessarily mean that the page is, right? It could just be a subframe alone that's navigating, with no change to the top-level page URL. > > I don't understand this fully; am just adding on top of what is already there. Specifically, the current m_pageDismissalEventBeingDispatched variable is in FrameLoader, and not at the Page level, so that's why I'm passing a Frame (to get to its loader). Sure, that's fine. The point is that whether we can run a modal dialog is per-Page state, not per Frame state. That means you'll probably need to crawl the Frame tree to see if anyone is handling a page dismissal event. > >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:949 > >> + PlatformBridge::histogramEnumeration("Renderer.ModalDialogsDuringPageDismissal", intDialog * numDismissalTypes + intDismissal, numDismissalTypes * numDialogTypes); > > > > This is a giant hack. Can we please not have these sorts of hacks? > > Do you mean that the histogram as a whole is a hack, or some specific part of this code? The histogram is needed; it's what let us collect the 2.3% stat we discussed on webkit-dev. I'd like to keep this around for a few more months, to see if there are any shifts in web developers' behaviour due to disallowing these modal dialogs. I have previously promised to remove the histogram once the data collection experiment is over; I certainly intend to honour that. I meant the packing of a bunch of enum values into a single int. > >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:972 > >> + logMessage.append("Blocked modal dialog"); > > > > Please remove these default cases and explicitly handle all the enum values. > > I am handling all the enum cases explicitly (well, all except for the NumFoo enum value which is not expected to be used). The default is just a catch all, just in case somebody adds a new enum value and forgets to update this code. We generally leave off the default case and have the compiler alert us that we've added a new enum by breaking the build. That's another reason to remove the NumFoo enum values. > >>> Source/WebCore/page/Chrome.cpp:209 > >>> +bool Chrome::canRunModalNow(const Frame* frame) const > >> > >> We don't use "const Frame", just "Frame". > >> > >> Shouldn't this function be per-Page? If one of the frames is processing unload, that should probably suppress alerts in the whole Page. > > > > If a frame is navigating somewhere, it doesn't necessarily mean that the page is, right? It could just be a subframe alone that's navigating, with no change to the top-level page URL. > > > > I don't understand this fully; am just adding on top of what is already there. Specifically, the current m_pageDismissalEventBeingDispatched variable is in FrameLoader, and not at the Page level, so that's why I'm passing a Frame (to get to its loader). > > > > I'll fix the const part in the next patch. > > I had to put back the const, because DOMWindow.cpp only gets a const pointer (line 386). That code is wrong then. It's not sensible for Frame* to be const. Rather than spreading const to more places incorrectly, we should remove the offending consts. > >>> Source/WebCore/page/ChromeClient.h:330 > >>> + virtual bool shouldRunModalDialogDuringPageDismissal(const DialogType&, const String&, FrameLoader::PageDismissalType) const { return true; } > >> > >> What is this string argument? We probably need a parameter name to know. > > > > Done. > > I had to remove the parameter names because otherwise they give an "unused variable" error (since this is not a pure virtual function, but completely defined inline here). I've added a comment naming the params; if that's not proper webkit style, do let me know. The UNUSED_PARAM macro can help you solve this problem.
Comment on attachment 99029 [details] Addressed most of abarth's comments; fixed build errors View in context: https://bugs.webkit.org/attachment.cgi?id=99029&action=review Looks like we just have some const and Frame/Page nits to clean up. I'd like to give the whole patch one more read-through once we've got those sorted out. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:956 > + String logMessage = makeString("Blocked ", kDialogs[dialog], "('", dialogMessage, "') during ", kDismissals[dismissal], "."); > + const_cast<ChromeClientImpl*>(this)->addMessageToConsole(OtherMessageSource, LogMessageType, ErrorMessageLevel, logMessage, 0, String()); OMG, so much nicer. Thank you.
(In reply to comment #54) > Sure, that's fine. The point is that whether we can run a modal dialog is per-Page state, not per Frame state. That means you'll probably need to crawl the Frame tree to see if anyone is handling a page dismissal event. Done. > That code is wrong then. It's not sensible for Frame* to be const. Rather than spreading const to more places incorrectly, we should remove the offending consts. Okay, but now I don't need to pass the Frame anymore, so I haven't touched this code. I think removing const-ness belongs logically in a different patch anyway. No need to combine it with this. > The UNUSED_PARAM macro can help you solve this problem. Done.
Created attachment 99116 [details] Now checking all frames in the page
Comment on attachment 99116 [details] Now checking all frames in the page Attachment 99116 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8965132 New failing tests: fast/loader/page-dismissal-modal-dialogs.html
Created attachment 99133 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 99116 [details] Now checking all frames in the page Looks like your test doesn't pass.
(In reply to comment #60) > Looks like your test doesn't pass. Yeah, I found that strange. It passed on my local machine. Perhaps I did something wrong. While I'm at it, I'll add a couple of more tests to test the frame vs page stuff.
Created attachment 99227 [details] Updated test and expectations
Created attachment 99228 [details] Diff of test expectations from current files to newly added chromium-specific files
Comment on attachment 99227 [details] Updated test and expectations View in context: https://bugs.webkit.org/attachment.cgi?id=99227&action=review Very nice. Thanks for iterating on the patch. > Source/WebCore/page/Chrome.cpp:209 > +static bool canRunModalIfDuringPageDismissal(Page* page, ChromeClient* client, ChromeClient::DialogType dialog, const String& message) This is fine, but ChromeClient can be reached from Page, so you could just pass Page, but whatever.
Created attachment 99298 [details] Don't pass ChromeClient explicitly
(In reply to comment #64) > > Source/WebCore/page/Chrome.cpp:209 > > +static bool canRunModalIfDuringPageDismissal(Page* page, ChromeClient* client, ChromeClient::DialogType dialog, const String& message) > > This is fine, but ChromeClient can be reached from Page, so you could just pass Page, but whatever. Done. Thanks for the review! The new patch only differs from the old patch in the above issue (passing ChromeClient), so it should be a quick one for you to review. Please also add cq+ (I'm not a committer). Aside: Is there a way to see the diff between two patches using the web interface?
> Aside: Is there a way to see the diff between two patches using the web interface? There is, but it's super ugly and very few people use it.
Comment on attachment 99298 [details] Don't pass ChromeClient explicitly Ok. Let's rock and roll.
Comment on attachment 99298 [details] Don't pass ChromeClient explicitly Rejecting attachment 99298 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: gPageDismissal(const WebCore::ChromeClient::DialogType&, const WTF::String&, WebCore::FrameLoader::PageDismissalType) const': Source/WebKit/chromium/src/ChromeClientImpl.cpp:953: error: comparison between signed and unsigned integer expressions Source/WebKit/chromium/src/ChromeClientImpl.cpp:957: error: comparison between signed and unsigned integer expressions make: *** [out/Debug/obj.target/webkit/Source/WebKit/chromium/src/ChromeClientImpl.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/8957782
Created attachment 99337 [details] Fix debug build
Comment on attachment 99337 [details] Fix debug build Clearing flags on attachment: 99337 Committed r90164: <http://trac.webkit.org/changeset/90164>
All reviewed patches have been landed. Closing bug.