Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Fix the dynamic browser action setIcon path to work with any size icon.
Browse files Browse the repository at this point in the history 
BUG=564926,575256
[email protected]

Review URL: https://codereview.chromium.org/1580983002

Cr-Commit-Position: refs/heads/master@{#371133}
(cherry picked from commit 1686943)

Review URL: https://codereview.chromium.org/1647903003 .

Cr-Commit-Position: refs/branch-heads/2623@{#186}
Cr-Branched-From: 92d7753-refs/heads/master@{#369907}
  • Loading branch information
Evan Stade committed Jan 29, 2016
1 parent 8beab57 commit 14b3317
Show file tree
Hide file tree
Showing 12 changed files with 168 additions and 209 deletions.
167 changes: 84 additions & 83 deletions chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "ui/gfx/image/canvas_image_source.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/image/image_skia_operations.h"
#include "ui/gfx/image/image_unittest_util.h"
#include "ui/gfx/skia_util.h"

using content::WebContents;
Expand Down Expand Up @@ -68,26 +69,19 @@ const char kEmptyImageDataError[] =
"of ImageData objects.";
const char kEmptyPathError[] = "The path property must not be empty.";

// Views platforms have the icon superimposed over a button's background.
// Macs don't, but still need a 29x29-sized image (and the easiest way to do
// that is to superimpose the icon over a blank background).
gfx::ImageSkia AddBackground(const gfx::ImageSkia& icon) {
#if !defined(OS_MACOSX)
ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
gfx::ImageSkia bg = *rb.GetImageSkiaNamed(IDR_BROWSER_ACTION);
#else
const gfx::Size size(29, 29); // Size of browser actions buttons.
gfx::ImageSkia bg(new BlankImageSource(size), size);
#endif
return gfx::ImageSkiaOperations::CreateSuperimposedImage(bg, icon);
}

bool ImagesAreEqualAtScale(const gfx::ImageSkia& i1,
const gfx::ImageSkia& i2,
float scale) {
SkBitmap bitmap1 = i1.GetRepresentation(scale).sk_bitmap();
SkBitmap bitmap2 = i2.GetRepresentation(scale).sk_bitmap();
return gfx::BitmapsAreEqual(bitmap1, bitmap2);
// Makes sure |bar_rendering| has |model_icon| in the middle (there's additional
// padding that correlates to the rest of the button, and this is ignored).
void VerifyIconsMatch(const gfx::Image& bar_rendering,
const gfx::Image& model_icon) {
gfx::Rect icon_portion(gfx::Point(), bar_rendering.Size());
icon_portion.ClampToCenteredSize(model_icon.Size());

EXPECT_TRUE(gfx::test::AreBitmapsEqual(
model_icon.AsImageSkia().GetRepresentation(1.0f).sk_bitmap(),
gfx::ImageSkiaOperations::ExtractSubset(bar_rendering.AsImageSkia(),
icon_portion)
.GetRepresentation(1.0f)
.sk_bitmap()));
}

class BrowserActionApiTest : public ExtensionApiTest {
Expand Down Expand Up @@ -188,140 +182,147 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, DynamicBrowserAction) {
ASSERT_EQ(action_icon_last_id,
icon_factory.GetIcon(0).ToSkBitmap()->getGenerationID());

uint32_t action_icon_current_id = 0;
gfx::Image last_bar_icon = GetBrowserActionsBar()->GetIcon(0);
EXPECT_TRUE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));

ResultCatcher catcher;
// The reason we don't test more standard scales (like 1x, 2x, etc.) is that
// these may be generated from the provided scales.
float kSmallIconScale = 21.f / ExtensionAction::ActionIconSize();
float kLargeIconScale = 42.f / ExtensionAction::ActionIconSize();
ASSERT_FALSE(ui::IsSupportedScale(kSmallIconScale));
ASSERT_FALSE(ui::IsSupportedScale(kLargeIconScale));

// Tell the extension to update the icon using ImageData object.
ResultCatcher catcher;
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
action_icon = icon_factory.GetIcon(0);
uint32_t action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;
VerifyIconsMatch(last_bar_icon, action_icon);

EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(2.0f));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
1.0f));
// Check that only the smaller size was set (only a 21px icon was provided).
EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));

// Tell the extension to update the icon using path.
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);
// Make sure the browser action bar updated.
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon = icon_factory.GetIcon(0);
action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;
VerifyIconsMatch(last_bar_icon, action_icon);

