Bug 224683 - [macOS] Add some support for webpage translation in WebKitLegacy
Summary: [macOS] Add some support for webpage translation in WebKitLegacy
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: 224680
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-16 11:09 PDT by Wenson Hsieh
Modified: 2021-04-17 21:25 PDT (History)
7 users (show)

See Also:


Attachments
Blocked on #224680 (7.24 KB, patch)
2021-04-16 13:19 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Blocked on #224680 (9.65 KB, patch)
2021-04-16 13:45 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix iOS 14/Big Sur builds (17.36 KB, patch)
2021-04-16 17:20 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Fix iOS 14/Big Sur builds (2) (17.41 KB, patch)
2021-04-16 17:44 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
Patch for landing (17.42 KB, patch)
2021-04-17 20:51 PDT, 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-04-16 11:09:16 PDT
<rdar://problem/75641882>
Comment 1 Wenson Hsieh 2021-04-16 13:19:26 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-04-16 13:45:48 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2021-04-16 16:17:42 PDT
Comment on attachment 426269 [details]
Blocked on #224680

(This is still blocked on the other bug — I'm going to retry all EWS once it's able to apply)
Comment 4 Wenson Hsieh 2021-04-16 17:20:39 PDT Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2021-04-16 17:44:12 PDT
Created attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)
Comment 6 Darin Adler 2021-04-17 18:04:39 PDT
Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=426299&action=review

> Source/WebKitLegacy/mac/WebView/WebView.mm:9652
> +    auto textToTranslate = adoptNS([[NSAttributedString alloc] initWithString:text]);

We don’t need this local variable. Could just merge this line with the next.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9655
> +        [translationViewController setPreferredContentSize:NSMakeSize(400, 400)];

Hardcoded magic size without comment is not great.

> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
> +    if (aim == highlight)

Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.
Comment 7 Darin Adler 2021-04-17 20:01:34 PDT
Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=426299&action=review

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
>> +    if (aim == highlight)
> 
> Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.

Over some threshold. Not sure "1 pixel" is quite right. I don’t really understand the preferred edge heuristic, which is probably why I can’t make a good suggestion.
Comment 8 Wenson Hsieh 2021-04-17 20:26:28 PDT
Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=426299&action=review

Thanks for the review!

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9652
>> +    auto textToTranslate = adoptNS([[NSAttributedString alloc] initWithString:text]);
> 
> We don’t need this local variable. Could just merge this line with the next.

Fixed!

>> Source/WebKitLegacy/mac/WebView/WebView.mm:9655
>> +        [translationViewController setPreferredContentSize:NSMakeSize(400, 400)];
> 
> Hardcoded magic size without comment is not great.

Oh, this reminded me that on recent builds of macOS, this fallback size should no longer be necessary. I removed these two lines.

>>> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
>>> +    if (aim == highlight)
>> 
>> Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.
> 
> Over some threshold. Not sure "1 pixel" is quite right. I don’t really understand the preferred edge heuristic, which is probably why I can’t make a good suggestion.

So I actually took this part directly from the Reveal framework's logic for setting up and presenting `LTUITranslationViewController`. I believe the intent here is to consult the `userInterfaceLayoutDirection` as a sort of tiebreaker in the case where the menu location is exactly in the middle of the bounding rect of the selection, but in practice, it's pretty unlikely that we'll end up in this codepath.

I'll change this to use `areEssentiallyEqual` instead, since that's what we generally use to (sensibly) compare floating point values elsewhere in WebKit.
Comment 9 Darin Adler 2021-04-17 20:34:34 PDT
Comment on attachment 426299 [details]
Fix iOS 14/Big Sur builds (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=426299&action=review

>>>> Source/WebKitLegacy/mac/WebView/WebView.mm:9670
>>>> +    if (aim == highlight)
>>> 
>>> Seems wrong to ever compare two floating point values and expect them to be exactly equal. Seems to me this should be some other kind of check, perhaps a check that the absolute value is one pixel or more.
>> 
>> Over some threshold. Not sure "1 pixel" is quite right. I don’t really understand the preferred edge heuristic, which is probably why I can’t make a good suggestion.
> 
> So I actually took this part directly from the Reveal framework's logic for setting up and presenting `LTUITranslationViewController`. I believe the intent here is to consult the `userInterfaceLayoutDirection` as a sort of tiebreaker in the case where the menu location is exactly in the middle of the bounding rect of the selection, but in practice, it's pretty unlikely that we'll end up in this codepath.
> 
> I'll change this to use `areEssentiallyEqual` instead, since that's what we generally use to (sensibly) compare floating point values elsewhere in WebKit.

That’s OK, and better but it’s still not logical, even though the Reveal framework does it. Obviously we want to share the same logic as the rest of the OS, so it’s not important to do better and would be best to match as well as we can, ideally sharing the code.

A tie breaker used when a person happens to click super-precisely seems obviously wrong, not logical. The saving grace, I suppose, is that as you say it’s so unlikely that it’s nearly irrelevant. This is essentially dead code.
Comment 10 Wenson Hsieh 2021-04-17 20:51:57 PDT
Created attachment 426365 [details]
Patch for landing
Comment 11 EWS 2021-04-17 21:25:07 PDT
Committed r276220 (236702@main): <https://commits.webkit.org/236702@main>

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