[webkit-reviews] review granted: [Bug 237120] WebKit continues to render PDF images in Captive Portal mode : [Attachment 453248] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 237120: WebKit continues to render PDF images in Captive Portal mode
https://bugs.webkit.org/show_bug.cgi?id=237120

Attachment 453248: Patch

https://bugs.webkit.org/attachment.cgi?id=453248&action=review




--- Comment #7 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 453248
  --> https://bugs.webkit.org/attachment.cgi?id=453248
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453248&action=review

r=me with changes.

> Source/WebCore/page/RuntimeEnabledFeatures.h:204
> +    void setPDFImagesEnabled(bool isEnabled) { m_pdfImagesEnabled =
isEnabled; }

Per WebKit coding style, Boolean variables need a prefix. Should be something
like:
void setArePDFImagesEnabled(bool arePDFImagesEnabled) { m_arePDFImagesEnabled =
arePDFImagesEnabled; }

> Source/WebCore/page/RuntimeEnabledFeatures.h:205
> +    bool pdfImagesEnabled() const { return m_pdfImagesEnabled; }

bool arePDFImagesEnabled() const { return m_arePDFImagesEnabled; }

> Source/WebCore/page/RuntimeEnabledFeatures.h:360
> +    bool m_pdfImagesEnabled { true };

ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4074
> +	   RuntimeEnabledFeatures::sharedFeatures().setPDFImagesEnabled(false);

This has nothing to do with the page, it is process-wide.
As a result, I think this call should be in the WebProcess constructor, right
after we initialize WebProcess::m_isCaptivePortalModeEnabled.

> Tools/ChangeLog:20
> +	   * TestWebKitAPI/Tests/WebKitCocoa/missing.png: Added.

I think we can probably omit this one.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CaptivePortalPDF.html:3
> +	   <style>

Why the styling for an API test?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CaptivePortalPDF.html:20
> +	   <h1>Captive Portal Mode - PDF Test</h1>

This is not a layout test and I don't think we need so much HTML code for an
API test. I don't think these tests are expected to work when loading manually
in the browser.
It's not like the window.webkit.messageHandlers.testHandler would exist in the
browser anyway.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CaptivePortalPDF.html:30
> +		   <td><img width="150px" height="100px"
src="missing.png"></td>

What's the purpose on the second image in this test? Seems like we could just
omit it, or am I missing something?