Bug 58115 - Gather data on modal dialogs shown during unload events
Summary: Gather data on modal dialogs shown during unload events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 56397
  Show dependency treegraph
 
Reported: 2011-04-07 20:08 PDT by Sreeram Ramachandran
Modified: 2011-06-07 15:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.31 KB, patch)
2011-04-08 15:03 PDT, Sreeram Ramachandran
no flags Details | Formatted Diff | Diff
Test case (674 bytes, text/html)
2011-04-08 15:09 PDT, Sreeram Ramachandran
no flags Details
Added detailed ChangeLog notes (8.05 KB, patch)
2011-04-09 00:01 PDT, Sreeram Ramachandran
no flags Details | Formatted Diff | Diff
Updated test case (711 bytes, text/html)
2011-04-09 00:07 PDT, Sreeram Ramachandran
no flags Details
Fixed ChangeLog line breaks (8.03 KB, patch)
2011-04-09 00:14 PDT, Sreeram Ramachandran
no flags Details | Formatted Diff | Diff
Unextracted the function (7.92 KB, patch)
2011-04-09 00:23 PDT, Sreeram Ramachandran
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sreeram Ramachandran 2011-04-07 20:08:46 PDT
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.
Comment 1 Sreeram Ramachandran 2011-04-08 15:03:18 PDT
Created attachment 88878 [details]
Patch
Comment 2 Sreeram Ramachandran 2011-04-08 15:09:58 PDT
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 3 Sreeram Ramachandran 2011-04-08 15:19:17 PDT
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 4 Ryosuke Niwa 2011-04-08 23:13:48 PDT
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.
Comment 5 Sreeram Ramachandran 2011-04-09 00:01:16 PDT
Created attachment 88921 [details]
Added detailed ChangeLog notes
Comment 6 Ryosuke Niwa 2011-04-09 00:06:15 PDT
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.
Comment 7 Ryosuke Niwa 2011-04-09 00:06:54 PDT
(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".
Comment 8 Sreeram Ramachandran 2011-04-09 00:07:32 PDT
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).
Comment 9 Sreeram Ramachandran 2011-04-09 00:14:09 PDT
Created attachment 88923 [details]
Fixed ChangeLog line breaks
Comment 10 Sreeram Ramachandran 2011-04-09 00:16:17 PDT
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 11 Sreeram Ramachandran 2011-04-09 00:16:40 PDT
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.
Comment 12 Ryosuke Niwa 2011-04-09 00:18:05 PDT
(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 13 Sreeram Ramachandran 2011-04-09 00:23:03 PDT
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.
Comment 14 Sreeram Ramachandran 2011-04-09 00:23:34 PDT
Created attachment 88924 [details]
Unextracted the function
Comment 15 Ryosuke Niwa 2011-04-09 00:23:46 PDT
By the way, you don't have to ask for another review when fixing per my comment after r+. Just ask for cq.
Comment 16 Sreeram Ramachandran 2011-04-09 00:27:30 PDT
(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 17 WebKit Commit Bot 2011-04-09 01:01:36 PDT
Comment on attachment 88924 [details]
Unextracted the function

Clearing flags on attachment: 88924

Committed r83375: <http://trac.webkit.org/changeset/83375>
Comment 18 WebKit Commit Bot 2011-04-09 01:01:46 PDT
All reviewed patches have been landed.  Closing bug.