Skip to content

Commit

Permalink
Stub in holding space support for dynamic images.
Browse files Browse the repository at this point in the history
This is the first step in supporting dynamic images for holding space as
is needed due to the fact that file thumbnails are loaded async. The
next CL will wire the new `HoldingSpaceImage` class up to the existing
but currently unused `HoldingSpaceThumbnailLoader`.

Bug: 1113772
Change-Id: I5ffc660d5eb3462c247a530f61811c40fcb9b550
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2387264
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Slow <xiyuan@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803737}
  • Loading branch information
David Black authored and Commit Bot committed Sep 2, 2020
1 parent 677ab16 commit c06d590
Show file tree
Hide file tree
Showing 24 changed files with 289 additions and 144 deletions.
2 changes: 2 additions & 0 deletions ash/public/cpp/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ component("cpp") {
"holding_space/holding_space_controller.cc",
"holding_space/holding_space_controller.h",
"holding_space/holding_space_controller_observer.h",
"holding_space/holding_space_image.cc",
"holding_space/holding_space_image.h",
"holding_space/holding_space_item.cc",
"holding_space/holding_space_item.h",
"holding_space/holding_space_model.cc",
Expand Down
71 changes: 71 additions & 0 deletions ash/public/cpp/holding_space/holding_space_image.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// 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.

#include "ash/public/cpp/holding_space/holding_space_image.h"

#include <memory>

#include "ui/gfx/image/image_skia_source.h"
#include "ui/gfx/skia_util.h"

namespace ash {

// HoldingSpaceImage::ImageSkiaSource ------------------------------------------

class HoldingSpaceImage::ImageSkiaSource : public gfx::ImageSkiaSource {
public:
explicit ImageSkiaSource(const gfx::ImageSkia& placeholder)
: placeholder_(placeholder) {}
ImageSkiaSource(const ImageSkiaSource&) = delete;
ImageSkiaSource& operator=(const ImageSkiaSource&) = delete;
~ImageSkiaSource() override = default;

private:
// gfx::ImageSkiaSource:
gfx::ImageSkiaRep GetImageForScale(float scale) override {
// TODO(dmblack): Retrieve thumbnail and call `NotifyUpdated()` when ready.
return placeholder_.GetRepresentation(scale);
}

const gfx::ImageSkia placeholder_;
};

// HoldingSpaceImage -----------------------------------------------------------

HoldingSpaceImage::HoldingSpaceImage(const gfx::ImageSkia& placeholder)
: image_skia_(std::make_unique<ImageSkiaSource>(placeholder),
placeholder.size()) {}

HoldingSpaceImage::~HoldingSpaceImage() {
NotifyDestroying();
}

bool HoldingSpaceImage::operator==(const HoldingSpaceImage& rhs) const {
return gfx::BitmapsAreEqual(*image_skia_.bitmap(), *rhs.image_skia_.bitmap());
}

void HoldingSpaceImage::AddObserver(Observer* observer) const {
observers_.AddObserver(observer);
}

void HoldingSpaceImage::RemoveObserver(Observer* observer) const {
observers_.RemoveObserver(observer);
}

void HoldingSpaceImage::NotifyDestroying() {
for (auto& observer : observers_)
observer.OnHoldingSpaceImageDestroying(this);
}

void HoldingSpaceImage::NotifyUpdated(float scale) {
// Force invalidate `image_skia_` for `scale` so that it will request the
// updated `gfx::ImageSkiaRep` at next access.
image_skia_.RemoveRepresentation(scale);
image_skia_.RemoveUnsupportedRepresentationsForScale(scale);

for (auto& observer : observers_)
observer.OnHoldingSpaceImageUpdated(this);
}

} // namespace ash
60 changes: 60 additions & 0 deletions ash/public/cpp/holding_space/holding_space_image.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// 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.

#ifndef ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_IMAGE_H_
#define ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_IMAGE_H_

#include "ash/public/cpp/ash_public_export.h"
#include "base/observer_list.h"
#include "ui/gfx/image/image_skia.h"

namespace ash {

// TODO(dmblack): Implement dynamic updating.
// A wrapper around a `gfx::ImageSkia` that supports dynamic updates. When
// updates occur or an instance is being destroyed, observers are notified.
class ASH_PUBLIC_EXPORT HoldingSpaceImage {
public:
// An observer which receives notifications of `HoldingSpaceImage` events.
class Observer : public base::CheckedObserver {
public:
// Invoked when the `HoldingSpaceImage` is updated. UI classes should react
// to this event by invalidating any associated views.
virtual void OnHoldingSpaceImageUpdated(const HoldingSpaceImage*) {}

// Invoked when the `HoldingSpaceImage` is being destroyed. Any observers
// should react to this event by unregistering from the observer list.
virtual void OnHoldingSpaceImageDestroying(const HoldingSpaceImage*) {}
};

explicit HoldingSpaceImage(const gfx::ImageSkia& placeholder);
HoldingSpaceImage(const HoldingSpaceImage&) = delete;
HoldingSpaceImage& operator=(const HoldingSpaceImage&) = delete;
~HoldingSpaceImage();

bool operator==(const HoldingSpaceImage& rhs) const;

// Adds/remove the specified `observer`.
void AddObserver(Observer* observer) const;
void RemoveObserver(Observer* observer) const;

// Returns the underlying `gfx::ImageSkia`. Note that the image source may be
// dynamically updated, so UI classes should observe and react to updates.
const gfx::ImageSkia& image_skia() const { return image_skia_; }

private:
class ImageSkiaSource;

void NotifyDestroying();
void NotifyUpdated(float scale);

gfx::ImageSkia image_skia_;

// Mutable to allow const access from `AddObserver()`/`RemoveObserver()`.
mutable base::ObserverList<HoldingSpaceImage::Observer> observers_;
};

} // namespace ash

#endif // ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_IMAGE_H_
12 changes: 6 additions & 6 deletions ash/public/cpp/holding_space/holding_space_item.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

#include "ash/public/cpp/holding_space/holding_space_item.h"

#include "base/files/file_path.h"
#include "ash/public/cpp/holding_space/holding_space_image.h"
#include "base/memory/ptr_util.h"
#include "base/strings/strcat.h"
#include "base/util/values/values_util.h"
Expand Down Expand Up @@ -45,7 +45,7 @@ HoldingSpaceItem::~HoldingSpaceItem() = default;
bool HoldingSpaceItem::operator==(const HoldingSpaceItem& rhs) const {
return type_ == rhs.type_ && id_ == rhs.id_ && file_path_ == rhs.file_path_ &&
file_system_url_ == rhs.file_system_url_ && text_ == rhs.text_ &&
image_.BackedBySameObjectAs(rhs.image_);
*image_ == *rhs.image_;
}

// static
Expand All @@ -60,11 +60,11 @@ std::unique_ptr<HoldingSpaceItem> HoldingSpaceItem::CreateFileBackedItem(
Type type,
const base::FilePath& file_path,
const GURL& file_system_url,
const gfx::ImageSkia& image) {
std::unique_ptr<HoldingSpaceImage> image) {
// Note: std::make_unique does not work with private constructors.
return base::WrapUnique(new HoldingSpaceItem(
type, GetFileBackedItemId(type, file_path), file_path, file_system_url,
file_path.BaseName().LossyDisplayName(), image));
file_path.BaseName().LossyDisplayName(), std::move(image)));
}

