Bug 222953 – [macOS] Add a way to trigger webpage translation via the context menu

• Component: Platform
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/73901967>

[Wenson Hsieh]

Created attachment 422637 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75194080>

[Wenson Hsieh]

Created attachment 422640 [details]
Fix non-internal builds

[Devin Rousso]

Comment on attachment 422640 [details]
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&`

[Wenson Hsieh]

Comment on attachment 422640 [details]
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?

Good call!

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

👍🏻

[Wenson Hsieh]

> >> 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&`.
> 
> 👍🏻

Actually, never mind — this doesn't work here because selectedText() returns a temporary value.

[Wenson Hsieh]

Created attachment 422651 [details]
For EWS

[EWS]

Committed r274148: <https://commits.webkit.org/r274148>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422651 [details].