[webkit-reviews] review denied: [Bug 56397] Suppress modal JavaScript/HTML dialogs during unload events : [Attachment 98931] Now logging page dismissal type explicitly in the histograms

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Sreeram Ramachandran
<sreeram at chromium.org>'s request for review:
Bug 56397: Suppress modal JavaScript/HTML dialogs during unload events
https://bugs.webkit.org/show_bug.cgi?id=56397

Attachment 98931: Now logging page dismissal type explicitly in the histograms
https://bugs.webkit.org/attachment.cgi?id=98931&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.