Replace ui::DomKey implicit constructor with C++17 inline constexpr [chromium/src : main]
Satoru Takabayashi (Gerrit)
Satoru Takabayashi has uploaded the change for review![]( "Open in Gerrit")
Commit message
Replace ui::DomKey implicit constructor with C++17 inline constexpr
- Removed DomKey::Key enum in favor of static const DomKey objects.
- Added ui::DomKey::FromInteger() helper for the call sites previously
relied on static_cast<ui::DomKey>().
Change diff
Change information
- M chrome/browser/devtools/devtools_window.cc
- M chrome/browser/ui/views/autofill/popup/popup_view_views_unittest.cc
- M components/input/native_web_keyboard_event_aura.cc
- M components/ui_devtools/views/devtools_event_util.cc
- M third_party/blink/renderer/core/events/keyboard_event.cc
- M third_party/blink/renderer/core/input/keyboard_event_manager.cc
- M ui/events/keycodes/dom/dom_key.h
- M ui/events/keycodes/dom/keycode_converter_unittest.cc
- M ui/events/keycodes/dom_us_layout_data.h
- M ui/events/keycodes/keyboard_code_conversion_unittest.cc
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi added 1 comment![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 6 comments![]( "Open in Gerrit")
LGTM with comments.
/*dom_key=*/ui::DomKey::FromBaseOrNone(2099727),In this case, we want valid domkey always, so `ui::DomKey::FromBase(2099727).value()`, to crash if unexpected.
We can do the same in most of tests.
ui::DomKey::FromBaseOrNone(event.dom_key)));note: this can slightly change the behavior. We may want log if dom_key is invalid?
Ditto for the same pattern in non-test code.
ui::DomKey::FromBaseOrNone(web_event.dom_key), web_event.TimeStamp(),ditto.
ui::DomKey::FromBaseOrNone(web_event.dom_key))we can just use `ui::DomKey::FromBase` here, IIUC.
It will the check ui::DomKey == std::optional<ui::DomKey>, which returns false if FromBase returns nullopt.
Ditto for below.
constexpr static DomKey FromBaseOrNone(Base value) {so I guess this can be dropped.
Related details
- Satoru Takabayashi
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. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi added 5 comments![]( "Open in Gerrit")
In this case, we want valid domkey always, so `ui::DomKey::FromBase(2099727).value()`, to crash if unexpected.
We can do the same in most of tests.
Agreed. Fixed!
note: this can slightly change the behavior. We may want log if dom_key is invalid?
Ditto for the same pattern in non-test code.
I think changing the existing behavior is a little risky so I chose to introduce FromBaseAsIs() which creates DomKey just like before, but internally it logs the invalid key. Does this sound good to you?
ui::DomKey::FromBaseOrNone(web_event.dom_key), web_event.TimeStamp(),Satoru Takabayashiditto.
Used FromBaseAsIs() here too.
we can just use `ui::DomKey::FromBase` here, IIUC.
It will the check ui::DomKey == std::optional<ui::DomKey>, which returns false if FromBase returns nullopt.
Ditto for below.
Did you mean ui::DomKey::FromBase(...) would work here? I tried and it didn't build:
../../components/input/web_input_event_builders_android_unittest.cc:85:16: error: no viable conversion from 'std::optional<DomKey>' to 'DomKey'
85 | ui::DomKey::FromBase(web_event.dom_key));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I went with i::DomKey::FromBase(...).value()
so I guess this can be dropped.
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 4 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
LGTM
ui::DomKey::FromBaseOrNone(web_event.dom_key))Satoru Takabayashiwe can just use `ui::DomKey::FromBase` here, IIUC.
It will the check ui::DomKey == std::optional<ui::DomKey>, which returns false if FromBase returns nullopt.
Ditto for below.
Did you mean ui::DomKey::FromBase(...) would work here? I tried and it didn't build:
../../components/input/web_input_event_builders_android_unittest.cc:85:16: error: no viable conversion from 'std::optional<DomKey>' to 'DomKey'
85 | ui::DomKey::FromBase(web_event.dom_key));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I went with i::DomKey::FromBase(...).value()
Marked as unresolved. Sorry for miscommunication. I meant:
```
// In anonymous namespace.
std::ostream& operator<<(
std::ostream& os, const std::optional<ui::DomKey>& dom_key) {
if (!dom_key) {
return os << "nullopt";
}
return os << ui::KeycodeConverter::DomKeyToKeyString(dom_key.value());
}
```
then
```
EXPECT_EQ(ui::DomKey::FromCharacter(entry.character),
ui::DomKey::FromBase(web_event.dom_key))
<< ui::DomKey::FromBase(web_event.dom_key);
```
if it works.
// an error if the value is invalid. Used to preserve existing behaviors.I think this is fine to keep the current behavior (regradless whether it's intentional or not) in this refactoring, but at the same time nice if we can remove this eventually.
Maybe nice to have a TODO with a lower priority bug to audit whether we actually need this or not, and remove this if possible?
static std::optional<DomKey> FromBase(Base value) {clarification: can't we keep this constexpr...?
Related details
- Satoru Takabayashi
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. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi added 3 comments![]( "Open in Gerrit")
ui::DomKey::FromBaseOrNone(web_event.dom_key))Satoru Takabayashiwe can just use `ui::DomKey::FromBase` here, IIUC.
It will the check ui::DomKey == std::optional<ui::DomKey>, which returns false if FromBase returns nullopt.
Ditto for below.
Hidehiko AbeDid you mean ui::DomKey::FromBase(...) would work here? I tried and it didn't build:
../../components/input/web_input_event_builders_android_unittest.cc:85:16: error: no viable conversion from 'std::optional<DomKey>' to 'DomKey'
85 | ui::DomKey::FromBase(web_event.dom_key));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I went with i::DomKey::FromBase(...).value()
Marked as unresolved. Sorry for miscommunication. I meant:
```
// In anonymous namespace.
std::ostream& operator<<(
std::ostream& os, const std::optional<ui::DomKey>& dom_key) {
if (!dom_key) {
return os << "nullopt";
}
return os << ui::KeycodeConverter::DomKeyToKeyString(dom_key.value());
}
```
then
```
EXPECT_EQ(ui::DomKey::FromCharacter(entry.character),
ui::DomKey::FromBase(web_event.dom_key))
<< ui::DomKey::FromBase(web_event.dom_key);
```if it works.
Done! It looks like the operator<<() needed to be in "ui" namespace due to ADL reasons.
// an error if the value is invalid. Used to preserve existing behaviors.I think this is fine to keep the current behavior (regradless whether it's intentional or not) in this refactoring, but at the same time nice if we can remove this eventually.
Maybe nice to have a TODO with a lower priority bug to audit whether we actually need this or not, and remove this if possible?
Done. Added a TODO reusing the same bug Id.
clarification: can't we keep this constexpr...?
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
2 comments
namespace ui {nit: nice to wrap the anonymous namespace too, to avoid conflict.
```
namespace ui {
namespace {
std::ostream& ...
} // namespace
} // namespace ui
```
Related details
- Satoru Takabayashi
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. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi added 1 comment![]( "Open in Gerrit")
namespace ui {nit: nice to wrap the anonymous namespace too, to avoid conflict.
```
namespace ui {
namespace {
std::ostream& ...
} // namespace
} // namespace ui
```
Didn't compile due to ADL reasons so let's keep it as is. Since it's defined in unitstest.cc I think there is no need to worry about conflicts.
```
../../components/input/web_input_event_builders_android_unittest.cc:33:15: error: unused function 'operator<<' [-Werror,-Wunused-function]
33 | std::ostream& operator<<(std::ostream& os, const ui::DomKey& dom_key) {
| ^~~~~~~~
1 error generated.
```
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keren Zhu (Gerrit)
Keren Zhu voted Code-Review+1![]( "Open in Gerrit")
- Satoru Takabayashi
- Stephen Nusko
- Takashi Toyoshima
- Wez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi voted Commit-Queue+1![]( "Open in Gerrit")
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
2 comments
LGTM for just `//components/input` I didn't look anywhere else besides dom_key.h
LOG(ERROR) << "Invalid DomKey: " << value;I assume this is for testing purposes? There doesn't seem to be a reason to submit with this debugging logging in is there I don't think?
Or should this be a if we want this to be out in the field?
`DUMP_WITHOUT_CRASHING` ?
Or `CHECK(IsValidValue(value), NotFatalUntil::M1##)`?
Related details
- Satoru Takabayashi
- Takashi Toyoshima
- Wez
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Satoru Takabayashi
- Wez
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. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi added 2 comments![]( "Open in Gerrit")
LOG(ERROR) << "Invalid DomKey: " << value;I assume this is for testing purposes? There doesn't seem to be a reason to submit with this debugging logging in is there I don't think?
Or should this be a if we want this to be out in the field?
`DUMP_WITHOUT_CRASHING` ?
Or `CHECK(IsValidValue(value), NotFatalUntil::M1##)`?
Thank you for the input! I added this logging as it could be helpful for debugging DOM key-related issues.
Using DumpWithoutCrashing could be an alternative if we want to remove the function as mentioned in the TODO comment. However, on second thought, I think it is not worth the effort. It would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous. I can think of two options:
1) Remove the TODO comment but keep the logging.
2) Remove the TODO comment and the logging.
hidehiko@, what do you think?
LOG(ERROR) << "Invalid DomKey: " << value;I assume this is for testing purposes? There doesn't seem to be a reason to submit with this debugging logging in is there I don't think?
Or should this be a if we want this to be out in the field?
`DUMP_WITHOUT_CRASHING` ?
Or `CHECK(IsValidValue(value), NotFatalUntil::M1##)`?
- Stephen Nusko
- Wez
Hidehiko Abe (Gerrit)
Hidehiko Abe added 1 comment![]( "Open in Gerrit")
LOG(ERROR) << "Invalid DomKey: " << value;Satoru TakabayashiI assume this is for testing purposes? There doesn't seem to be a reason to submit with this debugging logging in is there I don't think?
Or should this be a if we want this to be out in the field?
`DUMP_WITHOUT_CRASHING` ?
Or `CHECK(IsValidValue(value), NotFatalUntil::M1##)`?
Thank you for the input! I added this logging as it could be helpful for debugging DOM key-related issues.
Using DumpWithoutCrashing could be an alternative if we want to remove the function as mentioned in the TODO comment. However, on second thought, I think it is not worth the effort. It would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous. I can think of two options:
1) Remove the TODO comment but keep the logging.
2) Remove the TODO comment and the logging.hidehiko@, what do you think?
It would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous.
Actually, can we run CHECK() after confirming no fatal crash reports?
So that, in the future, if we accidentally introduce something broken in caller side, we can catch.
It can be done later with lower priority, though.
Related details
- Satoru Takabayashi
- Stephen Nusko
- Wez
Wez (Gerrit)
Wez added 6 comments![]( "Open in Gerrit")
namespace ui {Satoru Takabayashinit: nice to wrap the anonymous namespace too, to avoid conflict.
```
namespace ui {
namespace {
std::ostream& ...
} // namespace
} // namespace ui
```
Didn't compile due to ADL reasons so let's keep it as is. Since it's defined in unitstest.cc I think there is no need to worry about conflicts.
```
../../components/input/web_input_event_builders_android_unittest.cc:33:15: error: unused function 'operator<<' [-Werror,-Wunused-function]
33 | std::ostream& operator<<(std::ostream& os, const ui::DomKey& dom_key) {
| ^~~~~~~~
1 error generated.
```
gTest supports using either `operator<<` if available, or an `AbslStringify()` overload, or a `PrintTo()` method on the type itself.
Ideally I think we'd go with a version that sits on the type itself (which basically means we have to go with `PrintTo()`). If we can't do that (given that the string names live in `ui::KeycodeConverter` rather than alongside `ui::DomKey` itself) then I'd suggest perhaps defining the operator in the same header as the `KeycodeConverter` - that way the namespace pollution is at least part of the "correct" bit of code.
// that raw integers cannot be implicitly converted to DomKey.Doesn't `explicit` prevent implicit conversions?
static DomKey FromBaseAsIs(Base value) {"as-is" is something of a colloquialism; "unchecked" would be clearer here, for example.
static DomKey FromBaseAsIs(Base value) {Why do we need this at all? It's not a performance optimization (since we use `IsValidValue()` in both calls), so presumably we just have call-sites for which we don't know how to tolerate failure?
It seems that we have replaced a number of _explicit_ static-case call-sites with this - can we instead leave the existing from-`Base` constructor in-place, but marked `explicit`, so that we (1) don't need to change those call-sites and (2) it is clear what their semantics are?
Whether or not we need "unchecked" behaviour long term seems to hinge on whether the `WebKeyboardEvent`s synthesized by content ever need to pass-through these types - if they do then I'd guess that we need to support invalid values simply because it's valid for content to synthesize events with whatever contents it likes.
But then the `IsValidValue()` API is only validating that the value is explicitly tagged as a Unicode, non-Unicode, or dead-key value - we're still not validating beyond that that the value makese sense. We could coerce all "invalid" values to `DomKey::NONE` in `FromBase()` and have it return a plain `DomKey`, or provide a `FromBaseOrNone()` that does that if we prefer to keep it separate.
// |dom_key_data.inc| describes the non-printable DomKey values, and is
// included here to create constants for them in the DomKey:: scope.nit: This comment needs updating.
static const DomKey NONE;nit: `NONE` is essentially a not-set or not-valid value, so providing APIs that use `std::optional<DomKey>` is a little odd, and ends up wasting space - see my comment, below, on `FromBaseAsIs()`!
Related details
- Satoru Takabayashi
- Stephen Nusko
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
namespace ui {Satoru Takabayashinit: nice to wrap the anonymous namespace too, to avoid conflict.
```
namespace ui {
namespace {
std::ostream& ...
} // namespace
} // namespace ui
```
WezDidn't compile due to ADL reasons so let's keep it as is. Since it's defined in unitstest.cc I think there is no need to worry about conflicts.
```
../../components/input/web_input_event_builders_android_unittest.cc:33:15: error: unused function 'operator<<' [-Werror,-Wunused-function]
33 | std::ostream& operator<<(std::ostream& os, const ui::DomKey& dom_key) {
| ^~~~~~~~
1 error generated.
```
gTest supports using either `operator<<` if available, or an `AbslStringify()` overload, or a `PrintTo()` method on the type itself.
Ideally I think we'd go with a version that sits on the type itself (which basically means we have to go with `PrintTo()`). If we can't do that (given that the string names live in `ui::KeycodeConverter` rather than alongside `ui::DomKey` itself) then I'd suggest perhaps defining the operator in the same header as the `KeycodeConverter` - that way the namespace pollution is at least part of the "correct" bit of code.
+1 I like that better.
LOG(ERROR) << "Invalid DomKey: " << value;Satoru TakabayashiI assume this is for testing purposes? There doesn't seem to be a reason to submit with this debugging logging in is there I don't think?
Or should this be a if we want this to be out in the field?
`DUMP_WITHOUT_CRASHING` ?
Or `CHECK(IsValidValue(value), NotFatalUntil::M1##)`?
Hidehiko AbeThank you for the input! I added this logging as it could be helpful for debugging DOM key-related issues.
Using DumpWithoutCrashing could be an alternative if we want to remove the function as mentioned in the TODO comment. However, on second thought, I think it is not worth the effort. It would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous. I can think of two options:
1) Remove the TODO comment but keep the logging.
2) Remove the TODO comment and the logging.hidehiko@, what do you think?
It would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous.
Actually, can we run CHECK() after confirming no fatal crash reports?
So that, in the future, if we accidentally introduce something broken in caller side, we can catch.It can be done later with lower priority, though.
Nothing collects debug logs, are you imagining that someone would make a bug report with the log output or that developers would see this? If that is the case I would recommend a DCHECK, but generally sounds like we should try to move towards a CHECK.
I recommend
```
CHECK(value != 0 && !IsValidValue(value), base::NotFatalUntil::M150);
```
M150 means you have a full stable release (M149) to get crashes reported to you. And then if nothing happens it just becomes a CHECK, and if something does happen you'll get crash bugs and worst case just need to cherrypick a removal in.
Related details
- Satoru Takabayashi
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Hidehiko Abe
- Keren Zhu
- Satoru Takabayashi
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. |
Keren Zhu (Gerrit)
Keren Zhu voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hidehiko Abe
- Satoru Takabayashi
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. |
Satoru Takabayashi (Gerrit)
Satoru Takabayashi added 7 comments![]( "Open in Gerrit")
Satoru Takabayashinit: nice to wrap the anonymous namespace too, to avoid conflict.
```
namespace ui {
namespace {
std::ostream& ...
} // namespace
} // namespace ui
```
WezDidn't compile due to ADL reasons so let's keep it as is. Since it's defined in unitstest.cc I think there is no need to worry about conflicts.
```
../../components/input/web_input_event_builders_android_unittest.cc:33:15: error: unused function 'operator<<' [-Werror,-Wunused-function]
33 | std::ostream& operator<<(std::ostream& os, const ui::DomKey& dom_key) {
| ^~~~~~~~
1 error generated.
```
Stephen NuskogTest supports using either `operator<<` if available, or an `AbslStringify()` overload, or a `PrintTo()` method on the type itself.
Ideally I think we'd go with a version that sits on the type itself (which basically means we have to go with `PrintTo()`). If we can't do that (given that the string names live in `ui::KeycodeConverter` rather than alongside `ui::DomKey` itself) then I'd suggest perhaps defining the operator in the same header as the `KeycodeConverter` - that way the namespace pollution is at least part of the "correct" bit of code.
+1 I like that better.
Good idea! Moved it to ui/events/keycodes/dom/keycode_converter.h.
Doesn't `explicit` prevent implicit conversions?
Thanks. Updated the comment accordingly.
Satoru TakabayashiI assume this is for testing purposes? There doesn't seem to be a reason to submit with this debugging logging in is there I don't think?
Or should this be a if we want this to be out in the field?
`DUMP_WITHOUT_CRASHING` ?
Or `CHECK(IsValidValue(value), NotFatalUntil::M1##)`?
Hidehiko AbeThank you for the input! I added this logging as it could be helpful for debugging DOM key-related issues.
Using DumpWithoutCrashing could be an alternative if we want to remove the function as mentioned in the TODO comment. However, on second thought, I think it is not worth the effort. It would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous. I can think of two options:
1) Remove the TODO comment but keep the logging.
2) Remove the TODO comment and the logging.hidehiko@, what do you think?
Stephen NuskoIt would require confirming the absence of crash reports and updating existing production call sites to use FromBase(x).value(), which is potentially dangerous.
Actually, can we run CHECK() after confirming no fatal crash reports?
So that, in the future, if we accidentally introduce something broken in caller side, we can catch.It can be done later with lower priority, though.
Nothing collects debug logs, are you imagining that someone would make a bug report with the log output or that developers would see this? If that is the case I would recommend a DCHECK, but generally sounds like we should try to move towards a CHECK.
I recommend
```
CHECK(value != 0 && !IsValidValue(value), base::NotFatalUntil::M150);
```M150 means you have a full stable release (M149) to get crashes reported to you. And then if nothing happens it just becomes a CHECK, and if something does happen you'll get crash bugs and worst case just need to cherrypick a removal in.
As mentioned earlier, I just got rid of this function.
Why do we need this at all? It's not a performance optimization (since we use `IsValidValue()` in both calls), so presumably we just have call-sites for which we don't know how to tolerate failure?
It seems that we have replaced a number of _explicit_ static-case call-sites with this - can we instead leave the existing from-`Base` constructor in-place, but marked `explicit`, so that we (1) don't need to change those call-sites and (2) it is clear what their semantics are?
Whether or not we need "unchecked" behaviour long term seems to hinge on whether the `WebKeyboardEvent`s synthesized by content ever need to pass-through these types - if they do then I'd guess that we need to support invalid values simply because it's valid for content to synthesize events with whatever contents it likes.
But then the `IsValidValue()` API is only validating that the value is explicitly tagged as a Unicode, non-Unicode, or dead-key value - we're still not validating beyond that that the value makese sense. We could coerce all "invalid" values to `DomKey::NONE` in `FromBase()` and have it return a plain `DomKey`, or provide a `FromBaseOrNone()` that does that if we prefer to keep it separate.
Thank you for the great idea! I just deleted this function and made the from-Base constructor public. Hope I understand your suggestion correctly.
"as-is" is something of a colloquialism; "unchecked" would be clearer here, for example.
Acknowledged. The function is gone now.
// |dom_key_data.inc| describes the non-printable DomKey values, and is
// included here to create constants for them in the DomKey:: scope.nit: This comment needs updating.
Done
static const DomKey NONE;nit: `NONE` is essentially a not-set or not-valid value, so providing APIs that use `std::optional<DomKey>` is a little odd, and ends up wasting space - see my comment, below, on `FromBaseAsIs()`!
Good point. It looks like `std::optional<DomKey> FromBase()` is used in only one place:
```
std::optional<ui::DomKey> dom_key =
ui::DomKey::FromBase(key_data->dom_key);
if (!dom_key)
return false;
```
We could change the code to:
```
const ui::DomKey dom_key(key_data->dom_key);
if (dom_key != ui::DomKey::NONE && !dom_key.IsValid())
return false;
```
Then we can get rid of FromBase(). What do you think?
Related details
- Hidehiko Abe
- Keren Zhu
- Stephen Nusko
- Takashi Toyoshima
- Wez
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Hidehiko Abe
- Keren Zhu
- Satoru Takabayashi
- Stephen Nusko
- Wez
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. |
Keren Zhu (Gerrit)
Keren Zhu voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hidehiko Abe
- Satoru Takabayashi
- Stephen Nusko
- Wez
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. |