Skip to content

Commit

Permalink
Clipboard: Allow more flexibility in when PNGs are encoded on Chrome OS
Browse files Browse the repository at this point in the history 
While transitioning to reading images as PNGs from the clipboard, we
changed the backing store of images on Chrome OS to be raw PNG bytes
rather than bitmaps. The naive implementation meant encoding bitmaps as
PNGs on the UI thread, which is not acceptable behavior going forward.

This CL removes the ability to read images as bitmaps. Only allowing
images to be read as PNGs greatly reduces the number of states the
clipboard can be in. Images must be encoded to PNG format before they
are read.

This CL allows the owner of the clipboard to determine the best time to
encode the bitmap to a PNG by temporarily storing the bitmap for as
long as the PNG has not yet been encoded. This enables the owner to
encode the PNG on a background thread at a time it chooses.

Bug: 1247356
Change-Id: Ia87a42b3eb64725662eee97f853488342c99473e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3260985
Reviewed-by: Gary Kacmarcik <garykac@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Reviewed-by: Aya Elsayed <ayaelattar@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Reviewed-by: Olivier Li <olivierli@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#948608}
  • Loading branch information
@a-sully
a-sully authored and Chromium LUCI CQ committed Dec 6, 2021
1 parent a59c96d commit bc60932
Show file tree
Hide file tree
Showing 17 changed files with 598 additions and 82 deletions.
129 changes: 120 additions & 9 deletions ash/clipboard/clipboard_history_controller_impl.cc
View file
Expand Up @@ -4,7 +4,10 @@

#include "ash/clipboard/clipboard_history_controller_impl.h"

#include <map>
#include <memory>
#include <set>
#include <vector>

#include "ash/accelerators/accelerator_controller_impl.h"
#include "ash/clipboard/clipboard_history_menu_model_adapter.h"
Expand All @@ -22,17 +25,24 @@
#include "ash/shell.h"
#include "ash/wm/window_util.h"
#include "base/bind.h"
#include "base/json/values_util.h"
#include "base/location.h"
#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "base/ranges/algorithm.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/thread_pool.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/unguessable_token.h"
#include "base/values.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "ui/aura/window.h"
#include "ui/aura/window_tree_host.h"
#include "ui/base/accelerators/accelerator.h"
#include "ui/base/clipboard/clipboard_constants.h"
#include "ui/base/clipboard/clipboard_data.h"
#include "ui/base/clipboard/clipboard_non_backed.h"
#include "ui/base/clipboard/scoped_clipboard_writer.h"
#include "ui/base/data_transfer_policy/data_transfer_endpoint.h"
#include "ui/base/ime/input_method.h"
#include "ui/base/ime/text_input_client.h"
Expand Down Expand Up @@ -68,6 +78,24 @@ ui::ClipboardNonBacked* GetClipboard() {
return clipboard;
}

// Serially encodes bitmaps in |bitmaps_to_be_encoded| to PNGs.
// This function should run on a background thread.
// |bitmaps_to_be_encoded| maps ClipboardHistoryItem IDs to their corresponding
// bitmaps which need to be encoded. Returns a map of ClipboardHistoryItem
// IDs to encoded PNGs.
// TODO(crbug.com/1277000): Rather than encoding serially, consider posting each
// encoding as a task.
std::map<base::UnguessableToken, std::vector<uint8_t>> EncodeBitmapsToPNG(
std::map<base::UnguessableToken, SkBitmap> bitmaps_to_be_encoded) {
std::map<base::UnguessableToken, std::vector<uint8_t>> encoded_pngs;
base::ranges::for_each(bitmaps_to_be_encoded, [&](const auto& id_and_bitmap) {
encoded_pngs.emplace(
id_and_bitmap.first,
ui::ClipboardData::EncodeBitmapData(id_and_bitmap.second));
});
return encoded_pngs;
}

} // namespace

// ClipboardHistoryControllerImpl::AcceleratorTarget ---------------------------
Expand Down Expand Up @@ -285,6 +313,12 @@ gfx::Rect ClipboardHistoryControllerImpl::GetMenuBoundsInScreenForTest() const {
return context_menu_->GetMenuBoundsInScreenForTest();
}

void ClipboardHistoryControllerImpl::GetHistoryValuesForTest(
GetHistoryValuesCallback callback) const {
GetHistoryValues(/*item_id_filter=*/std::set<std::string>(),
std::move(callback));
}

bool ClipboardHistoryControllerImpl::ShouldShowNewFeatureBadge() const {
return chromeos::features::IsClipboardHistoryContextMenuNudgeEnabled() &&
nudge_controller_->ShouldShowNewFeatureBadge();
Expand Down Expand Up @@ -312,12 +346,63 @@ ClipboardHistoryControllerImpl::CreateScopedPause() {
clipboard_history_.get());
}