EXPECT_FALSE(
action_icon.ToImageSkia()->HasRepresentation(2.0f));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
1.0f));
// Check that only the smaller size was set (only a 21px icon was provided).
EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));

// Tell the extension to update the icon using dictionary of ImageData
// objects.
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon = icon_factory.GetIcon(0);
action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;
VerifyIconsMatch(last_bar_icon, action_icon);

EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(2.0f));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
1.0f));
// Check both sizes were set (as two icon sizes were provided).
EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_TRUE(action_icon.AsImageSkia().HasRepresentation(kLargeIconScale));

// Tell the extension to update the icon using dictionary of paths.
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon = icon_factory.GetIcon(0);
action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;
VerifyIconsMatch(last_bar_icon, action_icon);

EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(2.0f));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
1.0f));
// Check both sizes were set (as two icon sizes were provided).
EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_TRUE(action_icon.AsImageSkia().HasRepresentation(kLargeIconScale));

// Tell the extension to update the icon using dictionary of ImageData
// objects, but setting only size 19.
// objects, but setting only one size.
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon = icon_factory.GetIcon(0);
action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;
VerifyIconsMatch(last_bar_icon, action_icon);

EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(2.0f));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
1.0f));
// Check that only the smaller size was set (only a 21px icon was provided).
EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));

// Tell the extension to update the icon using dictionary of paths, but
// setting only size 19.
// setting only one size.
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon = icon_factory.GetIcon(0);
action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;
VerifyIconsMatch(last_bar_icon, action_icon);

EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(2.0f));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon.ToImageSkia()),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
1.0f));
// Check that only the smaller size was set (only a 21px icon was provided).
EXPECT_TRUE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kLargeIconScale));

// Tell the extension to update the icon using dictionary of ImageData
// objects, but setting only size 38.
// objects, but setting only size 42.
GetBrowserActionsBar()->Press(0);
ASSERT_TRUE(catcher.GetNextResult());

action_icon = icon_factory.GetIcon(0);

const gfx::ImageSkia* action_icon_skia = action_icon.ToImageSkia();

EXPECT_FALSE(action_icon_skia->HasRepresentation(1.0f));
EXPECT_TRUE(action_icon_skia->HasRepresentation(2.0f));
EXPECT_FALSE(gfx::test::AreImagesEqual(last_bar_icon,
GetBrowserActionsBar()->GetIcon(0)));
last_bar_icon = GetBrowserActionsBar()->GetIcon(0);

action_icon = icon_factory.GetIcon(0);
action_icon_current_id = action_icon.ToSkBitmap()->getGenerationID();
EXPECT_GT(action_icon_current_id, action_icon_last_id);
action_icon_last_id = action_icon_current_id;

EXPECT_TRUE(gfx::BitmapsAreEqual(
*action_icon.ToSkBitmap(),
action_icon_skia->GetRepresentation(2.0f).sk_bitmap()));

EXPECT_TRUE(
ImagesAreEqualAtScale(AddBackground(*action_icon_skia),
*GetBrowserActionsBar()->GetIcon(0).ToImageSkia(),
2.0f));
// Check that only the larger size was set (only a 42px icon was provided).
EXPECT_FALSE(action_icon.ToImageSkia()->HasRepresentation(kSmallIconScale));
EXPECT_TRUE(action_icon.AsImageSkia().HasRepresentation(kLargeIconScale));

