[webkit-reviews] review granted: [Bug 222953] [macOS] Add a way to trigger webpage translation via the context menu : [Attachment 422640] Fix non-internal builds

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 222953: [macOS] Add a way to trigger webpage translation via the context
menu
https://bugs.webkit.org/show_bug.cgi?id=222953

Attachment 422640: Fix non-internal builds

https://bugs.webkit.org/attachment.cgi?id=422640&action=review




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 422640
  --> https://bugs.webkit.org/attachment.cgi?id=422640
Fix non-internal builds

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

r=me

> Source/WebCore/page/ContextMenuContext.h:47
> +    IntRect selectionBounds() const { return m_selectionBounds; }

`const IntRect&`

> Source/WebCore/page/ContextMenuController.cpp:846
> +	   auto selectedString = m_context.hitTestResult().selectedText();

NIT: I dunno if it's actually more efficient/performant/etc, but I usually mark
stuff like this as `auto&`.

> Source/WebCore/page/ContextMenuController.cpp:849
> +	   if (auto view = makeRefPtr(frame->view()); view &&
!selectedString.isEmpty()) {

Is it worth putting this inside a `if (!selectedString.isEmpty()) { ... }` to
avoid churning the ref count if not?

> Source/WebKit/Shared/ContextMenuContextData.h:64
> +    WebCore::IntRect selectionBounds() const { return m_selectionBounds; }

`const WebCore::IntRect&`