.
Created attachment 424356 [details] Patch
<rdar://problem/75886351>
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?
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
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 👍🏻
Created attachment 424393 [details] Patch for landing
Committed r275112: <https://commits.webkit.org/r275112> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424393 [details].