To help make the decision of whether to disallow modal dialogs during unload events (see bug 56397), we'd like to collect some data on how often users encounter such dialogs. This will likely only be collected in Chromium, through its histogram feature. It is intended to be a temporary measure; once we are satisfied with the data, the code enabling this will be completely reverted.
Created attachment 88878 [details] Patch
Created attachment 88880 [details] Test case I tested patch 88878 using this test case. After navigating away, about:histograms in chromium shows the histogram count has increased by 12 (+3 for each type of dialog), as expected.
Comment on attachment 88880 [details] Test case Please disable popup blocking before testing. Otherwise, showModalDialog() never returns, and it becomes very difficult to close the tab (see http://crbug.com/44357).
Comment on attachment 88878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88878&action=review > Source/WebCore/ChangeLog:8 > + > + No new tests. (OOPS!) You need to explain what kind of you change you're making here. Also, you need to replace "No new tests. (OOPS!)" by an explanation as to why you're not adding new tests. > Source/WebCore/page/Chrome.cpp:63 > +static inline void willRunModalDialog(const Frame* frame, const ChromeClient::DialogType& dialogType, const ChromeClient* client) > +{ > + if (frame->loader()->pageDismissalEventBeingDispatched()) > + client->willRunModalDialogDuringPageDismissal(dialogType); > +} > + I don't think we normally define a function like this at the top of a file. Instead, we put it right above where it first appears. In this case, right before Chrome::runJavaScriptAlert. > Source/WebKit/chromium/ChangeLog:7 > + You need to explain what kind of change you're making here.
Created attachment 88921 [details] Added detailed ChangeLog notes
Comment on attachment 88921 [details] Added detailed ChangeLog notes View in context: https://bugs.webkit.org/attachment.cgi?id=88921&action=review > Source/WebCore/page/Chrome.cpp:287 > +static inline bool isDuringPageDismissal(const Frame* frame) > +{ > + return frame->loader()->pageDismissalEventBeingDispatched(); > +} > + I don't think you extract this as a function. > Source/WebKit/chromium/ChangeLog:10 > + dispatched during unload events. Count the notifications through a > + histogram. I don't think you need to cut off a sentence awkwardly like this since WebKit doesn't have any line limits. You can put histogram in the previous line. > Source/WebKit/chromium/ChangeLog:13 > + No tests because it's not clear how to test chromium histograms from > + webkit. Ditto.
(In reply to comment #6) > (From update of attachment 88921 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88921&action=review > > > Source/WebCore/page/Chrome.cpp:287 > > +static inline bool isDuringPageDismissal(const Frame* frame) > > +{ > > + return frame->loader()->pageDismissalEventBeingDispatched(); > > +} > > + > > I don't think you extract this as a function. Meant to say "I don't think you should".
Created attachment 88922 [details] Updated test case Please disable popup blocking before testing. Otherwise, showModalDialog() never returns, and it becomes very difficult to close the tab (see http://crbug.com/44357). Test: 1. Open the test file in chromium built with this patch. 2. Disable popup blocking. 3. Click on the "Navigate away" link. 4. Click "OK" on all the dialogs that appear. 5. Go to "about:histograms". 6. The "Renderer.ModalDialogsDuringPageDismissal" histogram should have increased by 12 counts (+3 for each type of dialog).
Created attachment 88923 [details] Fixed ChangeLog line breaks
Comment on attachment 88921 [details] Added detailed ChangeLog notes View in context: https://bugs.webkit.org/attachment.cgi?id=88921&action=review >>> Source/WebCore/page/Chrome.cpp:287 >>> + >> >> I don't think you extract this as a function. > > Meant to say "I don't think you should". It will come in handy for the next patch (to actually disable dialogs), which uses this function in for example, canRunModalDialogNow(). I can unextract it now, and extract it out again in the next patch. Would you prefer that? >> Source/WebKit/chromium/ChangeLog:10 >> + histogram. > > I don't think you need to cut off a sentence awkwardly like this since WebKit doesn't have any line limits. You can put histogram in the previous line. Done. >> Source/WebKit/chromium/ChangeLog:13 >> + webkit. > > Ditto. Done.
Comment on attachment 88878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88878&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > You need to explain what kind of you change you're making here. Also, you need to replace "No new tests. (OOPS!)" by an explanation as to why you're not adding new tests. Done. >> Source/WebCore/page/Chrome.cpp:63 >> + > > I don't think we normally define a function like this at the top of a file. Instead, we put it right above where it first appears. In this case, right before Chrome::runJavaScriptAlert. Done. >> Source/WebKit/chromium/ChangeLog:7 >> + > > You need to explain what kind of change you're making here. Done.
(In reply to comment #10) > (From update of attachment 88921 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88921&action=review > > >>> Source/WebCore/page/Chrome.cpp:287 > > Meant to say "I don't think you should". > > It will come in handy for the next patch (to actually disable dialogs), which uses this function in for example, canRunModalDialogNow(). I can unextract it now, and extract it out again in the next patch. Would you prefer that? Yes, I'd prefer that.
Comment on attachment 88921 [details] Added detailed ChangeLog notes View in context: https://bugs.webkit.org/attachment.cgi?id=88921&action=review >>>>> Source/WebCore/page/Chrome.cpp:287 >>>>> + >>>> >>>> I don't think you extract this as a function. >>> >>> Meant to say "I don't think you should". >> >> It will come in handy for the next patch (to actually disable dialogs), which uses this function in for example, canRunModalDialogNow(). I can unextract it now, and extract it out again in the next patch. Would you prefer that? > > Yes, I'd prefer that. Done.
Created attachment 88924 [details] Unextracted the function
By the way, you don't have to ask for another review when fixing per my comment after r+. Just ask for cq.
(In reply to comment #15) > By the way, you don't have to ask for another review when fixing per my comment after r+. Just ask for cq. Noted. I see the --no-review option to webkit-patch upload. But I didn't see an r+ from you on the earlier patches. Perhaps it was a mid-air collision. Thanks for the review and approval!
Comment on attachment 88924 [details] Unextracted the function Clearing flags on attachment: 88924 Committed r83375: <http://trac.webkit.org/changeset/83375>
All reviewed patches have been landed. Closing bug.