Skip to content

Commit

Permalink
[dPWA] Checking that shortcut menu sizes are the same size
Browse files Browse the repository at this point in the history 
(cherry picked from commit 20d8b7a)

Bug: 1417955
Change-Id: I85d6a3f50491f276d0fce9dad50850b8a82e3499
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4367804
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Dibyajyoti Pal <dibyapal@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1121969}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4374061
Commit-Queue: Dibyajyoti Pal <dibyapal@chromium.org>
Cr-Commit-Position: refs/branch-heads/5672@{#44}
Cr-Branched-From: 5f2a724-refs/heads/main@{#1121455}
  • Loading branch information
@dmurph
dmurph authored and Chromium LUCI CQ committed Mar 27, 2023
1 parent 3477a3a commit 13c7be2
Show file tree
Hide file tree
Showing 8 changed files with 176 additions and 111 deletions.
24 changes: 15 additions & 9 deletions chrome/browser/web_applications/os_integration/shortcut_menu_handling_sub_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,20 @@ void ShortcutMenuHandlingSubManager::Execute(
void ShortcutMenuHandlingSubManager::StoreShortcutMenuData(
const AppId& app_id,
proto::ShortcutMenus* shortcut_menus,
WebAppIconManager::ShortcutIconDataVector shortcut_menu_items) {
if (shortcut_menu_items.size() == 0) {
return;
}
WebAppIconManager::ShortcutIconDataVector downloaded_shortcut_menu_items) {
std::vector<WebAppShortcutsMenuItemInfo> shortcut_menu_item_info =
registrar_->GetAppShortcutsMenuItemInfos(app_id);
CHECK_EQ(shortcut_menu_item_info.size(), shortcut_menu_items.size());
for (size_t menu_index = 0; menu_index < shortcut_menu_items.size();
// Due to the bitmaps possibly being not populated (see
// https://crbug.com/1427444), we just have empty bitmap data in that case. We
// continue to check to make sure that there aren't MORE bitmaps than
// items.
CHECK_LE(downloaded_shortcut_menu_items.size(),
shortcut_menu_item_info.size());
while (downloaded_shortcut_menu_items.size() <
shortcut_menu_item_info.size()) {
downloaded_shortcut_menu_items.emplace_back();
}
for (size_t menu_index = 0; menu_index < shortcut_menu_item_info.size();
menu_index++) {
proto::ShortcutMenuInfo* new_shortcut_menu_item =
shortcut_menus->add_shortcut_menu_info();
Expand All @@ -111,23 +117,23 @@ void ShortcutMenuHandlingSubManager::StoreShortcutMenuData(
shortcut_menu_item_info[menu_index].url.spec());

for (const auto& [size, time] :
shortcut_menu_items[menu_index][IconPurpose::ANY]) {
downloaded_shortcut_menu_items[menu_index][IconPurpose::ANY]) {
proto::ShortcutIconData* icon_data =
new_shortcut_menu_item->add_icon_data_any();
icon_data->set_icon_size(size);
icon_data->set_timestamp(syncer::TimeToProtoTime(time));
}

for (const auto& [size, time] :
shortcut_menu_items[menu_index][IconPurpose::MASKABLE]) {
downloaded_shortcut_menu_items[menu_index][IconPurpose::MASKABLE]) {
proto::ShortcutIconData* icon_data =
new_shortcut_menu_item->add_icon_data_maskable();
icon_data->set_icon_size(size);
icon_data->set_timestamp(syncer::TimeToProtoTime(time));
}

for (const auto& [size, time] :
shortcut_menu_items[menu_index][IconPurpose::MONOCHROME]) {
downloaded_shortcut_menu_items[menu_index][IconPurpose::MONOCHROME]) {
proto::ShortcutIconData* icon_data =
new_shortcut_menu_item->add_icon_data_monochrome();
icon_data->set_icon_size(size);
Expand Down
87 changes: 78 additions & 9 deletions ...me/browser/web_applications/os_integration/shortcut_menu_handling_sub_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <cstddef>
#include <memory>
#include <utility>
#include <vector>
Expand All @@ -26,6 +27,7 @@
#include "chrome/browser/web_applications/web_app_install_info.h"
#include "chrome/browser/web_applications/web_app_install_params.h"
#include "chrome/browser/web_applications/web_app_provider.h"
#include "chrome/browser/web_applications/web_app_registry_update.h"
#include "chrome/common/chrome_features.h"
#include "components/webapps/browser/install_result_code.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -242,15 +244,18 @@ TEST_P(ShortcutMenuHandlingSubManagerConfigureTest, TestConfigure) {
testing::Eq(base::StrCat(
{kWebAppUrl.spec(), base::NumberToString(menu_index)})));

EXPECT_TRUE(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_any_size() == num_sizes);
EXPECT_TRUE(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_maskable_size() == num_sizes);
EXPECT_TRUE(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_monochrome_size() == num_sizes);
EXPECT_EQ(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_any_size(),
num_sizes);
EXPECT_EQ(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_maskable_size(),
num_sizes);
EXPECT_EQ(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_monochrome_size(),
num_sizes);

for (int size_index = 0; size_index < num_sizes; size_index++) {
EXPECT_TRUE(os_integration_state.shortcut_menus()
Expand Down Expand Up @@ -284,6 +289,70 @@ TEST_P(ShortcutMenuHandlingSubManagerConfigureTest, TestConfigure) {
}
}

// This tests our handling of https://crbug.com/1427444.
TEST_P(ShortcutMenuHandlingSubManagerConfigureTest, NoDownloadedIcons_1427444) {
const int num_menu_items = 2;

const std::vector<int> sizes = {icon_size::k64, icon_size::k128};
const std::vector<SkColor> colors = {SK_ColorRED, SK_ColorRED};
const AppId& app_id = InstallWebAppWithShortcutMenuIcons(
MakeIconBitmaps({{IconPurpose::ANY, sizes, colors},
{IconPurpose::MASKABLE, sizes, colors},
{IconPurpose::MONOCHROME, sizes, colors}},
num_menu_items));
// Remove the downloaded icons & resync os integration.
{
ScopedRegistryUpdate remove_downloaded(&provider().sync_bridge_unsafe());
remove_downloaded->UpdateApp(app_id)->SetDownloadedShortcutsMenuIconsSizes(
{});
}
if (AreOsIntegrationSubManagersEnabled()) {
base::test::TestFuture<void> future;
provider().scheduler().SynchronizeOsIntegration(app_id,
future.GetCallback());
ASSERT_TRUE(future.Wait());
}

auto state =
provider().registrar_unsafe().GetAppCurrentOsIntegrationState(app_id);
ASSERT_TRUE(state.has_value());
const proto::WebAppOsIntegrationState& os_integration_state = state.value();
if (AreOsIntegrationSubManagersEnabled()) {
EXPECT_TRUE(
os_integration_state.shortcut_menus().shortcut_menu_info_size() ==
num_menu_items);

for (int menu_index = 0; menu_index < num_menu_items; menu_index++) {
EXPECT_THAT(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.shortcut_name(),
testing::Eq(base::StrCat(
{"shortcut_name", base::NumberToString(menu_index)})));

EXPECT_THAT(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.shortcut_launch_url(),
testing::Eq(base::StrCat(
{kWebAppUrl.spec(), base::NumberToString(menu_index)})));

EXPECT_EQ(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_any_size(),
0);
EXPECT_EQ(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_maskable_size(),
0);
EXPECT_EQ(os_integration_state.shortcut_menus()
.shortcut_menu_info(menu_index)
.icon_data_monochrome_size(),
0);
}
} else {
ASSERT_FALSE(os_integration_state.has_shortcut_menus());
}
}

INSTANTIATE_TEST_SUITE_P(
All,
ShortcutMenuHandlingSubManagerConfigureTest,
Expand Down
16 changes: 11 additions & 5 deletions chrome/browser/web_applications/test/web_app_test_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,11 @@ std::vector<ScopeExtensionInfo> CreateRandomScopeExtensions(

std::vector<WebAppShortcutsMenuItemInfo> CreateRandomShortcutsMenuItemInfos(
const GURL& scope,
int num,
RandomHelper& random) {
const uint32_t suffix = random.next_uint();
std::vector<WebAppShortcutsMenuItemInfo> shortcuts_menu_item_infos;
for (int i = random.next_uint(4) + 1; i >= 0; --i) {
for (int i = num - 1; i >= 0; --i) {
std::string suffix_str =
base::NumberToString(suffix) + base::NumberToString(i);
WebAppShortcutsMenuItemInfo shortcut_info;
Expand Down Expand Up @@ -308,14 +309,15 @@ std::vector<WebAppShortcutsMenuItemInfo> CreateRandomShortcutsMenuItemInfos(
}

std::vector<IconSizes> CreateRandomDownloadedShortcutsMenuIconsSizes(
int num,
RandomHelper& random) {
std::vector<IconSizes> results;
for (unsigned int i = 0; i < 3; ++i) {
for (int i = 0; i < num; ++i) {
IconSizes result;
std::vector<SquareSizePx> shortcuts_menu_icon_sizes_any;
std::vector<SquareSizePx> shortcuts_menu_icon_sizes_maskable;
std::vector<SquareSizePx> shortcuts_menu_icon_sizes_monochrome;
for (unsigned int j = 0; j < i; ++j) {
for (int j = 0; j < i; ++j) {
shortcuts_menu_icon_sizes_any.push_back(random.next_uint(256) + 1);
shortcuts_menu_icon_sizes_maskable.push_back(random.next_uint(256) + 1);
shortcuts_menu_icon_sizes_monochrome.push_back(random.next_uint(256) + 1);
Expand Down Expand Up @@ -695,10 +697,14 @@ std::unique_ptr<WebApp> CreateRandomWebApp(const GURL& base_url,
}
app->SetAdditionalSearchTerms(std::move(additional_search_terms));

int num_shortcut_menus = static_cast<int>(random.next_uint(4)) + 1;
app->SetShortcutsMenuItemInfos(
CreateRandomShortcutsMenuItemInfos(scope, random));
CreateRandomShortcutsMenuItemInfos(scope, num_shortcut_menus, random));
app->SetDownloadedShortcutsMenuIconsSizes(
CreateRandomDownloadedShortcutsMenuIconsSizes(random));
CreateRandomDownloadedShortcutsMenuIconsSizes(num_shortcut_menus,
random));
CHECK_EQ(app->shortcuts_menu_item_infos().size(),
app->downloaded_shortcuts_menu_icons_sizes().size());
app->SetManifestUrl(base_url.Resolve("/manifest" + seed_str + ".json"));

const int num_allowed_launch_protocols = random.next_uint(8);
Expand Down
21 changes: 12 additions & 9 deletions chrome/browser/web_applications/web_app_database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1097,16 +1097,13 @@ std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(
file_handler.accept.push_back(std::move(accept_entry));
}

if (WebAppFileHandlerManager::IconsEnabled()) {
absl::optional<std::vector<apps::IconInfo>> file_handler_icon_infos =
ParseAppIconInfos("WebApp", file_handler_proto.downloaded_icons());
if (!file_handler_icon_infos) {
// ParseAppIconInfos() reports any errors.
return nullptr;
}
file_handler.downloaded_icons =
std::move(file_handler_icon_infos.value());
absl::optional<std::vector<apps::IconInfo>> file_handler_icon_infos =
ParseAppIconInfos("WebApp", file_handler_proto.downloaded_icons());
if (!file_handler_icon_infos) {
// ParseAppIconInfos() reports any errors.
return nullptr;
}
file_handler.downloaded_icons = std::move(file_handler_icon_infos.value());

file_handlers.push_back(std::move(file_handler));
}
Expand Down Expand Up @@ -1194,6 +1191,7 @@ std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(
}
shortcuts_menu_item_infos.emplace_back(std::move(shortcut_info));
}
const size_t shortcut_menu_item_size = shortcuts_menu_item_infos.size();
web_app->SetShortcutsMenuItemInfos(std::move(shortcuts_menu_item_infos));

std::vector<IconSizes> shortcuts_menu_icons_sizes;
Expand All @@ -1217,6 +1215,11 @@ std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(

shortcuts_menu_icons_sizes.push_back(std::move(icon_sizes));
}
// Due to the bitmaps possibly being not populated (see
// https://crbug.com/1427444), we just have empty bitmap data in that case.
while (shortcuts_menu_icons_sizes.size() < shortcut_menu_item_size) {
shortcuts_menu_icons_sizes.emplace_back();
}
web_app->SetDownloadedShortcutsMenuIconsSizes(
std::move(shortcuts_menu_icons_sizes));

Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/web_applications/web_app_icon_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,7 @@ TypedResult<ShortcutsMenuIconBitmaps> ReadShortcutsMenuIconsBlocking(
// item.
results.value.push_back(std::move(result));
}
CHECK_EQ(shortcuts_menu_icons_sizes.size(), results.value.size());
return results;
}

Expand Down Expand Up @@ -565,6 +566,7 @@ ReadShortcutMenuIconsWithTimestampBlocking(
// item.
results.value.push_back(std::move(data));
}
CHECK_EQ(shortcuts_menu_icons_sizes.size(), results.value.size());
return results;
}

Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/web_applications/web_app_install_info.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,8 @@ struct WebAppInstallInfo {
// Vector of shortcut icon bitmaps keyed by their square size. The index of a
// given |IconBitmaps| matches that of the shortcut in
// |shortcuts_menu_item_infos| whose bitmaps it contains.
// Notes: It is not guaranteed that these are populated if the menu items are.
// See https://crbug.com/1427444.
ShortcutsMenuIconBitmaps shortcuts_menu_icon_bitmaps;

// The URL protocols/schemes that the app can handle.
Expand Down
27 changes: 22 additions & 5 deletions chrome/browser/web_applications/web_app_install_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include "chrome/browser/web_applications/web_app_chromeos_data.h"
#include "chrome/browser/web_applications/web_app_constants.h"
#include "chrome/browser/web_applications/web_app_icon_generator.h"
#include "chrome/browser/web_applications/web_app_install_info.h"
#include "chrome/browser/web_applications/web_app_install_params.h"
#include "chrome/browser/web_applications/web_app_sources.h"
#include "chrome/browser/web_applications/web_app_utils.h"
Expand Down Expand Up @@ -221,19 +222,32 @@ std::vector<SquareSizePx> GetSquareSizePxs(
}

std::vector<IconSizes> GetDownloadedShortcutsMenuIconsSizes(
const std::vector<WebAppShortcutsMenuItemInfo>& shortcuts_menu_items,
const ShortcutsMenuIconBitmaps& shortcuts_menu_icon_bitmaps) {
// Due to the bitmaps possibly being not populated (see
// https://crbug.com/1427444), we create empty bitmaps in that case. We
// continue to check to make sure that there aren't MORE bitmaps than
// items.
CHECK_LE(shortcuts_menu_icon_bitmaps.size(), shortcuts_menu_items.size());
std::vector<IconSizes> shortcuts_menu_icons_sizes;
shortcuts_menu_icons_sizes.reserve(shortcuts_menu_icon_bitmaps.size());
for (const auto& shortcut_icon_bitmaps : shortcuts_menu_icon_bitmaps) {
shortcuts_menu_icons_sizes.reserve(shortcuts_menu_items.size());
IconBitmaps empty_icon_bitmaps;
for (size_t i = 0; i < shortcuts_menu_items.size(); ++i) {
const IconBitmaps* shortcut_icon_bitmaps;
if (i < shortcuts_menu_icon_bitmaps.size()) {
shortcut_icon_bitmaps = &shortcuts_menu_icon_bitmaps[i];
} else {
shortcut_icon_bitmaps = &empty_icon_bitmaps;
}
IconSizes icon_sizes;
icon_sizes.SetSizesForPurpose(IconPurpose::ANY,
GetSquareSizePxs(shortcut_icon_bitmaps.any));
GetSquareSizePxs(shortcut_icon_bitmaps->any));
icon_sizes.SetSizesForPurpose(
IconPurpose::MASKABLE,
GetSquareSizePxs(shortcut_icon_bitmaps.maskable));
GetSquareSizePxs(shortcut_icon_bitmaps->maskable));
icon_sizes.SetSizesForPurpose(
IconPurpose::MONOCHROME,
GetSquareSizePxs(shortcut_icon_bitmaps.monochrome));
GetSquareSizePxs(shortcut_icon_bitmaps->monochrome));
shortcuts_menu_icons_sizes.push_back(std::move(icon_sizes));
}
return shortcuts_menu_icons_sizes;
Expand Down Expand Up @@ -362,6 +376,8 @@ void PopulateShortcutItemIcons(WebAppInstallInfo* web_app_info,
web_app_info->shortcuts_menu_icon_bitmaps.emplace_back(
std::move(shortcut_icon_bitmaps));
}
CHECK_EQ(web_app_info->shortcuts_menu_icon_bitmaps.size(),
web_app_info->shortcuts_menu_item_infos.size());
}

// Reconcile the file handling icons that were specified in the manifest with
Expand Down Expand Up @@ -1187,6 +1203,7 @@ void SetWebAppManifestFields(const WebAppInstallInfo& web_app_info,
web_app.SetShortcutsMenuItemInfos(web_app_info.shortcuts_menu_item_infos);
web_app.SetDownloadedShortcutsMenuIconsSizes(
GetDownloadedShortcutsMenuIconsSizes(
web_app_info.shortcuts_menu_item_infos,
web_app_info.shortcuts_menu_icon_bitmaps));
}

Expand Down
Loading

0 comments on commit 13c7be2

Please sign in to comment.