Bug 58115 – Gather data on modal dialogs shown during unload events

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Sreeram Ramachandran]

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.

[Sreeram Ramachandran]

Created attachment 88878 [details]
Patch

[Sreeram Ramachandran]

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.

[Sreeram Ramachandran]

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

[Ryosuke Niwa]

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.

[Sreeram Ramachandran]

Created attachment 88921 [details]
Added detailed ChangeLog notes

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

(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".

[Sreeram Ramachandran]

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

[Sreeram Ramachandran]

Created attachment 88923 [details]
Fixed ChangeLog line breaks

[Sreeram Ramachandran]

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.

[Sreeram Ramachandran]

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.

[Ryosuke Niwa]

(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.

[Sreeram Ramachandran]

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.

[Sreeram Ramachandran]

Created attachment 88924 [details]
Unextracted the function

[Ryosuke Niwa]

By the way, you don't have to ask for another review when fixing per my comment after r+. Just ask for cq.

[Sreeram Ramachandran]

(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!

[WebKit Commit Bot]

Comment on attachment 88924 [details]
Unextracted the function

Clearing flags on attachment: 88924

Committed r83375: <http://trac.webkit.org/changeset/83375>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.