base::Value ClipboardHistoryControllerImpl::GetHistoryValues(
const std::set<std::string>& item_id_filter) const {
// TODO(crbug.com/1272798): If there are multiple calls in a row to
// GetHistoryValues, the same bitmaps may be encoded to PNG multiple times which
// is a resource waste. The ClipboardHistoryControllerImpl should track the ids
// of ClipboardHistoryItems for which there is a pending encoding task and avoid
// scheduling a duplicate encoding task for these items.
void ClipboardHistoryControllerImpl::GetHistoryValues(
const std::set<std::string>& item_id_filter,
GetHistoryValuesCallback callback) const {
// Map of ClipboardHistoryItem IDs to their corresponding bitmaps.
std::map<base::UnguessableToken, SkBitmap> bitmaps_to_be_encoded;
// Get the clipboard data for each clipboard history item.
for (auto& item : clipboard_history_->GetItems()) {
// If the |item_id_filter| contains values, then only return the clipboard
// items included in it.
if (!item_id_filter.empty() &&
item_id_filter.find(item.id().ToString()) == item_id_filter.end()) {
continue;
}

if (ash::ClipboardHistoryUtil::CalculateDisplayFormat(item.data()) ==
ash::ClipboardHistoryUtil::ClipboardHistoryDisplayFormat::kPng) {
const auto& maybe_png = item.data().maybe_png();
if (!maybe_png.has_value()) {
// The clipboard contains an image which has not yet been encoded to a
// PNG.
auto maybe_bitmap = item.data().GetBitmapIfPngNotEncoded();
DCHECK(maybe_bitmap.has_value());
bitmaps_to_be_encoded.emplace(item.id(),
std::move(maybe_bitmap.value()));
}
}
}

// Encode images on a background thread.
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE,
base::BindOnce(&EncodeBitmapsToPNG, std::move(bitmaps_to_be_encoded)),
base::BindOnce(
&ClipboardHistoryControllerImpl::GetHistoryValuesWithEncodedPNGs,
weak_ptr_factory_.GetWeakPtr(), item_id_filter, std::move(callback)));

if (!new_bitmap_to_write_while_encoding_for_test_.isNull()) {
ui::ScopedClipboardWriter scw(ui::ClipboardBuffer::kCopyPaste);
scw.WriteImage(new_bitmap_to_write_while_encoding_for_test_);
new_bitmap_to_write_while_encoding_for_test_.reset();
}
}

