[webkit-reviews] review granted: [Bug 187574] Allow removal of white backgrounds : [Attachment 345125] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 187574: Allow removal of white backgrounds
https://bugs.webkit.org/show_bug.cgi?id=187574

Attachment 345125: Patch

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




--- Comment #28 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 345125
  --> https://bugs.webkit.org/attachment.cgi?id=345125
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:1367
> +	   WTFLogAlways("punching out inline");

Nope.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:952
> +		   context.fillRect(backgroundRectForPainting, bgColor,
operation); // FIXME: use op if it is DestinationOut?

Remove or fix the fixme.

> Source/WebCore/rendering/RenderTableCell.cpp:1293
> +	   WTFLogAlways("punching out table cell");

Nope.

> Source/WebCore/rendering/RenderTheme.h:256
> +    virtual bool usingDarkAppearance(const Page&) const { return false; }

Can RenderTheme really not get to Page()? Maybe just pass the renderer?

> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:173
> +    if (options.punchOutWhiteBackgroundsInDarkMode)
> +	   m_mainWebView->setDrawsBackground(false);

Hmmmm, weird. Those things naively seem unrelated.

You also need to reset this between tests.