// static
Expand Down Expand Up @@ -123,12 +123,12 @@ HoldingSpaceItem::HoldingSpaceItem(Type type,
const base::FilePath& file_path,
const GURL& file_system_url,
const base::string16& text,
const gfx::ImageSkia& image)
std::unique_ptr<HoldingSpaceImage> image)
: type_(type),
id_(id),
file_path_(file_path),
file_system_url_(file_system_url),
text_(text),
image_(image) {}
image_(std::move(image)) {}

} // namespace ash
16 changes: 9 additions & 7 deletions ash/public/cpp/holding_space/holding_space_item.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

#include "ash/public/cpp/ash_public_export.h"
#include "base/callback_forward.h"
#include "base/files/file_path.h"
#include "base/strings/string16.h"
#include "ui/gfx/image/image_skia.h"
#include "url/gurl.h"

namespace base {
Expand All @@ -20,6 +20,8 @@ class DictionaryValue;

namespace ash {

class HoldingSpaceImage;

// Contains data needed to display a single item in the temporary holding space
// UI.
class ASH_PUBLIC_EXPORT HoldingSpaceItem {
Expand Down Expand Up @@ -50,14 +52,14 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
Type type,
const base::FilePath& file_path,
const GURL& file_system_url,
const gfx::ImageSkia& image);
std::unique_ptr<HoldingSpaceImage> image);

// Returns a file system URL for a given file path.
using FileSystemUrlResolver = base::OnceCallback<GURL(const base::FilePath&)>;

// Returns an image for a given file path.
using ImageResolver =
base::OnceCallback<gfx::ImageSkia(const base::FilePath&)>;
using ImageResolver = base::OnceCallback<std::unique_ptr<HoldingSpaceImage>(
const base::FilePath&)>;

// Deserializes from `base::DictionaryValue` to `HoldingSpaceItem`.
static std::unique_ptr<HoldingSpaceItem> Deserialize(
Expand All @@ -77,7 +79,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {

const base::string16& text() const { return text_; }

const gfx::ImageSkia& image() const { return image_; }
const HoldingSpaceImage& image() const { return *image_; }

const base::FilePath& file_path() const { return file_path_; }

Expand All @@ -90,7 +92,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
const base::FilePath& file_path,
const GURL& file_system_url,
const base::string16& text,
const gfx::ImageSkia& image);
std::unique_ptr<HoldingSpaceImage> image);

const Type type_;

Expand All @@ -107,7 +109,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
base::string16 text_;

// The image representation of the item.
gfx::ImageSkia image_;
std::unique_ptr<HoldingSpaceImage> image_;
};

} // namespace ash
Expand Down
15 changes: 10 additions & 5 deletions ash/public/cpp/holding_space/holding_space_item_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
#include <memory>
#include <vector>