void ClipboardHistoryControllerImpl::GetHistoryValuesWithEncodedPNGs(
const std::set<std::string>& item_id_filter,
GetHistoryValuesCallback callback,
std::map<base::UnguessableToken, std::vector<uint8_t>> encoded_pngs) {
base::Value item_results(base::Value::Type::LIST);

bool all_images_encoded = true;
// Get the clipboard data for each clipboard history item.
for (const auto& item : history()->GetItems()) {
for (auto& item : clipboard_history_->GetItems()) {
// If the |item_id_filter| contains values, then only return the clipboard
// items included in it.
if (!item_id_filter.empty() &&
Expand All @@ -327,12 +412,33 @@ base::Value ClipboardHistoryControllerImpl::GetHistoryValues(

base::Value item_value(base::Value::Type::DICTIONARY);
switch (ash::ClipboardHistoryUtil::CalculateDisplayFormat(item.data())) {
case ash::ClipboardHistoryUtil::ClipboardHistoryDisplayFormat::kPng:
item_value.SetKey(kImageDataKey, base::Value(webui::GetPngDataUrl(
item.data().png().data(),
item.data().png().size())));
item_value.SetKey(kFormatDataKey, base::Value(kPngFormat));
case ash::ClipboardHistoryUtil::ClipboardHistoryDisplayFormat::kPng: {
if (!item.data().maybe_png().has_value()) {
// The clipboard contains an image which has not yet been encoded to a
// PNG. Hopefully we just finished encoding and the PNG can be found
// in `encoded_pngs`, otherwise this item was added while other PNGs
// were being encoded.
auto png_it = encoded_pngs.find(item.id());
if (png_it == encoded_pngs.end()) {
// Can't find the encoded PNG. We'll need to restart
// GetHistoryValues from the top, but allow this for loop to finish
// to let PNGs we've already encoded get set to their appropriate
// clipboards, to avoid re-encoding.
all_images_encoded = false;
} else {
item.data().SetPngDataAfterEncoding(std::move(png_it->second));
}
}

const auto& maybe_png = item.data().maybe_png();
if (maybe_png.has_value()) {
item_value.SetKey(kImageDataKey, base::Value(webui::GetPngDataUrl(
maybe_png.value().data(),
maybe_png.value().size())));
item_value.SetKey(kFormatDataKey, base::Value(kPngFormat));
}
break;
}
case ash::ClipboardHistoryUtil::ClipboardHistoryDisplayFormat::kHtml: {
const SkBitmap& bitmap =
*(resource_manager_->GetImageModel(item).GetImage().ToSkBitmap());
Expand Down Expand Up @@ -364,7 +470,12 @@ base::Value ClipboardHistoryControllerImpl::GetHistoryValues(
item_results.Append(std::move(item_value));
}

return item_results;
if (!all_images_encoded) {
GetHistoryValues(item_id_filter, std::move(callback));
return;
}

std::move(callback).Run(std::move(item_results));
}

std::vector<std::string> ClipboardHistoryControllerImpl::GetHistoryItemIds()
Expand Down
31 changes: 29 additions & 2 deletions ash/clipboard/clipboard_history_controller_impl.h
View file
Expand Up @@ -5,7 +5,9 @@
#ifndef ASH_CLIPBOARD_CLIPBOARD_HISTORY_CONTROLLER_IMPL_H_
#define ASH_CLIPBOARD_CLIPBOARD_HISTORY_CONTROLLER_IMPL_H_

#include <map>
#include <memory>
#include <set>
#include <vector>

#include "ash/ash_export.h"
Expand All @@ -14,9 +16,12 @@
#include "ash/clipboard/clipboard_history_resource_manager.h"
#include "ash/public/cpp/clipboard_history_controller.h"
#include "base/callback.h"
#include "base/callback_forward.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/timer/timer.h"
#include "base/unguessable_token.h"
#include "base/values.h"
#include "chromeos/crosapi/mojom/clipboard_history.mojom.h"

namespace aura {
Expand Down Expand Up @@ -71,6 +76,8 @@ class ASH_EXPORT ClipboardHistoryControllerImpl
// Returns bounds for the contextual menu in screen coordinates.
gfx::Rect GetMenuBoundsInScreenForTest() const;

void GetHistoryValuesForTest(GetHistoryValuesCallback callback) const;

// Whether the ClipboardHistory has items.
bool IsEmpty() const;

Expand Down Expand Up @@ -101,6 +108,10 @@ class ASH_EXPORT ClipboardHistoryControllerImpl
confirmed_operation_callback_for_test_ = new_callback;
}

void set_new_bitmap_to_write_while_encoding_for_test(const SkBitmap& bitmap) {
new_bitmap_to_write_while_encoding_for_test_ = bitmap;
}

private:
class AcceleratorTarget;
class MenuDelegate;
Expand All @@ -111,8 +122,8 @@ class ASH_EXPORT ClipboardHistoryControllerImpl
void MarkNewFeatureBadgeShown() override;
void OnScreenshotNotificationCreated() override;
std::unique_ptr<ScopedClipboardHistoryPause> CreateScopedPause() override;
base::Value GetHistoryValues(
const std::set<std::string>& item_id_filter) const override;
void GetHistoryValues(const std::set<std::string>& item_id_filter,
GetHistoryValuesCallback callback) const override;
std::vector<std::string> GetHistoryItemIds() const override;
bool PasteClipboardItemById(const std::string& item_id) override;
bool DeleteClipboardItemById(const std::string& item_id) override;
Expand All @@ -128,6 +139,16 @@ class ASH_EXPORT ClipboardHistoryControllerImpl
void OnCachedImageModelUpdated(
const std::vector<base::UnguessableToken>& menu_item_ids) override;

// Invoked by GetHistoryValues once all clipboard instances with images have
// been encoded into PNGs. Returns the history which tracks what is being
// copied to the clipboard. Only the items listed in `item_id_filter` are
// returned. If `item_id_filter` is empty, then all items in the history are
// returned.
void GetHistoryValuesWithEncodedPNGs(
const std::set<std::string>& item_id_filter,
GetHistoryValuesCallback callback,
std::map<base::UnguessableToken, std::vector<uint8_t>> encoded_pngs);

void ExecuteSelectedMenuItem(int event_flags);

// Executes the command specified by `command_id` with the given event flags.
Expand Down Expand Up @@ -200,6 +221,12 @@ class ASH_EXPORT ClipboardHistoryControllerImpl
// operation.
base::RepeatingClosure confirmed_operation_callback_for_test_;

// A new bitmap to be written to the clipboard while existing images are being
// encoded during `GetHistoryValues()`, which will force `GetHistoryValues()`
// to re-run in order to encode this new bitmap. This member is marked mutable
// so it can be cleared once it has been written to the clipboard.
mutable SkBitmap new_bitmap_to_write_while_encoding_for_test_;

base::WeakPtrFactory<ClipboardHistoryControllerImpl> weak_ptr_factory_{this};
};

Expand Down

0 comments on commit bc60932

Please sign in to comment.