Bug 225038 – Subdivide image overlay text into one or more elements per line

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://75505043

[Wenson Hsieh]

Created attachment 427015 [details]
For EWS

[Wenson Hsieh]

Created attachment 427017 [details]
Fix non-internal builds

[Devin Rousso]

Comment on attachment 427017 [details]
Fix non-internal builds

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

r=me as well :)

> Source/WebCore/html/HTMLElement.cpp:1348
> +        if (lineQuad.isEmpty())
>              continue;

Should/Can we check this before `scale`?

> Source/WebCore/html/HTMLElement.cpp:1354
> +        auto lineBounds = rotatedBoundingRect(lineQuad, 0.01);

NIT: Can we pull this into a `constexpr auto minRotationInRadians = 0.01;` or something?

> Source/WebCore/html/HTMLElement.cpp:1360
> +            std::round(lineBounds.center.x() - lineBounds.size.width() / 2), "px, "_s,
> +            std::round(lineBounds.center.y() - lineBounds.size.height() / 2), "px) "_s,

NIT: I personally like to add parenthesis to avoid any chance of order-of-operations confusion (both from the compiler and from the reader):
```
    std::round(lineBounds.center.x() - (lineBounds.size.width() / 2)), "px, "_s,
    std::round(lineBounds.center.y() - (lineBounds.size.height() / 2)), "px) "_s,
```

> Source/WebCore/html/HTMLElement.cpp:1361
> +            lineBounds.angleInRadians ? makeString("rotate(", lineBounds.angleInRadians, "rad) "_s) : emptyString()

`"rotate("_s`

> Source/WebCore/html/HTMLElement.cpp:1419
> +                    adjustLayoutUnitForAbsoluteZoom(renderer->offsetHeight(), *renderer).toFloat()

you could add a trailing comma here

> Source/WebCore/html/shadow/imageOverlay.css:33
> +    font-size: 1024px;

o_0 this probably deserves a comment

> Source/WebCore/platform/ImageExtractionResult.h:37
> +    ImageExtractionTextData(const String& theText, const FloatQuad& quad)

I wonder if we could make this `FloatQuad&&` instead.

> Source/WebCore/platform/ImageExtractionResult.h:72
> +    ImageExtractionLineData(const FloatQuad& quad, const Vector<ImageExtractionTextData>& theChildren)

I wonder if we could make these `FloatQuad&&` and `Vector<ImageExtractionTextData>&&` instead.

[Wenson Hsieh]

Comment on attachment 427017 [details]
Fix non-internal builds

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

Tim and I chatted

>> Source/WebCore/html/HTMLElement.cpp:1348
>>              continue;
> 
> Should/Can we check this before `scale`?

👍🏻

>> Source/WebCore/html/HTMLElement.cpp:1354
>> +        auto lineBounds = rotatedBoundingRect(lineQuad, 0.01);
> 
> NIT: Can we pull this into a `constexpr auto minRotationInRadians = 0.01;` or something?

Per Slack discussion with Tim, I'm renaming `rotatedBoundingRect` to `rotatedBoundingRectWithMinimumAngleOfRotation`, which should help clear the confusion.

>> Source/WebCore/html/HTMLElement.cpp:1360
>> +            std::round(lineBounds.center.y() - lineBounds.size.height() / 2), "px) "_s,
> 
> NIT: I personally like to add parenthesis to avoid any chance of order-of-operations confusion (both from the compiler and from the reader):
> ```
>     std::round(lineBounds.center.x() - (lineBounds.size.width() / 2)), "px, "_s,
>     std::round(lineBounds.center.y() - (lineBounds.size.height() / 2)), "px) "_s,
> ```

👍🏻

>> Source/WebCore/html/HTMLElement.cpp:1361
>> +            lineBounds.angleInRadians ? makeString("rotate(", lineBounds.angleInRadians, "rad) "_s) : emptyString()
> 
> `"rotate("_s`

Good catch. Fixed!

>> Source/WebCore/html/HTMLElement.cpp:1419
>> +                    adjustLayoutUnitForAbsoluteZoom(renderer->offsetHeight(), *renderer).toFloat()
> 
> you could add a trailing comma here

👍🏻

>> Source/WebCore/html/shadow/imageOverlay.css:33
>> +    font-size: 1024px;
> 
> o_0 this probably deserves a comment

Yep, adding one.

>> Source/WebCore/platform/ImageExtractionResult.h:37
>> +    ImageExtractionTextData(const String& theText, const FloatQuad& quad)
> 
> I wonder if we could make this `FloatQuad&&` instead.

We could, though I don't think it'll make much of a difference. I'll change It to use && anyways.

>> Source/WebCore/platform/ImageExtractionResult.h:72
>> +    ImageExtractionLineData(const FloatQuad& quad, const Vector<ImageExtractionTextData>& theChildren)
> 
> I wonder if we could make these `FloatQuad&&` and `Vector<ImageExtractionTextData>&&` instead.

Ditto.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 427017 [details]
> Fix non-internal builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427017&action=review
> 
> Tim and I chatted

(This unfinished sentence was supposed to be about the name of `WebCore::rotatedBoundingRect`, but I wrote what I was going to write here in an inline comment anyways)

[Wenson Hsieh]

Created attachment 427107 [details]
For EWS

[Wenson Hsieh]

Created attachment 427109 [details]
For EWS

[EWS]

Committed r276626 (237054@main): <https://commits.webkit.org/237054@main>

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