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

Use 'cobalt' instead of 'content_shell' for logging and paths. #4292

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jellefoks
Copy link
Member

@jellefoks jellefoks commented Oct 21, 2024

This changes the paths and the default log file to 'cobalt' instead of
'content_shell'.

With this PR, the local storage for the cobalt target on linux will be
~/.config/cobalt

b/375193856
b/374191454

@jellefoks

This comment was marked as resolved.

@jellefoks jellefoks force-pushed the poor_mans_cobalt_separate_storage branch 12 times, most recently from b80d454 to ecd4f1e Compare October 28, 2024 20:14
@jellefoks jellefoks marked this pull request as ready for review October 30, 2024 17:50
@jellefoks jellefoks requested a review from a team as a code owner October 30, 2024 17:50
@jellefoks jellefoks enabled auto-merge (squash) October 30, 2024 17:50
build/build_config.h Outdated Show resolved Hide resolved
@@ -211,6 +211,11 @@ namespace chrome_cleaner {
class ResetShortcutsComponent;
class SystemReportComponent;
} // namespace chrome_cleaner
#if BUILDFLAG(IS_COBALT)
namespace cobalt {
class CobaltPathProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

@niranjanyardi Did you look into a starboard-centric way this should be done? Should we create a follow up bug for you to look at potentially changing this or are you fine with this solution? (either way, I think it should be submitted as is)

Copy link
Member Author

Choose a reason for hiding this comment

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

The needed changes here is what I brought up in the chat, and this was the suggested approach. I'm not aware of a followup. This mirrors occurences of ShellPathProvider in this file, plus the #if to mark our changes to help with rebasing.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of the dependencies and of the chat is that it'd make more sense to me to see USE_STARBOARD here, and define the paths in Starboard.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said in the chat: This is not something needed for Starboard, but for Cobalt. You can currently build cobalt without Starboard, and changing that here would break that unnecessarily

Now sometime in the future that, the paths may likely be provided by Starboard, and then the class can be renamed to StarboardPathProvider and this can be changed. However, that is currently not the case.

cobalt/cobalt_paths.h Outdated Show resolved Hide resolved
cobalt/cobalt_paths.cc Outdated Show resolved Hide resolved
@jellefoks jellefoks force-pushed the poor_mans_cobalt_separate_storage branch from ecd4f1e to c7e054a Compare October 30, 2024 19:06
This changes the paths and the default log file to 'cobalt' instead of
'content_shell'.

With this PR, the local storage for the `cobalt` target on linux will be
`~/.config/cobalt`

b/375193856
b/374191454
Copy link
Contributor

@andrewsavage1 andrewsavage1 left a comment

Choose a reason for hiding this comment

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

This LGTM, but it would be nice to have someone like John or Miguel also take a look as I'm not familiar with some of the initialization code we're overriding

@@ -0,0 +1,27 @@
// Copyright 2021 The Chromium Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% on this, but I assume we'd want a Cobalt copyright on the forked code

// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CONTENT_SHELL_BROWSER_SHELL_PATHS_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we want to rename the header guard here

#include "content/public/common/content_switches.h"
#include "content/shell/common/shell_switches.h"

#if !BUILDFLAG(IS_ANDROID)
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these headers used? I'm confused because I don't see any linux-specifc code added, just android-specific code

// Directory where user data can be written.
SHELL_DIR_USER_DATA = SHELL_PATH_START,

// TODO(jam): move from content/common since it's test only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we get rid of the TODO comment from upstream?

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