Bug 222953 - [macOS] Add a way to trigger webpage translation via the context menu
Summary: [macOS] Add a way to trigger webpage translation via the context menu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-08 16:20 PST by Wenson Hsieh
Modified: 2021-03-09 08:21 PST (History)
5 users (show)

See Also:

Attachments
Patch (39.76 KB, patch)
2021-03-08 16:48 PST, Wenson Hsieh 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix non-internal builds (39.80 KB, patch)
2021-03-08 17:05 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
For EWS (39.85 KB, patch)
2021-03-08 18:27 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-03-08 16:20:03 PST 
<rdar://problem/73901967>
Comment 1 Wenson Hsieh 2021-03-08 16:48:22 PST Comment hidden (obsolete)
Comment 2 Radar WebKit Bug Importer 2021-03-08 16:48:59 PST Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-03-08 17:05:15 PST 
Created attachment 422640 [details]
Fix non-internal builds
Comment 4 Devin Rousso 2021-03-08 17:13:56 PST 
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&`
Comment 5 Wenson Hsieh 2021-03-08 18:19:34 PST 
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&`

👍🏻
Comment 6 Wenson Hsieh 2021-03-08 18:23:39 PST 
> >> 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.
Comment 7 Wenson Hsieh 2021-03-08 18:27:17 PST 
Created attachment 422651 [details]
For EWS
Comment 8 EWS 2021-03-09 08:21:35 PST 
Committed r274148: <https://commits.webkit.org/r274148>

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