// Try setting icon with empty dictionary of ImageData objects.
GetBrowserActionsBar()->Press(0);
Expand Down
6 changes: 5 additions & 1 deletion chrome/browser/extensions/extension_action.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,12 @@ bool ExtensionAction::ParseIconFromCanvasDictionary(
for (base::DictionaryValue::Iterator iter(dict); !iter.IsAtEnd();
iter.Advance()) {
int icon_size = 0;
if (!base::StringToInt(iter.key(), &icon_size))
// Chrome helpfully scales the provided icon(s), but let's not go overboard.
const int kActionIconMaxSize = 10 * extension_misc::EXTENSION_ICON_ACTION;
if (!base::StringToInt(iter.key(), &icon_size) || icon_size <= 0 ||
icon_size > kActionIconMaxSize) {
continue;
}

const base::BinaryValue* image_data;
std::string binary_string64;
Expand Down
22 changes: 10 additions & 12 deletions chrome/browser/ui/extensions/icon_with_badge_image_source.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,13 @@ SkPaint* GetBadgeTextPaintSingleton() {
}

gfx::ImageSkiaRep ScaleImageSkiaRep(const gfx::ImageSkiaRep& rep,
int target_width,
float target_scale) {
gfx::Size scaled_size =
gfx::ScaleToCeiledSize(rep.pixel_size(), target_scale / rep.scale());
return gfx::ImageSkiaRep(skia::ImageOperations::Resize(
rep.sk_bitmap(),
skia::ImageOperations::RESIZE_BEST,
scaled_size.width(),
scaled_size.height()), target_scale);
return gfx::ImageSkiaRep(
skia::ImageOperations::Resize(rep.sk_bitmap(),
skia::ImageOperations::RESIZE_BEST,
target_width, target_width),
target_scale);
}

} // namespace
Expand Down Expand Up @@ -129,11 +128,10 @@ void IconWithBadgeImageSource::Draw(gfx::Canvas* canvas) {
return;

gfx::ImageSkia skia = icon_.AsImageSkia();
// TODO(estade): Fix setIcon and enable this on !MD.
if (ui::MaterialDesignController::IsModeMaterial()) {
gfx::ImageSkiaRep rep = skia.GetRepresentation(canvas->image_scale());
if (rep.scale() != canvas->image_scale())
skia.AddRepresentation(ScaleImageSkiaRep(rep, canvas->image_scale()));
gfx::ImageSkiaRep rep = skia.GetRepresentation(canvas->image_scale());
if (rep.scale() != canvas->image_scale()) {
skia.AddRepresentation(
ScaleImageSkiaRep(rep, skia.width(), canvas->image_scale()));
}
if (grayscale_)
skia = gfx::ImageSkiaOperations::CreateHSLShiftedImage(skia, {-1, 0, 0.6});
Expand Down
8 changes: 2 additions & 6 deletions chrome/browser/ui/views/toolbar/toolbar_action_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,12 +158,8 @@ void ToolbarActionView::UpdateState() {
view_controller_->GetIcon(web_contents,
GetPreferredSize()).AsImageSkia());

if (!icon.isNull()) {
gfx::ImageSkia bg = *ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
IDR_BROWSER_ACTION);
SetImage(views::Button::STATE_NORMAL,
gfx::ImageSkiaOperations::CreateSuperimposedImage(bg, icon));
}
if (!icon.isNull())
SetImage(views::Button::STATE_NORMAL, icon);

SetTooltipText(view_controller_->GetTooltip(web_contents));
SetAccessibleName(view_controller_->GetAccessibleName(web_contents));
Expand Down
14 changes: 4 additions & 10 deletions chrome/common/extensions/api/browser_action.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,28 +91,22 @@
{ "$ref": "ImageDataType" },
{
"type": "object",
"properties": {
"19": {"$ref": "ImageDataType", "optional": true},
"38": {"$ref": "ImageDataType", "optional": true}
}
"additionalProperties": { "type": "any" }
}
],
"optional": true,
"description": "Either an ImageData object or a dictionary {size -> ImageData} representing icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.imageData = foo' is equivalent to 'details.imageData = {'19': foo}'"
"description": "Either an ImageData object or a dictionary {size -> ImageData} representing icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * n will be selected, where n is the size of the icon in the UI. At least one image must be specified. Note that 'details.imageData = foo' is equivalent to 'details.imageData = {'16': foo}'"
},
"path": {
"choices": [
{ "type": "string" },
{
"type": "object",
"properties": {
"19": {"type": "string", "optional": true},
"38": {"type": "string", "optional": true}
}
"additionalProperties": { "type": "any" }
}
],
"optional": true,
"description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * 19 will be selected. Initially only scales 1 and 2 will be supported. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.imageData = {'19': foo}'"
"description": "Either a relative image path or a dictionary {size -> relative image path} pointing to icon to be set. If the icon is specified as a dictionary, the actual image to be used is chosen depending on screen's pixel density. If the number of image pixels that fit into one screen space unit equals <code>scale</code>, then image with size <code>scale</code> * n will be selected, where n is the size of the icon in the UI. At least one image must be specified. Note that 'details.path = foo' is equivalent to 'details.path = {'16': foo}'"
},
"tabId": {
"type": "integer",
Expand Down
Loading

0 comments on commit 14b3317

Please sign in to comment.