Bug 227186 - Remove redundant HitTestLocation(const LayoutPoint& centerPoint, unsigned topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned leftPadding)
Summary: Remove redundant HitTestLocation(const LayoutPoint& centerPoint, unsigned top...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on: 227182 227185
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-19 15:51 PDT by zalan
Modified: 2021-06-21 08:09 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.66 KB, patch)
2021-06-19 15:57 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (9.66 KB, patch)
2021-06-20 06:26 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (9.60 KB, patch)
2021-06-20 21:13 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (9.55 KB, patch)
2021-06-21 05:48 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2021-06-19 15:51:11 PDT
Let's replace the _one_ caller (Internals) with the LayoutRect version.
Comment 1 zalan 2021-06-19 15:57:21 PDT
Created attachment 431807 [details]
Patch
Comment 2 zalan 2021-06-20 06:26:41 PDT
Created attachment 431819 [details]
Patch
Comment 3 Sam Weinig 2021-06-20 13:15:14 PDT
Comment on attachment 431819 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:2323
> +    auto hitTestResult = HitTestResult { };
> +    auto size = LayoutSize { leftPadding + rightPadding, topPadding + bottomPadding };
> +    if (size.isEmpty())
> +        hitTestResult = HitTestResult { point };
> +    else {
> +        auto adjustedPosition = LayoutPoint { flooredIntPoint(point) };
> +        adjustedPosition -= LayoutSize  { leftPadding, topPadding };
> +        hitTestResult = HitTestResult { LayoutRect { adjustedPosition, size } };
> +    }

I would usually do this using a lambda so that hitTestResult is not assigned twice.

Can the two adjustedPosition lines be written as one line? 

auto adjustedPosition = LayoutPoint { flooredIntPoint(point) } - LayoutSize { leftPadding, topPadding };

?
Comment 4 zalan 2021-06-20 21:12:14 PDT
(In reply to Sam Weinig from comment #3)
> Comment on attachment 431819 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431819&action=review
> 
> > Source/WebCore/testing/Internals.cpp:2323
> > +    auto hitTestResult = HitTestResult { };
> > +    auto size = LayoutSize { leftPadding + rightPadding, topPadding + bottomPadding };
> > +    if (size.isEmpty())
> > +        hitTestResult = HitTestResult { point };
> > +    else {
> > +        auto adjustedPosition = LayoutPoint { flooredIntPoint(point) };
> > +        adjustedPosition -= LayoutSize  { leftPadding, topPadding };
> > +        hitTestResult = HitTestResult { LayoutRect { adjustedPosition, size } };
> > +    }
> 
> I would usually do this using a lambda so that hitTestResult is not assigned
> twice.
I normally go overboard with lambdas, not sure what happened here. :)

> 
> Can the two adjustedPosition lines be written as one line? 
> 
> auto adjustedPosition = LayoutPoint { flooredIntPoint(point) } - LayoutSize
> { leftPadding, topPadding };
> 
> ?
Yup

Thanks.
Comment 5 zalan 2021-06-20 21:13:14 PDT
Created attachment 431830 [details]
Patch
Comment 6 zalan 2021-06-21 05:48:26 PDT
Created attachment 431850 [details]
Patch
Comment 7 EWS 2021-06-21 08:08:52 PDT
Committed r279066 (238986@main): <https://commits.webkit.org/238986@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431850 [details].
Comment 8 Radar WebKit Bug Importer 2021-06-21 08:09:46 PDT
<rdar://problem/79562100>