Sub ASCII digits for Arabic-Indic for Arabic keyboards on Windows [chromium/src : main]
John An (Gerrit)
John An added 3 comments![]( "Open in Gerrit")
#if BUILDFLAG(IS_WIN)John AnNit: wouldn't it be easier to follow if the variable is set unconditionally false outside the `#if`, then set it again inside?
John AnYes agreed.
Done
0 /* idThread = Current Thread */)))) == LANG_ARABIC &&John AnNot sure if the use of "eastern" (see below) calls for a specific sub-language. I see a bunch of `SUBLANG_ARABIC_*` too.
Sublanguages didn't end up being relevant here, but your comment made me revise to scoping this behavior to Arabic 101. There are 3 Arabic keyboard layouts and the layouts/input are mostly independent of sublanguage. The other 2 layouts - 102 and 102 AZERTY - already have altgr characters in the top row digit keys. Thanks!
kEasternArabicZeroUnicode)John AnWondering why `Eastern`? It seems U+0660 is simply described as "Arabic-Indic Digit Zero".
John AnLooking into this made me realize there are distinct Arabic-Indic 066x and Extended Arabic-Indic 06Fx. I thought Arabic-Indic and Eastern-Arabic were interchangeable terms. I'll look into what language should support which digits and make some changes. Thanks
Based on unicode docs there are 2 Eastern Arabic digit sets where Arabic-Indic is used in Arabic languages and Eastern Arabic-Indic are used with Arabic-script languages of Iran, Pakistan, and India eg. Persian, Urdu. I've revised a bunch of comments and naming to reflect this.
Related details
- Mustaq Ahmed
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mustaq Ahmed
| Code-Review | +1 |
1 comment
Thanks for looking into my language questions. Still LGTM (as a non-owner).
Related details
- John An
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
Hello! I'd appreciate a review of this CL when you can. If you feel there is someone better to review, I will try and find different folks.
Related details
- Jonathan Ross
- Kartar Singh
- Nasko Oskov
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
const char kArabic101KeyboardLayoutName[] = "00000401";```suggestion
const std::wstring_view kArabic101KeyboardLayoutName = "00000401";
```
base::EqualsASCII(base::WideToUTF16(std::wstring_view(kl_name)),
kArabic101KeyboardLayoutName) &&This is written somewhat awkwardly. My suggestion (in combination with the above change):
```suggestion
kl_name == kArabic101KeyboardLayoutName &&
```
Which requires no string conversions at all.
(In general, it's better to "convert" WCHAR/wchar_t on Windows using `AsStringPiece16()` or similar; however, we don't need to do that at all here)
? static_cast<char16_t>(event.GetCharacter() - u'0' +Is this cast needed?
Related details
- John An
- Jonathan Ross
- Nasko Oskov
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 5 comments![]( "Open in Gerrit")
Adding dtapuska@ for an extra pair of eyes, as he knows the input code.
GetKeyboardLayoutName(kl_name) &&nit: Prefix Win32 API calls with `::`.
GetKeyboardLayoutName(kl_name) &&Does the keyboard name change often? If not, can we cache the returned value and maybe observe changes instead of making a function call on each character insertion?
KEYBOARD_LAYOUT_ARABIC,Can we keep the list alphabetically sorted? I think it is fine if it is sorted just within the IS_WIN define, so it doesn't become harder to read with multiple #if statements.
case KEYBOARD_LAYOUT_ARABIC:Let's keep them alphabetically ordered.
Related details
- Dave Tapuska
- John An
- Jonathan Ross
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
base::FeatureList::IsEnabled(features::kArabicDigitSubstitution) &&We should probably cache this as a member variable of the class. There is overhead to feature lookup, so we try to not check it more than once per VSync.
If the keyboard changes can be observed, as per nasko@ recommendation. Then we could just cache the entire `should_substitute_digit`
Related details
- Dave Tapuska
- John An
- Rahul Arakeri
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
base::FeatureList::IsEnabled(features::kArabicDigitSubstitution) &&We should probably cache this as a member variable of the class. There is overhead to feature lookup, so we try to not check it more than once per VSync.
If the keyboard changes can be observed, as per nasko@ recommendation. Then we could just cache the entire `should_substitute_digit`
`base::Feature` caches the lookup result internally. This has been the case for some time now. Do we really need another layer of caching?
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
base::FeatureList::IsEnabled(features::kArabicDigitSubstitution) &&Daniel ChengWe should probably cache this as a member variable of the class. There is overhead to feature lookup, so we try to not check it more than once per VSync.
If the keyboard changes can be observed, as per nasko@ recommendation. Then we could just cache the entire `should_substitute_digit`
`base::Feature` caches the lookup result internally. This has been the case for some time now. Do we really need another layer of caching?
The internal cache certainly helps, but there's still some overhead to ` base::FeatureList::IsEnabled`. We've seen it in before in stack sampled profiling when a given feature was checked multiple times within a single frame production.
John An (Gerrit)
John An added 9 comments![]( "Open in Gerrit")
Thanks for the comments all! I've uploaded a patchset to address them
const char kArabic101KeyboardLayoutName[] = "00000401";```suggestion
const std::wstring_view kArabic101KeyboardLayoutName = "00000401";
```
Acknowledged
base::FeatureList::IsEnabled(features::kArabicDigitSubstitution) &&Daniel ChengWe should probably cache this as a member variable of the class. There is overhead to feature lookup, so we try to not check it more than once per VSync.
If the keyboard changes can be observed, as per nasko@ recommendation. Then we could just cache the entire `should_substitute_digit`
Jonathan Ross`base::Feature` caches the lookup result internally. This has been the case for some time now. Do we really need another layer of caching?
The internal cache certainly helps, but there's still some overhead to ` base::FeatureList::IsEnabled`. We've seen it in before in stack sampled profiling when a given feature was checked multiple times within a single frame production.
Ok. I decided to cache the state in a static struct. Keyboard layout is per-thread so it doesn't make sense for every RWHVA instance to have a member.
nit: Prefix Win32 API calls with `::`.
Acknowledged
Does the keyboard name change often? If not, can we cache the returned value and maybe observe changes instead of making a function call on each character insertion?
yes this would be better
base::EqualsASCII(base::WideToUTF16(std::wstring_view(kl_name)),
kArabic101KeyboardLayoutName) &&This is written somewhat awkwardly. My suggestion (in combination with the above change):
```suggestion
kl_name == kArabic101KeyboardLayoutName &&
```Which requires no string conversions at all.
(In general, it's better to "convert" WCHAR/wchar_t on Windows using `AsStringPiece16()` or similar; however, we don't need to do that at all here)
This does look cleaner. Still learning the cleanest ways to do string comparison in Chromium. Thanks!
? static_cast<char16_t>(event.GetCharacter() - u'0' +John AnIs this cast needed?
Nope
Can we keep the list alphabetically sorted? I think it is fine if it is sorted just within the IS_WIN define, so it doesn't become harder to read with multiple #if statements.
Acknowledged
Let's keep them alphabetically ordered.
- Daniel Cheng
- Dave Tapuska
- Jonathan Ross
- Mustaq Ahmed
- Nasko Oskov
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
2 comments
Overall, codewise it looks fine to me now. Still happy to defer to dtapuska@ on input matters.
// This state helps relieve unnecessary calls to GetKeyboardLayoutName() andnit: I'd move this comment above the struct, as it applies both to the definition and the declaration.
Related details
- Daniel Cheng
- Dave Tapuska
- John An
- Jonathan Ross
- Rahul Arakeri
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 3 comments![]( "Open in Gerrit")
Would appreciate another look at this CL. I've addressed the comments and it seems close!
Please fix this WARNING reported by ClangTidy: warning: building this file or its dependencies failed; no diagnostics will be i...
warning: building this file or its dependencies failed; no diagnostics will be issued.
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
// This state helps relieve unnecessary calls to GetKeyboardLayoutName() andnit: I'd move this comment above the struct, as it applies both to the definition and the declaration.
- Daniel Cheng
- Dave Tapuska
- Jonathan Ross
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Jonathan Ross (Gerrit)
Jonathan Ross voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jonathan Ross
| Code-Review | +1 |
2 comments
if (!arabic_digit_sub_state.feature_initialized) {Nit: set this to true as we init here
Related details
- Daniel Cheng
- Dave Tapuska
- John An
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
Nit: set this to true as we init here
- Daniel Cheng
- Dave Tapuska
- Jonathan Ross
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
#include "ui/events/keycodes/keyboard_codes_win.h"You need to include this only for Windows
Related details
- Daniel Cheng
- John An
- Jonathan Ross
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
You need to include this only for Windows
- Jonathan Ross
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted Code-Review+1![]( "Open in Gerrit")
- John An
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- John An
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
John An (Gerrit)
John An voted Commit-Queue+2![]( "Open in Gerrit")
- Nasko Oskov
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Sub ASCII digits for Arabic-Indic for Arabic keyboards on Windows
Windows Arabic keyboard layouts do not natively support Arabic-Indic
digit input. This means on Windows there is no way to input
Arabic-Indic digits into a web page text form. Any fix from Windows
will still leave browser instances on older Windows version without
this capability.
The proposed fix is to implement Arabic-Indic digit input in a
faux-AltGr layer. The browser can detect when Ctrl+Alt or right-Alt are
held and input Arabic-Indic digits for top-row digit input.
This is an initial, feature flag gated check-in for unconditional
Arabic-Indic digit substitution for Arabic keyboard layouts. The
implementation works by substituting ASCII digit characters for
Arabic-Indic digit characters in key events that the
RenderWidgetHostViewAura forwards to the renderer process.
The substituted input on Ctrl+Alt and right-Alt hold will be checked in
later. The decision to promote this to a browser setting will also come
later and be based on customer feedback.
- M content/browser/renderer_host/render_widget_host_view_aura.cc
- M content/browser/renderer_host/render_widget_host_view_aura.h
- M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
- M content/common/features.cc
- M content/common/features.h
- M ui/events/test/keyboard_layout.h
- M ui/events/test/keyboard_layout_win.cc
Code-Review: +1 by Jonathan Ross, +1 by Dave Tapuska
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |