Bug 223781 – Allow some image overlay content to render in fully transparent image elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 424356 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75886351>

[Tim Horton]

Comment on attachment 424356 [details]
Patch

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

> Source/WebCore/page/ImageOverlayController.cpp:86
> +        if (overlayHostRenderer->style().opacity() > 0.01)
> +            return nullptr;

This is sneakily hidden, and I almost missed it. Maybe possible to make it obvious that this is all inert in the "normal" case?

[Chris Dumez]

Comment on attachment 424356 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:2624
>      if (auto* page = this->page()) {

Can we merge with this block?

> Source/WebCore/page/ImageOverlayController.cpp:54
> +ImageOverlayController::~ImageOverlayController() = default;

Why do we need this destructor?

> Source/WebCore/page/ImageOverlayController.h:46
> +    ImageOverlayController(Page&);

explicit?

> Source/WebCore/page/ImageOverlayController.h:53
> +    void willMoveToPage(PageOverlay&, Page*) override;

final

> Source/WebCore/page/ImageOverlayController.h:54
> +    void didMoveToPage(PageOverlay&, Page*) override { }

final

> Source/WebCore/page/ImageOverlayController.h:55
> +    void drawRect(PageOverlay&, GraphicsContext&, const IntRect& dirtyRect) override;

final

[Wenson Hsieh]

Comment on attachment 424356 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:2624
>>      if (auto* page = this->page()) {
> 
> Can we merge with this block?

Yes — done!

>> Source/WebCore/page/ImageOverlayController.cpp:54
>> +ImageOverlayController::~ImageOverlayController() = default;
> 
> Why do we need this destructor?

Whoops, we don't! Good catch.

>> Source/WebCore/page/ImageOverlayController.cpp:86
>> +            return nullptr;
> 
> This is sneakily hidden, and I almost missed it. Maybe possible to make it obvious that this is all inert in the "normal" case?

Yeah, that's a good point. I'm planning to split this bit out into a helper method, with a brief comment:

```
bool ImageOverlayController::shouldUsePageOverlayToPaintSelection(const RenderElement& renderer)
{
    // If the selection is already painted (with nonzero opacity) in the overlay host's renderer,
    // then we don't need to fall back to a page overlay to paint the selection.
    return renderer.style().opacity() <= 0.01;
}
```

>> Source/WebCore/page/ImageOverlayController.h:46
>> +    ImageOverlayController(Page&);
> 
> explicit?

👍🏻

>> Source/WebCore/page/ImageOverlayController.h:53
>> +    void willMoveToPage(PageOverlay&, Page*) override;
> 
> final

👍🏻

>> Source/WebCore/page/ImageOverlayController.h:54
>> +    void didMoveToPage(PageOverlay&, Page*) override { }
> 
> final

👍🏻

>> Source/WebCore/page/ImageOverlayController.h:55
>> +    void drawRect(PageOverlay&, GraphicsContext&, const IntRect& dirtyRect) override;
> 
> final

👍🏻

[Wenson Hsieh]

Created attachment 424393 [details]
Patch for landing

[EWS]

Committed r275112: <https://commits.webkit.org/r275112>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424393 [details].