Skip to content

Commit

Permalink
borealis: Add internal/external monitor counts to feedback form.
Browse files Browse the repository at this point in the history
We're getting a lot of windowing issues reported, sometimes
without enough detail to know whether they're for external
monitors (for which there are multiple KIs) or internal ones.
It's valuable to know which case is being tested/reported.

BUG=b:191820689
TEST=play games with(out) external monitor attached, and with(out) internal monitor enabled

Change-Id: I1fe67842fee670f21945a78d97920ccb56155ed1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3552185
Reviewed-by: Daniel Ng <danielng@google.com>
Commit-Queue: Chloe Pelling <cpelling@google.com>
Cr-Commit-Position: refs/heads/main@{#986865}
  • Loading branch information
cpelling authored and Chromium LUCI CQ committed Mar 30, 2022
1 parent 683a5e4 commit 0f3f54a
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 17 deletions.
3 changes: 3 additions & 0 deletions chrome/browser/ash/borealis/borealis_context_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "components/exo/shell_surface_util.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/display/test/test_screen.h"

namespace borealis {

Expand All @@ -39,6 +40,7 @@ class BorealisContextTest : public testing::Test,
BorealisContextTest()
: new_window_provider_(std::make_unique<ash::TestNewWindowDelegate>()) {
profile_ = std::make_unique<TestingProfile>();
display::Screen::SetScreenInstance(&test_screen_);
borealis_disk_manager_dispatcher_ =
std::make_unique<BorealisDiskManagerDispatcher>();
borealis_shutdown_monitor_ =
Expand Down Expand Up @@ -82,6 +84,7 @@ class BorealisContextTest : public testing::Test,

protected:
content::BrowserTaskEnvironment task_env_;
display::test::TestScreen test_screen_;
std::unique_ptr<borealis::BorealisContext> borealis_context_;
std::unique_ptr<TestingProfile> profile_;
BorealisServiceFake* service_fake_;
Expand Down
48 changes: 33 additions & 15 deletions chrome/browser/ash/borealis/borealis_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "components/exo/shell_surface_util.h"
#include "net/base/url_util.h"
#include "third_party/re2/src/re2/re2.h"
#include "ui/display/screen.h"

namespace borealis {

Expand Down Expand Up @@ -50,6 +51,8 @@ static constexpr char kAppNameKey[] = "entry.504707995";
// JSON keys for prefilling JSON section.
static constexpr char kJSONAppIdKey[] = "steam_appid";
static constexpr char kJSONBoardKey[] = "board";
static constexpr char kJSONMonitorsExternal[] = "external_monitors";
static constexpr char kJSONMonitorsInternal[] = "internal_monitors";
static constexpr char kJSONPlatformKey[] = "platform_version";
static constexpr char kJSONProtonKey[] = "proton_version";
static constexpr char kJSONSpecsKey[] = "specs";
Expand All @@ -65,16 +68,19 @@ static constexpr char kNonGameIdHash1[] = "hnfpbccfbbbjkmcalgjofgokpgjjppon";
static constexpr char kNonGameIdHash2[] = "kooplpnkalpdpoohnhmlmfebokjkgnlb";
static constexpr char kNonGameIdHash3[] = "bmhgcnboebpgmobfgfjcfplecleopefa";

GURL GetSysInfoForUrlAsync(GURL url,
absl::optional<int> game_id,
std::string owner_id) {
base::DictionaryValue json_root;
GURL AssembleUrlAsync(std::string owner_id,
absl::optional<int> game_id,
std::string window_title) {
GURL url(kFeedbackUrl);
url = net::AppendQueryParameter(url, kAppNameKey, window_title);

base::DictionaryValue json_root;
if (game_id.has_value()) {
json_root.SetString(kJSONAppIdKey,
base::StringPrintf("%d", game_id.value()));
}

// System specs
json_root.SetString(kJSONBoardKey, base::SysInfo::HardwareModelName());
json_root.SetString(
kJSONSpecsKey,
Expand All @@ -85,6 +91,21 @@ GURL GetSysInfoForUrlAsync(GURL url,
json_root.SetString(kJSONPlatformKey,
base::SysInfo::OperatingSystemVersion());

// Number of monitors
int internal_displays = 0;
int external_displays = 0;
for (const display::Display& d :
display::Screen::GetScreen()->GetAllDisplays()) {
if (d.IsInternal()) {
internal_displays++;
} else {
external_displays++;
}
}
json_root.SetInteger(kJSONMonitorsInternal, internal_displays);
json_root.SetInteger(kJSONMonitorsExternal, external_displays);

// Proton/SLR versions
borealis::ProtonVersionInfo version_info;
std::string output;
if (borealis::GetProtonVersionInfo(owner_id, &output)) {
Expand All @@ -96,10 +117,9 @@ GURL GetSysInfoForUrlAsync(GURL url,
json_root.SetString(kJSONProtonKey, version_info.proton);
json_root.SetString(kJSONSteamKey, version_info.slr);

std::string json_string;
base::JSONWriter::Write(json_root, &json_string);
url = net::AppendQueryParameter(url, kDeviceInformationKey, json_string);

std::string device_info;
base::JSONWriter::Write(json_root, &device_info);
url = net::AppendQueryParameter(url, kDeviceInformationKey, device_info);
return url;
}

Expand Down Expand Up @@ -147,22 +167,20 @@ void FeedbackFormUrl(Profile* const profile,
return;
}

GURL url(kFeedbackUrl);
url = net::AppendQueryParameter(url, kAppNameKey, window_title);

// Attempt to get the Borealis app ID.
// TODO(b/173977876): Implement this in a more reliable way.
absl::optional<int> borealis_app_id;
absl::optional<int> game_id;
absl::optional<guest_os::GuestOsRegistryService::Registration> registration =
registry_service->GetRegistration(app_id);
if (registration.has_value()) {
borealis_app_id = GetBorealisAppId(registration->Exec());
game_id = GetBorealisAppId(registration->Exec());
}

base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, base::MayBlock(),
base::BindOnce(&GetSysInfoForUrlAsync, url, borealis_app_id,
ash::ProfileHelper::GetUserIdHashFromProfile(profile)),
base::BindOnce(&AssembleUrlAsync,
ash::ProfileHelper::GetUserIdHashFromProfile(profile),
std::move(game_id), std::move(window_title)),
std::move(url_callback));
}

Expand Down
9 changes: 7 additions & 2 deletions chrome/browser/ash/borealis/borealis_util_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@
#include "net/base/url_util.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/display/test/test_screen.h"

namespace borealis {
namespace {

class BorealisUtilTest : public testing::Test {
public:
BorealisUtilTest() { display::Screen::SetScreenInstance(&test_screen_); }

protected:
GURL GetFeedbackFormUrl(TestingProfile* profile,
const std::string& app_id,
Expand All @@ -34,6 +38,7 @@ class BorealisUtilTest : public testing::Test {
return returned_url;
}

display::test::TestScreen test_screen_;
content::BrowserTaskEnvironment task_environment_;
};

Expand Down Expand Up @@ -100,8 +105,8 @@ TEST_F(BorealisUtilTest, FeedbackFormUrlIsPrefilled) {
EXPECT_TRUE(
net::GetValueForKeyInQuery(url, kDeviceInformationKey, &json_string));
auto json_root = base::JSONReader::Read(json_string);
EXPECT_EQ(json_root.value().GetDict().size(), 5); // we expect the JSON field
// to have 5 key/value pairs.
// We currently add this many key/value pairs to the JSON field.
EXPECT_EQ(json_root.value().GetDict().size(), 7);
}

TEST_F(BorealisUtilTest, ProtonVersionProtonTitle) {
Expand Down

0 comments on commit 0f3f54a

Please sign in to comment.