Drop extensions use of legacy IPC serialization for SkBitmap, use mojo
This converts to using the skia::mojom::Bitmap serialize functions to
turn an SkBitmap into a string and back for putting into a base::Value
and sending over IPC.
This is a reland as the original CL would fail to serialize correctly
for large bitmaps, due to Bitmap mojoms trying to use a shared memory
handle, which does not encode/decode correctly as a string. Switched to
using InlineBitmap which will always serialize to bytes in the string
itself, and added a unit test for this.
Also a bonus browser test for the setIcon path from the extension
process.
[email protected]
[email protected]
Bug: 1144462
Change-Id: Idc2cfb0d70335557464be2abdeaa0bad11d38e9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527518
Commit-Queue: danakj <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/master@{#826091}
diff --git a/chrome/browser/extensions/api/DEPS b/chrome/browser/extensions/api/DEPS
index 9898aa8..88f2d8a 100644
--- a/chrome/browser/extensions/api/DEPS
+++ b/chrome/browser/extensions/api/DEPS
@@ -12,6 +12,7 @@
"+chrome/browser/ui/views/frame",
"+components/captive_portal",
"+components/web_package/test_support",
+ "+skia/public/mojom/bitmap.mojom.h",
],
"tls_socket_unittest\.cc": [
"+services/network/network_context.h",
diff --git a/chrome/browser/extensions/api/declarative_content/content_action_unittest.cc b/chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
index 3a8724a..a45f0f1 100644
--- a/chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
+++ b/chrome/browser/extensions/api/declarative_content/content_action_unittest.cc
@@ -24,12 +24,12 @@
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
#include "extensions/common/value_builder.h"
-#include "ipc/ipc_message_utils.h"
+#include "mojo/public/cpp/base/big_buffer.h"
+#include "skia/public/mojom/bitmap.mojom.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/skia/include/core/SkBitmap.h"
#include "ui/gfx/image/image.h"
-#include "ui/gfx/ipc/skia/gfx_skia_param_traits.h"
namespace extensions {
namespace {
@@ -212,65 +212,89 @@
ExtensionBuilder::ActionType::PAGE_ACTION));
TEST(DeclarativeContentActionTest, SetIcon) {
- TestExtensionEnvironment env;
- content::RenderViewHostTestEnabler rvh_enabler;
+ enum Mode { Base64, Mojo, MojoHuge };
+ for (Mode mode : {Base64, Mojo, MojoHuge}) {
+ SCOPED_TRACE(mode);
- // Simulate the process of passing ImageData to SetIcon::Create.
- SkBitmap bitmap;
- EXPECT_TRUE(bitmap.tryAllocN32Pixels(19, 19));
- // Fill the bitmap with red pixels.
- bitmap.eraseARGB(255, 255, 0, 0);
- IPC::Message bitmap_pickle;
- IPC::WriteParam(&bitmap_pickle, bitmap);
- std::string binary_data = std::string(
- static_cast<const char*>(bitmap_pickle.data()), bitmap_pickle.size());
- std::string data64;
- base::Base64Encode(binary_data, &data64);
+ TestExtensionEnvironment env;
+ content::RenderViewHostTestEnabler rvh_enabler;
- std::unique_ptr<base::DictionaryValue> dict =
- DictionaryBuilder()
- .Set("instanceType", "declarativeContent.SetIcon")
- .Set("imageData", DictionaryBuilder().Set("19", data64).Build())
- .Build();
+ // Simulate the process of passing ImageData to SetIcon::Create.
+ SkBitmap bitmap;
+ EXPECT_TRUE(bitmap.tryAllocN32Pixels(19, 19));
+ bitmap.eraseARGB(255, 255, 0, 0);
- const Extension* extension = env.MakeExtension(
- ParseJson(R"({"page_action": {"default_title": "Extension"}})"));
- base::HistogramTester histogram_tester;
- TestingProfile profile;
- std::string error;
- ContentAction::SetAllowInvisibleIconsForTest(false);
- std::unique_ptr<const ContentAction> result =
- ContentAction::Create(&profile, extension, *dict, &error);
- ContentAction::SetAllowInvisibleIconsForTest(true);
- EXPECT_EQ("", error);
- ASSERT_TRUE(result.get());
- EXPECT_THAT(
- histogram_tester.GetAllSamples("Extensions.DeclarativeSetIconWasVisible"),
- testing::ElementsAre(base::Bucket(1, 1)));
- EXPECT_THAT(histogram_tester.GetAllSamples(
- "Extensions.DeclarativeSetIconWasVisibleRendered"),
- testing::ElementsAre(base::Bucket(1, 1)));
- histogram_tester.ExpectUniqueSample(
- "Extensions.DeclarativeContentActionCreated", ContentActionType::kSetIcon,
- 1);
- histogram_tester.ExpectTotalCount(
- "Extensions.DeclarativeContentActionCreated", 1);
+ DictionaryBuilder builder;
+ builder.Set("instanceType", "declarativeContent.SetIcon");
+ switch (mode) {
+ case Base64: {
+ std::string data64 =
+ base::Base64Encode(skia::mojom::InlineBitmap::Serialize(&bitmap));
+ builder.Set("imageData", DictionaryBuilder().Set("19", data64).Build());
+ break;
+ }
+ case Mojo: {
+ std::vector<uint8_t> s = skia::mojom::InlineBitmap::Serialize(&bitmap);
+ builder.Set("imageData", DictionaryBuilder().Set("19", s).Build());
+ break;
+ }
+ case MojoHuge: {
+ // Normal skia::mojom::Bitmaps would serialize as a SharedMemory handle,
+ // which is not valid when serializing to a string. We use InlineBitmap
+ // instead, and this case verifies it does the right thing for a large
+ // image.
+ const int dimension =
+ std::ceil(std::sqrt(mojo_base::BigBuffer::kMaxInlineBytes));
+ EXPECT_TRUE(bitmap.tryAllocN32Pixels(dimension / 4 + 1, dimension));
+ EXPECT_GT(bitmap.computeByteSize(),
+ mojo_base::BigBuffer::kMaxInlineBytes);
+ bitmap.eraseARGB(255, 255, 0, 0);
+ std::vector<uint8_t> s = skia::mojom::InlineBitmap::Serialize(&bitmap);
+ builder.Set("imageData", DictionaryBuilder().Set("19", s).Build());
+ break;
+ }
+ }
+ std::unique_ptr<base::DictionaryValue> dict = builder.Build();
- ExtensionAction* action = ExtensionActionManager::Get(env.profile())
- ->GetExtensionAction(*extension);
- std::unique_ptr<content::WebContents> contents = env.MakeTab();
- const int tab_id = ExtensionTabUtil::GetTabId(contents.get());
- EXPECT_FALSE(action->GetIsVisible(tab_id));
- ContentAction::ApplyInfo apply_info = {
- extension, env.profile(), contents.get(), 100
- };
+ const Extension* extension = env.MakeExtension(
+ ParseJson(R"({"page_action": {"default_title": "Extension"}})"));
+ base::HistogramTester histogram_tester;
+ TestingProfile profile;
+ std::string error;
+ ContentAction::SetAllowInvisibleIconsForTest(false);
+ std::unique_ptr<const ContentAction> result =
+ ContentAction::Create(&profile, extension, *dict, &error);
+ ContentAction::SetAllowInvisibleIconsForTest(true);
+ EXPECT_EQ("", error);
+ ASSERT_TRUE(result.get());
+ EXPECT_THAT(histogram_tester.GetAllSamples(
+ "Extensions.DeclarativeSetIconWasVisible"),
+ testing::ElementsAre(base::Bucket(1, 1)));
+ EXPECT_THAT(histogram_tester.GetAllSamples(
+ "Extensions.DeclarativeSetIconWasVisibleRendered"),
+ testing::ElementsAre(base::Bucket(1, 1)));
+ histogram_tester.ExpectUniqueSample(
+ "Extensions.DeclarativeContentActionCreated",
+ ContentActionType::kSetIcon, 1);
+ histogram_tester.ExpectTotalCount(
+ "Extensions.DeclarativeContentActionCreated", 1);
- // The declarative icon shouldn't exist unless the content action is applied.
- EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
- result->Apply(apply_info);
- EXPECT_FALSE(action->GetDeclarativeIcon(tab_id).IsEmpty());
- result->Revert(apply_info);
- EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
+ ExtensionAction* action = ExtensionActionManager::Get(env.profile())
+ ->GetExtensionAction(*extension);
+ std::unique_ptr<content::WebContents> contents = env.MakeTab();
+ const int tab_id = ExtensionTabUtil::GetTabId(contents.get());
+ EXPECT_FALSE(action->GetIsVisible(tab_id));
+ ContentAction::ApplyInfo apply_info = {extension, env.profile(),
+ contents.get(), 100};
+
+ // The declarative icon shouldn't exist unless the content action is
+ // applied.
+ EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
+ result->Apply(apply_info);
+ EXPECT_FALSE(action->GetDeclarativeIcon(tab_id).IsEmpty());
+ result->Revert(apply_info);
+ EXPECT_TRUE(action->GetDeclarativeIcon(tab_id).IsEmpty());
+ }
}
TEST(DeclarativeContentActionTest, SetInvisibleIcon) {
@@ -283,12 +307,8 @@
uint32_t* pixels = bitmap.getAddr32(0, 0);
// Set a single pixel, which isn't enough to consider the icon visible.
pixels[0] = SkColorSetARGB(255, 255, 0, 0);
- IPC::Message bitmap_pickle;
- IPC::WriteParam(&bitmap_pickle, bitmap);
- std::string binary_data = std::string(
- static_cast<const char*>(bitmap_pickle.data()), bitmap_pickle.size());
- std::string data64;
- base::Base64Encode(binary_data, &data64);
+ std::string data64 =
+ base::Base64Encode(skia::mojom::InlineBitmap::Serialize(&bitmap));
std::unique_ptr<base::DictionaryValue> dict =
DictionaryBuilder()
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
index 71ff951..ef5d846 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
@@ -565,6 +565,40 @@
EXPECT_EQ("hi!", GetBrowserActionsBar()->GetTooltip(0));
}
+// Test that calling chrome.browserAction.setIcon() can set the icon for
+// extension.
+IN_PROC_BROWSER_TEST_P(BrowserActionApiLazyTest, BrowserActionSetIcon) {
+ ASSERT_TRUE(RunExtensionTest("browser_action/set_icon")) << message_;
+ const Extension* extension = GetSingleLoadedExtension();
+ ASSERT_TRUE(extension) << message_;
+
+ int tab_id = ExtensionTabUtil::GetTabId(
+ browser()->tab_strip_model()->GetActiveWebContents());
+
+ ExtensionAction* browser_action = GetBrowserAction(browser(), *extension);
+ ASSERT_TRUE(browser_action)
+ << "Browser action test extension should have a browser action.";
+
+ EXPECT_FALSE(browser_action->default_icon());
+ EXPECT_EQ(0u,
+ browser_action->GetExplicitlySetIcon(tab_id).RepresentationCount());
+
+ // Simulate a click on the browser action icon. The onClicked handler will
+ // call setIcon().
+ {
+ ResultCatcher catcher;
+ GetBrowserActionsBar()->Press(0);
+ ASSERT_TRUE(catcher.GetNextResult());
+ }
+
+ // The call to setIcon in background.html set an icon, so the
+ // current tab's setting should have changed, but the default setting
+ // should not have changed.
+ EXPECT_FALSE(browser_action->default_icon());
+ EXPECT_EQ(1u,
+ browser_action->GetExplicitlySetIcon(tab_id).RepresentationCount());
+}
+
// Test that calling chrome.browserAction.setPopup() can enable and change
// a popup.
IN_PROC_BROWSER_TEST_P(BrowserActionApiLazyTest, BrowserActionAddPopup) {
diff --git a/chrome/test/data/extensions/api_test/browser_action/set_icon/background.js b/chrome/test/data/extensions/api_test/browser_action/set_icon/background.js
new file mode 100644
index 0000000..b6e3953
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/set_icon/background.js
@@ -0,0 +1,24 @@
+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function getImageData() {
+ var canvas = document.createElement("canvas");
+ var ctx = canvas.getContext("2d");
+
+ ctx.fillStyle = "green";
+ ctx.fillRect(10, 10, 100, 100);
+
+ return ctx.getImageData(50, 50, 100, 100);
+}
+
+chrome.tabs.getSelected(null, function(tab) {
+ // When the browser action is clicked, add an icon.
+ chrome.browserAction.onClicked.addListener(function(tab) {
+ chrome.browserAction.setIcon({
+ imageData: getImageData()
+ });
+ chrome.test.notifyPass();
+ });
+ chrome.test.notifyPass();
+});
diff --git a/chrome/test/data/extensions/api_test/browser_action/set_icon/manifest.json b/chrome/test/data/extensions/api_test/browser_action/set_icon/manifest.json
new file mode 100644
index 0000000..aa9f00c
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/browser_action/set_icon/manifest.json
@@ -0,0 +1,14 @@
+{
+ "name": "A browser action to test chrome.browserAction.setIcon()",
+ "version": "1.0",
+ "manifest_version": 2,
+ "background": {
+ "scripts": ["background.js"]
+ },
+ "permissions": [
+ "tabs", "http://*/*"
+ ],
+ "browser_action": {
+ "default_title": "Test setIcon()"
+ }
+}
diff --git a/extensions/DEPS b/extensions/DEPS
index c37c9ba3..4d8fbad 100644
--- a/extensions/DEPS
+++ b/extensions/DEPS
@@ -18,6 +18,7 @@
"+extensions/test",
"+mojo/public",
"+services/service_manager/public",
+ "+skia/public/mojom",
"+testing",
"+third_party/blink/public/common/loader/url_loader_throttle.h",
"+third_party/skia/include",
diff --git a/extensions/browser/extension_action.cc b/extensions/browser/extension_action.cc
index 05a4547..a6564858 100644
--- a/extensions/browser/extension_action.cc
+++ b/extensions/browser/extension_action.cc
@@ -17,8 +17,7 @@
#include "extensions/common/extension_icon_set.h"
#include "extensions/common/manifest_handlers/icons_handler.h"
#include "extensions/grit/extensions_browser_resources.h"
-#include "ipc/ipc_message.h"
-#include "ipc/ipc_message_utils.h"
+#include "skia/public/mojom/bitmap.mojom.h"
#include "third_party/skia/include/core/SkBitmap.h"
#include "third_party/skia/include/core/SkCanvas.h"
#include "third_party/skia/include/core/SkPaint.h"
@@ -32,7 +31,6 @@
#include "ui/gfx/image/image.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/image/image_skia_source.h"
-#include "ui/gfx/ipc/skia/gfx_skia_param_traits.h"
#include "ui/gfx/skbitmap_operations.h"
#include "url/gurl.h"
@@ -123,23 +121,23 @@
gfx::ImageSkia* icon) {
for (base::DictionaryValue::Iterator iter(dict); !iter.IsAtEnd();
iter.Advance()) {
- std::string binary_string64;
- IPC::Message pickle;
+ std::string byte_string;
+ std::string base64_string;
+ const void* bytes = nullptr;
+ size_t num_bytes = 0;
if (iter.value().is_blob()) {
- pickle = IPC::Message(
- reinterpret_cast<const char*>(iter.value().GetBlob().data()),
- iter.value().GetBlob().size());
- } else if (iter.value().GetAsString(&binary_string64)) {
- std::string binary_string;
- if (!base::Base64Decode(binary_string64, &binary_string))
+ bytes = iter.value().GetBlob().data();
+ num_bytes = iter.value().GetBlob().size();
+ } else if (iter.value().GetAsString(&base64_string)) {
+ if (!base::Base64Decode(base64_string, &byte_string))
return IconParseResult::kDecodeFailure;
- pickle = IPC::Message(binary_string.c_str(), binary_string.length());
+ bytes = byte_string.c_str();
+ num_bytes = byte_string.length();
} else {
continue;
}
- base::PickleIterator pickle_iter(pickle);
SkBitmap bitmap;
- if (!IPC::ReadParam(&pickle, &pickle_iter, &bitmap))
+ if (!skia::mojom::InlineBitmap::Deserialize(bytes, num_bytes, &bitmap))
return IconParseResult::kUnpickleFailure;
CHECK(!bitmap.isNull());
diff --git a/extensions/common/extension_utility_types.h b/extensions/common/extension_utility_types.h
index 941a338..07a5d1e 100644
--- a/extensions/common/extension_utility_types.h
+++ b/extensions/common/extension_utility_types.h
@@ -8,8 +8,6 @@
#include <tuple>
#include <vector>
-#include "ui/gfx/ipc/skia/gfx_skia_param_traits.h"
-
class SkBitmap;
namespace base {
diff --git a/extensions/renderer/set_icon_natives.cc b/extensions/renderer/set_icon_natives.cc
index 0d008b4f73..7af1f5f 100644
--- a/extensions/renderer/set_icon_natives.cc
+++ b/extensions/renderer/set_icon_natives.cc
@@ -13,10 +13,9 @@
#include "base/bind.h"
#include "base/macros.h"
#include "base/strings/string_number_conversions.h"
-#include "content/public/common/common_param_traits.h"
#include "extensions/renderer/script_context.h"
#include "gin/data_object_builder.h"
-#include "ipc/ipc_message_utils.h"
+#include "skia/public/mojom/bitmap.mojom.h"
#include "third_party/blink/public/web/web_array_buffer_converter.h"
#include "third_party/skia/include/core/SkBitmap.h"
@@ -144,11 +143,9 @@
}
// Construct the Value object.
- IPC::Message bitmap_pickle;
- IPC::WriteParam(&bitmap_pickle, bitmap);
- blink::WebArrayBuffer buffer =
- blink::WebArrayBuffer::Create(bitmap_pickle.size(), 1);
- memcpy(buffer.Data(), bitmap_pickle.data(), bitmap_pickle.size());
+ std::vector<uint8_t> s = skia::mojom::InlineBitmap::Serialize(&bitmap);
+ blink::WebArrayBuffer buffer = blink::WebArrayBuffer::Create(s.size(), 1);
+ memcpy(buffer.Data(), s.data(), s.size());
*image_data_bitmap = blink::WebArrayBufferConverter::ToV8Value(
&buffer, context()->v8_context()->Global(), isolate);
