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

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