Skip to content

GitLab

T
tangled
Commit 2d9702ad authored by Rahul Singh's avatar Rahul Singh Committed by Commit Bot
Browse files
Options

desktop-pwas: Fix regression in Create Shortcuts Dialog 

This CL fixes an issue that prevents an app icon from appearing when the
Create shortcut dialog is invoked. The dialog is invoked by clicking:
Customize and control Google Chrome > More tools > Create shortcut...

Cause:
The initial CL for shortcuts menu https://crrev.com/c/2146212
modified FilterSquareIconsFromMap() in web_app_install_utils.cc.
From:
void FilterSquareIconsFromMap(const IconsMap& icons_map,
                              std::vector<SkBitmap>* square_icons)

To:
void FilterSquareIconsFromMap(
    const std::vector<WebApplicationIconInfo>& icon_infos,
    const IconsMap& icons_map,
    std::vector<SkBitmap>* square_icons)

This was done because icons_map can contain both app icon and shortcut
icon related bitmaps when a site specifies shortcuts icons in it's web
app manifest and the shortcuts menu feature is enabled.

The code inside FilterSquareIconsFromMap() iterated over icon_infos
while adding bitmaps to the square_icons vector. Those bitmaps were used
to generate app icons of various sizes.

For some websites like www.google.com icon_infos can be empty. In such
cases, the code to populate square_icons in FilterSquareIconsFromMap
doesn't run as there's nothing to iterate over. This results in an empty
square_icons which in turn leads to no app icon in the Create shortcut
dialog.

Fix:
This CL reverts FilterSquareIconsFromMap() to its state before the
shortcuts menu feature landed (no icon_infos argument).

In FilterAndResizeIconsGenerateMissing() which calls
FilterSquareIconsFromMap, the code checks if
kDesktopPWAsAppIconShortcutsMenu is enabled.

When the feature is disabled, it calls FilterSquareIconsFromMap().

When the feature is enabled, it calls the newly added:
void FilterSquareIconsFromMapDisregardShortcutIcons(
    const std::vector<WebApplicationIconInfo>& icon_infos,
    const IconsMap& icons_map,
    std::vector<SkBitmap>* square_icons){}
This method calls FilterSquareIconsFromMap() if icon_infos is empty.
Otherwise, it iterates over icon_infos.

Testing:
I tested this fix by invoking the Create shortcut dialog with the
DesktopPWAsAppIconShortcutsMenu feature enabled and also with it
disabled. In both cases, app icons showed up as expected.

I've also added a couple new unit tests for
FilterAndResizeIconsGenerateMissing().

