Bug 225038

Summary: Subdivide image overlay text into one or more elements per line
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: PlatformAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cdumez, changseok, drousso, esprehn+autocc, ews-watchlist, gyuyoung.kim, megan_gardner, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For EWS
ews-feeder: commit-queue-
Fix non-internal builds
none
For EWS
none
For EWS none

Description Wenson Hsieh 2021-04-25 16:38:10 PDT
rdar://75505043
Comment 1 Wenson Hsieh 2021-04-25 17:50:06 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-04-25 18:09:15 PDT
Created attachment 427017 [details]
Fix non-internal builds
Comment 3 Devin Rousso 2021-04-26 14:19:07 PDT
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.
Comment 4 Wenson Hsieh 2021-04-26 15:42:09 PDT
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.
Comment 5 Wenson Hsieh 2021-04-26 15:44:00 PDT
(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)
Comment 6 Wenson Hsieh 2021-04-26 17:01:55 PDT Comment hidden (obsolete)
Comment 7 Wenson Hsieh 2021-04-26 17:02:24 PDT
Created attachment 427109 [details]
For EWS
Comment 8 EWS 2021-04-26 19:59:25 PDT
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].