#include "ash/public/cpp/holding_space/holding_space_image.h"
#include "base/test/bind_test_util.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/image/image_skia.h"
#include "ui/gfx/image/image_unittest_util.h"

namespace ash {
Expand All @@ -30,10 +32,11 @@ using HoldingSpaceItemTest = testing::TestWithParam<HoldingSpaceItem::Type>;
TEST_P(HoldingSpaceItemTest, Serialization) {
const base::FilePath file_path("file_path");
const GURL file_system_url("file_system_url");
const gfx::ImageSkia image(gfx::test::CreateImageSkia(10, 10));
const gfx::ImageSkia placeholder(gfx::test::CreateImageSkia(10, 10));

const auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
/*type=*/GetParam(), file_path, file_system_url, image);
/*type=*/GetParam(), file_path, file_system_url,
std::make_unique<HoldingSpaceImage>(placeholder));

const base::DictionaryValue serialized_holding_space_item =
holding_space_item->Serialize();
Expand All @@ -44,8 +47,9 @@ TEST_P(HoldingSpaceItemTest, Serialization) {
base::BindLambdaForTesting(
[&](const base::FilePath& file_path) { return file_system_url; }),
/*image_resolver=*/
base::BindLambdaForTesting(
[&](const base::FilePath& file_path) { return image; }));
base::BindLambdaForTesting([&](const base::FilePath& file_path) {
return std::make_unique<HoldingSpaceImage>(placeholder);
}));

EXPECT_EQ(*deserialized_holding_space_item, *holding_space_item);
}
Expand All @@ -54,7 +58,8 @@ TEST_P(HoldingSpaceItemTest, Serialization) {
TEST_P(HoldingSpaceItemTest, DeserializeId) {
const auto holding_space_item = HoldingSpaceItem::CreateFileBackedItem(
/*type=*/GetParam(), base::FilePath("file_path"), GURL("file_system_url"),
gfx::test::CreateImageSkia(10, 10));
std::make_unique<HoldingSpaceImage>(
/*placeholder=*/gfx::test::CreateImageSkia(10, 10)));

const base::DictionaryValue serialized_holding_space_item =
holding_space_item->Serialize();
Expand Down
2 changes: 1 addition & 1 deletion ash/public/cpp/holding_space/holding_space_model.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <vector>

#include "ash/public/cpp/ash_public_export.h"
#include "base/callback_forward.h"
#include "base/callback.h"
#include "base/observer_list.h"

namespace ash {
Expand Down
Loading

0 comments on commit c06d590

Please sign in to comment.