[webkit-reviews] review granted: [Bug 61946] FrameLoaderClient::didRunInsecureContent - no way to distinguish between blocked/run cases. : [Attachment 96137] Call into permissions client on insecure content.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Thomas Sepez <tsepez at chromium.org>'s
request for review:
Bug 61946: FrameLoaderClient::didRunInsecureContent - no way to distinguish
between blocked/run cases.
https://bugs.webkit.org/show_bug.cgi?id=61946

Attachment 96137: Call into permissions client on insecure content.
https://bugs.webkit.org/attachment.cgi?id=96137&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96137&action=review

We could add a Chromium WebKit API test to show that the overrides work.  You
can look at how we test the other allow methods, if you want some inspiration.

> Source/WebCore/loader/FrameLoader.cpp:1119
> +    bool allowedPerSettings = settings &&
settings->allowDisplayOfInsecureContent();
> +    bool allowed =
m_client->allowDisplayingInsecureContent(allowedPerSettings);

I'd just inline the settings && settings->allowDisplayOfInsecureContent()
expression into the client call and get rid of the temporary, but that's just a
style nit.

> Source/WebCore/loader/FrameLoader.cpp:1126
> -    m_client->didDisplayInsecureContent();
> +    if (allowed)
> +	   m_client->didDisplayInsecureContent();

I'd send the URL through too.

> Source/WebCore/loader/FrameLoaderClient.h:301
> +	   virtual bool allowDisplayingInsecureContent(bool enabledPerSettings)
{ return enabledPerSettings; }

No URL here?