Skip to content

Conversation

@aee-google
Copy link
Contributor

Bug: 447659892

@aee-google aee-google requested a review from a team as a code owner December 23, 2025 01:03
@github-actions
Copy link

🤖 Gemini Suggested Commit Message


starboard: Add support for touch input

Integrate Starboard touch input events into Cobalt's UI event
pipeline. This change maps kSbInputEventTypePress,
kSbInputEventTypeUnpress, and kSbInputEventTypeMove to the
corresponding ui::TouchEvent types, enabling applications to
respond to touch screen and touch pad interactions.

Bug: 447659892

💡 Pro Tips for a Better Commit Message:

  1. Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
  2. Re-run the Generator: Post a comment with: /generate-commit-message

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for touch events by creating ui::TouchEvent from Starboard input events. The implementation has a few issues. There's a critical bug in a debug build that can cause a crash due to incorrect ordering of null checks and DCHECKs. Another critical issue is a potential use of an uninitialized variable in release builds if an unknown event type is received. I've also pointed out some areas for improvement, such as removing redundant logging and replacing a magic number with a named constant for better readability. Please address the critical issues and consider the other suggestions.

namespace {

std::unique_ptr<ui::Event> CreateTouchInputEvent(const SbEvent* event) {
DCHECK(event);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general we're replacing DCHECKs with CHECKs (see b/435425692), should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt about making this a CHECK for now instead? We can always change back if it seems like it causes crashes we could avoid otherwise (cc @jellefoks who I think wrote this code originally)

Bug: 447659892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants