Bug 236842 – [Live Text] Add a mechanism to inject images into image overlays

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

[Wenson Hsieh]

.

[Wenson Hsieh]

Created attachment 452534 [details]
For EWS

[Wenson Hsieh]

Created attachment 452535 [details]
Fix watchOS build

[Wenson Hsieh]

Created attachment 452536 [details]
Fix Big Sur build

[Wenson Hsieh]

Created attachment 452540 [details]
Fix non-internal iOS build

[Wenson Hsieh]

Created attachment 452545 [details]
Fix watchOS build (again)

[Aditya Keerthi]

Comment on attachment 452545 [details]
Fix watchOS build (again)

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

> Source/WebCore/dom/ImageOverlay.cpp:227
> +static void installStyleSheet(ShadowRoot& shadowRoot)

Consider renaming to `installImageOverlayStyleSheet`.

> Source/WebCore/dom/ImageOverlay.cpp:679
> +    RefPtr imageOverlayRoot = dynamicDowncast<HTMLDivElement>(static_cast<TreeScope&>(shadowRoot.get()).getElementById(imageOverlayElementIdentifier()));

Is the static cast to `TreeScope` necessary?

> Source/WebCore/dom/ImageOverlay.h:28
> +#include "FloatRect.h"

Can this be forward declared?

[Wenson Hsieh]

Comment on attachment 452545 [details]
Fix watchOS build (again)

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

Thanks for the review!

>> Source/WebCore/dom/ImageOverlay.cpp:227
>> +static void installStyleSheet(ShadowRoot& shadowRoot)
> 
> Consider renaming to `installImageOverlayStyleSheet`.

Makes sense! Renamed.

>> Source/WebCore/dom/ImageOverlay.cpp:679
>> +    RefPtr imageOverlayRoot = dynamicDowncast<HTMLDivElement>(static_cast<TreeScope&>(shadowRoot.get()).getElementById(imageOverlayElementIdentifier()));
> 
> Is the static cast to `TreeScope` necessary?

That's a good question…the issue is that `getElementById(const AtomString&)` exists on both TreeScope and DocumentFragment, so the compiler doesn't know which one to pick :/

I tried to work around this by initializing a String and using the `const String&` version, but that didn't seem to alleviate the issue from above. I think I'll keep it as is (though, maybe there's a cleaner way to avoid this?)

>> Source/WebCore/dom/ImageOverlay.h:28
>> +#include "FloatRect.h"
> 
> Can this be forward declared?

Good catch — fixed!

[Wenson Hsieh]

Created attachment 452567 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Wenson Hsieh from comment #7)
> Comment on attachment 452545 [details]
> Fix watchOS build (again)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452545&action=review
> 
> Thanks for the review!
> 
> >> Source/WebCore/dom/ImageOverlay.cpp:227
> >> +static void installStyleSheet(ShadowRoot& shadowRoot)
> > 
> > Consider renaming to `installImageOverlayStyleSheet`.
> 
> Makes sense! Renamed.
> 
> >> Source/WebCore/dom/ImageOverlay.cpp:679
> >> +    RefPtr imageOverlayRoot = dynamicDowncast<HTMLDivElement>(static_cast<TreeScope&>(shadowRoot.get()).getElementById(imageOverlayElementIdentifier()));
> > 
> > Is the static cast to `TreeScope` necessary?
> 
> That's a good question…the issue is that `getElementById(const AtomString&)`
> exists on both TreeScope and DocumentFragment, so the compiler doesn't know
> which one to pick :/
> 
> I tried to work around this by initializing a String and using the `const
> String&` version, but that didn't seem to alleviate the issue from above. I
> think I'll keep it as is (though, maybe there's a cleaner way to avoid this?)

I think we can solve this by adding `using TreeScope::getElementById;` in `ShadowRoot.h`.

Note that the `DocumentFragment` implementation already calls into the `TreeScope` implementation for shadow roots.
 
> >> Source/WebCore/dom/ImageOverlay.h:28
> >> +#include "FloatRect.h"
> > 
> > Can this be forward declared?
> 
> Good catch — fixed!

[Wenson Hsieh]

Comment on attachment 452545 [details]
Fix watchOS build (again)

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

>>>> Source/WebCore/dom/ImageOverlay.cpp:227
>>>> +static void installStyleSheet(ShadowRoot& shadowRoot)
>>> 
>>> Consider renaming to `installImageOverlayStyleSheet`.
>> 
>> Makes sense! Renamed.
> 
> I think we can solve this by adding `using TreeScope::getElementById;` in `ShadowRoot.h`.
> 
> Note that the `DocumentFragment` implementation already calls into the `TreeScope` implementation for shadow roots.

Thanks! That works — and it also looks like we do something similar already for `rootNode()` on ShadowRoot, so there's precedent.

I'll add that, and replace all the static_casts to TreeScope here.

[Wenson Hsieh]

Created attachment 452577 [details]
Patch for landing

[EWS]

Committed r290178 (247508@main): <https://commits.webkit.org/247508@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89169696>