Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for user on exit strategy. #4757

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aee-google
Copy link
Contributor

b/385357645

Copy link
Contributor

@yell0wd0g yell0wd0g left a comment

Choose a reason for hiding this comment

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

Drive-by comments (apologies if the CL wasn't ready to review!).

Can we add unittests for either or both of the classes?

configuration::Configuration::GetInstance()->CobaltUserOnExitStrategy());
if (exit_strategy_str == "stop") {
return static_cast<UserOnExitStrategy>(kUserOnExitStrategyClose);
} else if (exit_strategy_str == "suspend") {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for "else" after return inside the if branch, here and throughout.

I couldn't find a recent source for that style guide entry, only [an old one](https://chromium.googlesource.com/chromium/src.git/+/62.0.3178.1/styleguide/c++/c++.md#code-formatting but that pattern sounds very familiar to me.

Configuration::Configuration() {
configuration_api_ = static_cast<const CobaltExtensionConfigurationApi*>(
SbSystemGetExtension(kCobaltExtensionConfigurationName));
if (configuration_api_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Try and reduce the if-nesting, e.g.

  if (!configuration_api_) {
    return;
  }
  // Verify...

SbSystemGetExtension(kCobaltExtensionConfigurationName));
if (configuration_api_) {
// Verify it's the extension needed.
if (strcmp(configuration_api_->name, kCobaltExtensionConfigurationName) !=
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we use C90 string compare but this is a C++ file, so I'd suggest using like in the other file a if (std::string(configuration_api_->name) == kCobaltExtensionConfigurationName) ....

(std::string::operator== allows direct comparison with C-style strings.)

// instance as was previously initialized.
static Configuration* GetInstance();

const char* CobaltUserOnExitStrategy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return a constexpr std::string ? char* can introduce vulnerabilities and bugs.

]
deps = [
"//base",
"//starboard:starboard_headers_only",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably fully depend on //starboard:starboard_group

@@ -107,6 +108,8 @@ void CobaltContentRendererClient::ExposeInterfacesToBrowser(
void CobaltContentRendererClient::RenderFrameCreated(
content::RenderFrame* render_frame) {
new js_injection::JsCommunication(render_frame);
CobaltRenderFrameObserver* render_frame_observer =
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this get garbage collected?

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.

3 participants