[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 bugzilla-daemon at webkit.org
Mon Mar 8 17:13:56 PST 2021

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

Attachment 422640: Fix non-internal builds


--- 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


> 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&`

More information about the webkit-reviews mailing list