Bug: 1084913
Change-Id: I59bf6ca45e84546a93c891c52014017bd566c8f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209968
Commit-Queue: Rahul Singh <rahsin@microsoft.com>
Reviewed-by: default avatarChase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771315}
parent f31d74f8
Hide whitespace changes
Inline Side-by-side
Showing with 112 additions and 16 deletions
chrome/browser/web_applications/components/web_app_install_utils.cc
View file @ 2d9702ad
......@@ -5,6 +5,7 @@
#include "chrome/browser/web_applications/components/web_app_install_utils.h"
#include <algorithm>
#include <string>
#include <utility>
#include "base/metrics/histogram_functions.h"
......@@ -31,12 +32,28 @@ namespace {
constexpr int kMaxIcons = 20;
constexpr SquareSizePx kMaxIconSize = 1024;
// Get a list of non-empty square icons from |icons_map|.
void FilterSquareIconsFromMap(const IconsMap& icons_map,
std::vector<SkBitmap>* square_icons) {
for (const auto& url_icon : icons_map) {
for (const SkBitmap& icon : url_icon.second) {
if (!icon.empty() && icon.width() == icon.height())
square_icons->push_back(icon);
}
}
}
// Get a list of non-empty square app icons from |icons_map|. We will disregard
// shortcut icons here.
void FilterSquareIconsFromMap(
void FilterSquareIconsFromMapDisregardShortcutIcons(
const std::vector<WebApplicationIconInfo>& icon_infos,
const IconsMap& icons_map,
std::vector<SkBitmap>* square_icons) {
if (icon_infos.empty()) {
FilterSquareIconsFromMap(icons_map, square_icons);
return;
}
for (const auto& url_icon : icons_map) {
for (const auto& info : icon_infos) {
if (info.url == url_icon.first) {
......@@ -239,8 +256,13 @@ void FilterAndResizeIconsGenerateMissing(WebApplicationInfo* web_app_info,
// which are not actually created and linked on disk.
std::vector<SkBitmap> square_icons;
if (icons_map) {
FilterSquareIconsFromMap(web_app_info->icon_infos, *icons_map,
&square_icons);
if (base::FeatureList::IsEnabled(
features::kDesktopPWAsAppIconShortcutsMenu)) {
FilterSquareIconsFromMapDisregardShortcutIcons(web_app_info->icon_infos,
*icons_map, &square_icons);
} else {
FilterSquareIconsFromMap(*icons_map, &square_icons);
}
}
FilterSquareIconsFromBitmaps(web_app_info->icon_bitmaps, &square_icons);
......
chrome/browser/web_applications/components/web_app_install_utils_unittest.cc
View file @ 2d9702ad
......@@ -4,10 +4,14 @@
#include "chrome/browser/web_applications/components/web_app_install_utils.h"
#include <string>
#include <utility>
#include "base/strings/string_number_conversions.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/web_applications/components/web_app_constants.h"
#include "chrome/browser/web_applications/components/web_app_icon_generator.h"
#include "chrome/browser/web_applications/test/web_app_icon_test_utils.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/web_application_info.h"
......@@ -62,6 +66,17 @@ constexpr SquareSizePx kIconSize = 64;
constexpr unsigned int kNumTestIcons = 30;
} // namespace
class WebAppInstallUtilsWithShortcutsMenu : public testing::Test {
public:
WebAppInstallUtilsWithShortcutsMenu() {
scoped_feature_list.InitAndEnableFeature(
features::kDesktopPWAsAppIconShortcutsMenu);
}
private:
base::test::ScopedFeatureList scoped_feature_list;
};
TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifest) {
WebApplicationInfo web_app_info;
web_app_info.title = base::UTF8ToUTF16(kAlternativeAppTitle);
......@@ -131,11 +146,8 @@ TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifest) {
}
// Tests that WebAppInfo is correctly updated when Manifest contains Shortcuts.
TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifestWithShortcuts) {
// Enable the feature.
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(
features::kDesktopPWAsAppIconShortcutsMenu);
TEST_F(WebAppInstallUtilsWithShortcutsMenu,
UpdateWebAppInfoFromManifestWithShortcuts) {
WebApplicationInfo web_app_info;
web_app_info.title = base::UTF8ToUTF16(kAlternativeAppTitle);
web_app_info.app_url = AlternativeAppUrl();
......@@ -272,10 +284,8 @@ TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifestTooManyIcons) {
}
// Tests that we limit the number of shortcut icons declared by a site.
TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifestTooManyShortcutIcons) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(
features::kDesktopPWAsAppIconShortcutsMenu);
TEST_F(WebAppInstallUtilsWithShortcutsMenu,
UpdateWebAppInfoFromManifestTooManyShortcutIcons) {
blink::Manifest manifest;
for (unsigned int i = 0; i < kNumTestIcons; ++i) {
blink::Manifest::ShortcutItem shortcut_item;
......@@ -325,10 +335,8 @@ TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifestIconsTooLarge) {
}
// Tests that we limit the size of shortcut icons declared by a site.
TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifestShortcutIconsTooLarge) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(
features::kDesktopPWAsAppIconShortcutsMenu);
TEST_F(WebAppInstallUtilsWithShortcutsMenu,
UpdateWebAppInfoFromManifestShortcutIconsTooLarge) {
blink::Manifest manifest;
for (int i = 1; i <= 20; ++i) {
blink::Manifest::ShortcutItem shortcut_item;
......@@ -414,4 +422,70 @@ TEST(WebAppInstallUtils, PopulateShortcutItemIconsNoShortcutIcons) {
EXPECT_EQ(0U, web_app_info.shortcut_infos.size());
}
// Tests that when FilterAndResizeIconsGenerateMissing is called with no
// app icon or shortcut icon data in web_app_info, web_app_info.icon_bitmaps is
// correctly populated.
TEST(WebAppInstallUtils, FilterAndResizeIconsGenerateMissingNoWebAppIconData) {
WebApplicationInfo web_app_info;
IconsMap icons_map;
std::vector<SkBitmap> bmp1 = {CreateSquareIcon(32, SK_ColorWHITE)};
icons_map.emplace(IconUrl1(), bmp1);
FilterAndResizeIconsGenerateMissing(&web_app_info, &icons_map);
EXPECT_EQ(SizesToGenerate().size(), web_app_info.icon_bitmaps.size());
}
// Tests that when FilterAndResizeIconsGenerateMissing is called with no
// app icon or shortcut icon data in web_app_info, and kDesktopPWAShortcutsMenu
// feature enabled, web_app_info.icon_bitmaps is correctly populated.
TEST_F(WebAppInstallUtilsWithShortcutsMenu,
FilterAndResizeIconsGenerateMissingNoWebAppIconData) {
WebApplicationInfo web_app_info;
IconsMap icons_map;
std::vector<SkBitmap> bmp1 = {CreateSquareIcon(32, SK_ColorWHITE)};
icons_map.emplace(IconUrl1(), bmp1);
FilterAndResizeIconsGenerateMissing(&web_app_info, &icons_map);
EXPECT_EQ(SizesToGenerate().size(), web_app_info.icon_bitmaps.size());
}
// Tests that when FilterAndResizeIconsGenerateMissing is called with both
// app icon and shortcut icon bitmaps in icons_map, web_app_info.icon_bitmaps
// is correctly populated.
TEST_F(WebAppInstallUtilsWithShortcutsMenu,
FilterAndResizeIconsGenerateMissingWithShortcutIcons) {
// Construct |icons_map| to pass to FilterAndResizeIconsGenerateMissing().
IconsMap icons_map;
std::vector<SkBitmap> bmp1 = {CreateSquareIcon(32, SK_ColorWHITE)};
std::vector<SkBitmap> bmp2 = {CreateSquareIcon(kIconSize, SK_ColorBLUE)};
icons_map.emplace(IconUrl1(), bmp1);
icons_map.emplace(IconUrl2(), bmp2);
// Construct |info| to add to |web_app_info.icon_infos|.
WebApplicationInfo web_app_info;
WebApplicationIconInfo info;
info.url = IconUrl1();
web_app_info.icon_infos.push_back(info);
// Construct |shortcut_item| to add to |web_app_info.shortcut_infos|.
WebApplicationShortcutInfo shortcut_item;
shortcut_item.name = base::UTF8ToUTF16(kShortcutItemName);
shortcut_item.url = ShortcutItemUrl();
// Construct |icon| to add to |shortcut_item.shortcut_icon_infos|.
WebApplicationIconInfo icon;
icon.url = IconUrl2();
icon.square_size_px = kIconSize;
shortcut_item.shortcut_icon_infos.push_back(std::move(icon));
shortcut_item.shortcut_icon_bitmaps[kIconSize] =
CreateSquareIcon(kIconSize, SK_ColorBLUE);
web_app_info.shortcut_infos.push_back(std::move(shortcut_item));
FilterAndResizeIconsGenerateMissing(&web_app_info, &icons_map);
EXPECT_EQ(SizesToGenerate().size(), web_app_info.icon_bitmaps.size());
for (const auto& icon_bitmap : web_app_info.icon_bitmaps) {
EXPECT_EQ(SK_ColorWHITE, icon_bitmap.second.getColor(0, 0));
}
}
} // namespace web_app
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment