Re-autogen input_method_private_interface.js in Closure compiler. [chromium/src : main]
Bao-Duy Tran (Gerrit)
Bao-Duy Tran voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Essential Inputs Reviews
- Oliver Dunk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
jopa...@chromium.org is from context(chrome/inputs/essential-inputs.gwsq)
Related details
- John Palmer
- Oliver Dunk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oliver Dunk (Gerrit)
Oliver Dunk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Oliver Dunk
| Code-Review | +1 |
1 comment
Thanks! There was a precheck that ran on the other CL and called out certain files that needed updating, but it didn't catch this one. I wonder if this file needs to be listed somewhere?
Related details
- Bao-Duy Tran
- John Palmer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John Palmer (Gerrit)
John Palmer added 1 comment![]( "Open in Gerrit")
I don't believe this API supports promises (maybe it does in theory, but we don't use them in practice), so the use of resolve/reject for this API is probably confusing
Related details
- Bao-Duy Tran
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 Palmer (Gerrit)
John Palmer added 1 comment![]( "Open in Gerrit")
* failure, $(ref:runtime.lastError) is set.Losing this feels like a bad change?
John Palmer (Gerrit)
John Palmer added 1 comment![]( "Open in Gerrit")
I don't believe this API supports promises (maybe it does in theory, but we don't use them in practice), so the use of resolve/reject for this API is probably confusing
I am also unsure about the input related changes in https://chromium-review.googlesource.com/c/chromium/src/+/7300086
Bao-Duy Tran (Gerrit)
Bao-Duy Tran added 2 comments![]( "Open in Gerrit")
John PalmerI don't believe this API supports promises (maybe it does in theory, but we don't use them in practice), so the use of resolve/reject for this API is probably confusing
I am also unsure about the input related changes in https://chromium-review.googlesource.com/c/chromium/src/+/7300086
I believe this API *does* support Promise nowadays.
It's a "recent" thing, since https://crrev.com/c/4172683 in particular, and Promise should be the primary thing, appearing as the only supported thing in the JSON API specs, with old callback still functional for backward-compat in the meantime.
It just so happens that the main client of this API (internal-repo CrOS 1P VK+IME extension) hasn't migrated to Promise usage yet, and still relies on old callback ways with bespoke async wrappers. Some Promise-based usage of the API can be seen @ https://crrev.com/c/7197159 by a11y component.
(btw I think this question is technically tied to https://crrev.com/c/7300086 where the comments were changed, not this CL that just re-autogens dependent stuff accordingly)
* failure, $(ref:runtime.lastError) is set.Losing this feels like a bad change?
I guess this is technically a comment for https://crrev.com/c/4172683 where the API was switched from callback to Promise, not this CL that just re-autogens dependent stuff based on https://crrev.com/c/7300086 that adapted comments to the said switch to Promise.
Related details
- John Palmer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Bao-Duy Tran added 1 comment![]( "Open in Gerrit")
Thanks! There was a precheck that ran on the other CL and called out certain files that needed updating, but it didn't catch this one. I wonder if this file needs to be listed somewhere?
That sounds similar to manual IfChange-ThenChange, but I'm not quite familiar yet with how that works with autogen files. If you could point me to the "precheck" that was triggered on https://crrev.com/c/4172683, maybe I can figure out and give it a go for this autogen dependency as well.
btw, besides third_party/closure_compiler/interfaces/input_method_private_interface.js being re-autogen'd here, there's also https://crsrc.org/c/chrome/browser/resources/chromeos/accessibility/definitions/input_method_private.d.ts that'd need re-autogen as well; just that the latter isn't affected by comment changes in https://crrev.com/c/4172683.
There's also https://crsrc.org/c/tools/typescript/definitions/input_method_private.d.ts that should theoretically be an autogen based on the same API specs, but manually crafted. There's also yet another manually-crafted .d.ts in the internal repo.
Oliver Dunk (Gerrit)
Oliver Dunk added 2 comments![]( "Open in Gerrit")
Bao-Duy TranThanks! There was a precheck that ran on the other CL and called out certain files that needed updating, but it didn't catch this one. I wonder if this file needs to be listed somewhere?
That sounds similar to manual IfChange-ThenChange, but I'm not quite familiar yet with how that works with autogen files. If you could point me to the "precheck" that was triggered on https://crrev.com/c/4172683, maybe I can figure out and give it a go for this autogen dependency as well.
btw, besides third_party/closure_compiler/interfaces/input_method_private_interface.js being re-autogen'd here, there's also https://crsrc.org/c/chrome/browser/resources/chromeos/accessibility/definitions/input_method_private.d.ts that'd need re-autogen as well; just that the latter isn't affected by comment changes in https://crrev.com/c/4172683.
There's also https://crsrc.org/c/tools/typescript/definitions/input_method_private.d.ts that should theoretically be an autogen based on the same API specs, but manually crafted. There's also yet another manually-crafted .d.ts in the internal repo.
Adding @tjud...@chromium.org. Tim, do you have any thoughts here? It looks like it is this check [1] but I haven't been able to figure out why it didn't run for this file. Perhaps I bypassed it, but I don't think I did.
[1] https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/extensions/api/PRESUBMIT.py
* failure, $(ref:runtime.lastError) is set.Bao-Duy TranLosing this feels like a bad change?
I guess this is technically a comment for https://crrev.com/c/4172683 where the API was switched from callback to Promise, not this CL that just re-autogens dependent stuff based on https://crrev.com/c/7300086 that adapted comments to the said switch to Promise.
Yeah, this is tricky. The JSON schema format used to define this API is old - we're moving towards WebIDL and that format focuses on promises with callback signatures generated only as a backwards compatibility feature. Given that, we're slowly removing mentions of `runtime.lastError` across the schemas. In this case I do think we could've kept it. I mostly avoided touching private APIs but I think Gemini did this one and I figured it wasn't a huge deal to keep. Happy to revert the changes if you prefer.
Related details
- Bao-Duy Tran
- John Palmer
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 Palmer (Gerrit)
John Palmer voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Bao-Duy Tran
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. |