[webkit-reviews] review canceled: [Bug 221236] Introduce ImageOverlayElement and add UA shadow root support for image extraction : [Attachment 419365] Fix GTK/WPE builds

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 221236: Introduce ImageOverlayElement and add UA shadow root support for
image extraction
https://bugs.webkit.org/show_bug.cgi?id=221236

Attachment 419365: Fix GTK/WPE builds

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




--- Comment #15 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 419365
  --> https://bugs.webkit.org/attachment.cgi?id=419365
Fix GTK/WPE builds

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

>> Source/WebCore/dom/Element.cpp:4586
>> +	ensureUserAgentShadowRoot().appendChild(container);
> 
> Is updateWithImageExtractionResult() guaranteed to be only called once? I
don't see any code to update or remove the existing ImageOverlayElement.

In its current form, it will only be called once, yes. In the (near) future,
we'll need to be able to remove the overlay if (for instance) the image source
changes.

>> Source/WebCore/html/shadow/ImageOverlayElement.h:39
>> +class ImageOverlayElement final : public HTMLDivElement {
> 
> Why do we need this subclass? As far as I can see it currently has no data
members or unique behaviours in virtual functions. Could you just use
HTMLDivElement and put the related functions somewhere else?

I was planning to stash some additional members on `ImageOverlayElement` in a
future patch, but it looks like an alternative to that is perhaps keeping a map
of some sort on like `Document` or `Page`.

The `ImageOverlayElement` subclass also allows me to (more easily) identify one
of these image overlay elements through a type traits check…though I suppose I
could just check the class list?

>> Source/WebCore/style/UserAgentStyle.cpp:193
>> +	    }
> 
> Please don't add anything to default style sheet for this. Instead insert a
<style> element in the shadow tree. All selectors will be scoped to that tree
so nothing leaks out. See HTMLMeterElement::didAddUserAgentShadowRoot for an
example.

Whoops. Will do!