RESOLVED FIXED 222953
[macOS] Add a way to trigger webpage translation via the context menu
https://bugs.webkit.org/show_bug.cgi?id=222953
Summary [macOS] Add a way to trigger webpage translation via the context menu
Wenson Hsieh
Reported 2021-03-08 16:20:03 PST
Attachments
Patch (39.76 KB, patch)
2021-03-08 16:48 PST, Wenson Hsieh
ews-feeder: commit-queue-
Fix non-internal builds (39.80 KB, patch)
2021-03-08 17:05 PST, Wenson Hsieh
no flags
For EWS (39.85 KB, patch)
2021-03-08 18:27 PST, Wenson Hsieh
no flags
Wenson Hsieh
Comment 1 2021-03-08 16:48:22 PST Comment hidden (obsolete)
Radar WebKit Bug Importer
Comment 2 2021-03-08 16:48:59 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 3 2021-03-08 17:05:15 PST
Created attachment 422640 [details] Fix non-internal builds
Devin Rousso
Comment 4 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&`
Wenson Hsieh
Comment 5 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&` 👍🏻
Wenson Hsieh
Comment 6 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.
Wenson Hsieh
Comment 7 2021-03-08 18:27:17 PST
EWS
Comment 8 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].
Note You need to log in before you can comment on or make changes to this bug.