WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
<
rdar://problem/73901967
>
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 attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2021-03-08 16:48:22 PST
Comment hidden (obsolete)
Created
attachment 422637
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2021-03-08 16:48:59 PST
Comment hidden (obsolete)
<
rdar://problem/75194080
>
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
Created
attachment 422651
[details]
For EWS
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.
Top of Page
Format For Printing
XML
Clone This Bug