Summary: | Subdivide image overlay text into one or more elements per line | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | Platform | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bdakin, cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, megan_gardner, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2021-04-25 16:38:10 PDT
Created attachment 427015 [details]
For EWS
Created attachment 427017 [details]
Fix non-internal builds
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 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. (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) Created attachment 427107 [details]
For EWS
Created attachment 427109 [details]
For 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]. |