Commit 9a13b0fc authored by treib's avatar treib Committed by Commit bot

[Doodle] Move image fetching from LogoBridge to DoodleService

This will allow us to transparently implement different image fetching
logic (e.g. inlining the image data into the doodle config).
And this logic doesn't really belong in the bridge anyway.

BUG=719513

Review-Url: https://codereview.chromium.org/2886443002
Cr-Commit-Position: refs/heads/master@{#472732}
parent 71bba86c
...@@ -19,8 +19,6 @@ ...@@ -19,8 +19,6 @@
#include "chrome/browser/doodle/doodle_service_factory.h" #include "chrome/browser/doodle/doodle_service_factory.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_android.h" #include "chrome/browser/profiles/profile_android.h"
#include "chrome/browser/search/suggestions/image_decoder_impl.h"
#include "components/image_fetcher/core/image_fetcher_impl.h"
#include "components/search_provider_logos/logo_tracker.h" #include "components/search_provider_logos/logo_tracker.h"
#include "jni/LogoBridge_jni.h" #include "jni/LogoBridge_jni.h"
#include "net/url_request/url_fetcher.h" #include "net/url_request/url_fetcher.h"
...@@ -40,8 +38,6 @@ using base::android::ToJavaByteArray; ...@@ -40,8 +38,6 @@ using base::android::ToJavaByteArray;
namespace { namespace {
const int64_t kMaxImageDownloadBytes = 1024 * 1024;
ScopedJavaLocalRef<jobject> MakeJavaLogo(JNIEnv* env, ScopedJavaLocalRef<jobject> MakeJavaLogo(JNIEnv* env,
const SkBitmap* bitmap, const SkBitmap* bitmap,
const GURL& on_click_url, const GURL& on_click_url,
...@@ -200,11 +196,6 @@ LogoBridge::LogoBridge(jobject j_profile) ...@@ -200,11 +196,6 @@ LogoBridge::LogoBridge(jobject j_profile)
if (base::FeatureList::IsEnabled(chrome::android::kUseNewDoodleApi)) { if (base::FeatureList::IsEnabled(chrome::android::kUseNewDoodleApi)) {
doodle_service_ = DoodleServiceFactory::GetForProfile(profile); doodle_service_ = DoodleServiceFactory::GetForProfile(profile);
image_fetcher_ = base::MakeUnique<image_fetcher::ImageFetcherImpl>(
base::MakeUnique<suggestions::ImageDecoderImpl>(),
profile->GetRequestContext());
image_fetcher_->SetImageDownloadLimit(kMaxImageDownloadBytes);
doodle_observer_.Add(doodle_service_); doodle_observer_.Add(doodle_service_);
} else { } else {
logo_service_ = LogoServiceFactory::GetForProfile(profile); logo_service_ = LogoServiceFactory::GetForProfile(profile);
...@@ -284,47 +275,32 @@ void LogoBridge::FetchDoodleImage(const doodle::DoodleConfig& doodle_config, ...@@ -284,47 +275,32 @@ void LogoBridge::FetchDoodleImage(const doodle::DoodleConfig& doodle_config,
bool from_cache) { bool from_cache) {
DCHECK(!j_logo_observer_.is_null()); DCHECK(!j_logo_observer_.is_null());
// If there is a CTA image, that means the main image is animated. Show the // If there is a CTA image, that means the main image is animated. We show the
// non-animated CTA image first, and load the animated one only when the // non-animated CTA image first, and load the animated one only when the
// user requests it. // user requests it.
bool has_cta = doodle_config.large_cta_image.has_value(); bool has_cta = doodle_config.large_cta_image.has_value();
const GURL& image_url = has_cta ? doodle_config.large_cta_image->url
: doodle_config.large_image.url;
const GURL& animated_image_url = const GURL& animated_image_url =
has_cta ? doodle_config.large_image.url : GURL::EmptyGURL(); has_cta ? doodle_config.large_image.url : GURL::EmptyGURL();
// TODO(treib): For interactive doodles, use |fullpage_interactive_url|
// instead of |target_url|?
const GURL& on_click_url = doodle_config.target_url; const GURL& on_click_url = doodle_config.target_url;
const std::string& alt_text = doodle_config.alt_text; const std::string& alt_text = doodle_config.alt_text;
image_fetcher_->StartOrQueueNetworkRequest( doodle_service_->GetImage(
image_url.spec(), image_url,
base::Bind(&LogoBridge::DoodleImageFetched, base::Unretained(this), base::Bind(&LogoBridge::DoodleImageFetched, base::Unretained(this),
from_cache, on_click_url, alt_text, animated_image_url)); from_cache, on_click_url, alt_text, animated_image_url));
} }
void LogoBridge::DoodleImageFetched( void LogoBridge::DoodleImageFetched(bool config_from_cache,
bool config_from_cache, const GURL& on_click_url,
const GURL& on_click_url, const std::string& alt_text,
const std::string& alt_text, const GURL& animated_image_url,
const GURL& animated_image_url, const gfx::Image& image) {
const std::string& image_fetch_id,
const gfx::Image& image,
const image_fetcher::RequestMetadata& metadata) {
JNIEnv* env = base::android::AttachCurrentThread(); JNIEnv* env = base::android::AttachCurrentThread();
if (image.IsEmpty()) { ScopedJavaLocalRef<jobject> j_logo;
DLOG(WARNING) << "Failed to download doodle image"; if (!image.IsEmpty()) {
Java_LogoObserver_onLogoAvailable(env, j_logo_observer_, j_logo = MakeJavaLogo(env, image.ToSkBitmap(), on_click_url, alt_text,
ScopedJavaLocalRef<jobject>(), animated_image_url);
config_from_cache);
return;
} }
UMA_HISTOGRAM_BOOLEAN("NewTabPage.LogoImageDownloaded",
metadata.from_http_cache);
ScopedJavaLocalRef<jobject> j_logo = MakeJavaLogo(
env, image.ToSkBitmap(), on_click_url, alt_text, animated_image_url);
Java_LogoObserver_onLogoAvailable(env, j_logo_observer_, j_logo, Java_LogoObserver_onLogoAvailable(env, j_logo_observer_, j_logo,
config_from_cache); config_from_cache);
} }
......
...@@ -26,11 +26,6 @@ namespace gfx { ...@@ -26,11 +26,6 @@ namespace gfx {
class Image; class Image;
} // namespace gfx } // namespace gfx
namespace image_fetcher {
class ImageFetcher;
struct RequestMetadata;
} // namespace image_fetcher
// The C++ counterpart to LogoBridge.java. Enables Java code to access the // The C++ counterpart to LogoBridge.java. Enables Java code to access the
// default search provider's logo. // default search provider's logo.
class LogoBridge : public doodle::DoodleService::Observer { class LogoBridge : public doodle::DoodleService::Observer {
...@@ -76,16 +71,13 @@ class LogoBridge : public doodle::DoodleService::Observer { ...@@ -76,16 +71,13 @@ class LogoBridge : public doodle::DoodleService::Observer {
const GURL& on_click_url, const GURL& on_click_url,
const std::string& alt_text, const std::string& alt_text,
const GURL& animated_image_url, const GURL& animated_image_url,
const std::string& image_fetch_id, const gfx::Image& image);
const gfx::Image& image,
const image_fetcher::RequestMetadata& metadata);
// Only valid if UseNewDoodleApi is disabled. // Only valid if UseNewDoodleApi is disabled.
LogoService* logo_service_; LogoService* logo_service_;
// Only valid if UseNewDoodleApi is enabled. // Only valid if UseNewDoodleApi is enabled.
doodle::DoodleService* doodle_service_; doodle::DoodleService* doodle_service_;
std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
base::android::ScopedJavaGlobalRef<jobject> j_logo_observer_; base::android::ScopedJavaGlobalRef<jobject> j_logo_observer_;
ScopedObserver<doodle::DoodleService, doodle::DoodleService::Observer> ScopedObserver<doodle::DoodleService, doodle::DoodleService::Observer>
doodle_observer_; doodle_observer_;
......
...@@ -14,9 +14,11 @@ ...@@ -14,9 +14,11 @@
#include "base/timer/timer.h" #include "base/timer/timer.h"
#include "chrome/browser/google/google_url_tracker_factory.h" #include "chrome/browser/google/google_url_tracker_factory.h"
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search/suggestions/image_decoder_impl.h"
#include "components/doodle/doodle_fetcher.h" #include "components/doodle/doodle_fetcher.h"
#include "components/doodle/doodle_fetcher_impl.h" #include "components/doodle/doodle_fetcher_impl.h"
#include "components/doodle/doodle_service.h" #include "components/doodle/doodle_service.h"
#include "components/image_fetcher/core/image_fetcher_impl.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h" #include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/prefs/pref_service.h" #include "components/prefs/pref_service.h"
#include "components/safe_json/safe_json_parser.h" #include "components/safe_json/safe_json_parser.h"
...@@ -85,5 +87,8 @@ KeyedService* DoodleServiceFactory::BuildServiceInstanceFor( ...@@ -85,5 +87,8 @@ KeyedService* DoodleServiceFactory::BuildServiceInstanceFor(
base::MakeUnique<base::OneShotTimer>(), base::MakeUnique<base::OneShotTimer>(),
base::MakeUnique<base::DefaultClock>(), base::MakeUnique<base::DefaultClock>(),
base::MakeUnique<base::DefaultTickClock>(), base::MakeUnique<base::DefaultTickClock>(),
/*override_min_refresh_interval=*/base::nullopt); /*override_min_refresh_interval=*/base::nullopt,
base::MakeUnique<image_fetcher::ImageFetcherImpl>(
base::MakeUnique<suggestions::ImageDecoderImpl>(),
profile->GetRequestContext()));
} }
...@@ -19,9 +19,11 @@ static_library("doodle") { ...@@ -19,9 +19,11 @@ static_library("doodle") {
"//base", "//base",
"//components/data_use_measurement/core", "//components/data_use_measurement/core",
"//components/google/core/browser", "//components/google/core/browser",
"//components/image_fetcher/core",
"//components/keyed_service/core", "//components/keyed_service/core",
"//components/prefs", "//components/prefs",
"//net", "//net",
"//ui/gfx",
] ]
} }
...@@ -36,8 +38,10 @@ source_set("unit_tests") { ...@@ -36,8 +38,10 @@ source_set("unit_tests") {
deps = [ deps = [
":doodle", ":doodle",
"//components/google/core/browser", "//components/google/core/browser",
"//components/image_fetcher/core",
"//components/prefs:test_support", "//components/prefs:test_support",
"//net:test_support", "//net:test_support",
"//ui/base", "//ui/base",
"//ui/gfx:test_support",
] ]
} }
include_rules = [ include_rules = [
"+components/data_use_measurement/core", "+components/data_use_measurement/core",
"+components/google/core/browser", "+components/google/core/browser",
"+components/image_fetcher/core",
"+components/keyed_service/core", "+components/keyed_service/core",
"+components/prefs", "+components/prefs",
"+net/base", "+net/base",
"+net/http/http_status_code.h", "+net/http/http_status_code.h",
"+net/traffic_annotation", "+net/traffic_annotation",
"+net/url_request", "+net/url_request",
"+ui/gfx",
] ]
...@@ -10,10 +10,14 @@ ...@@ -10,10 +10,14 @@
#include "base/memory/ptr_util.h" #include "base/memory/ptr_util.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "base/values.h" #include "base/values.h"
#include "components/data_use_measurement/core/data_use_user_data.h"
#include "components/doodle/pref_names.h" #include "components/doodle/pref_names.h"
#include "components/image_fetcher/core/image_fetcher.h"
#include "components/image_fetcher/core/request_metadata.h"
#include "components/prefs/pref_registry.h" #include "components/prefs/pref_registry.h"
#include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h" #include "components/prefs/pref_service.h"
#include "ui/gfx/image/image.h"
namespace doodle { namespace doodle {
...@@ -26,6 +30,11 @@ const int64_t kMaxTimeToLiveSecs = 30 * 24 * 60 * 60; // 30 days ...@@ -26,6 +30,11 @@ const int64_t kMaxTimeToLiveSecs = 30 * 24 * 60 * 60; // 30 days
// The default value for DoodleService::min_refresh_interval_. // The default value for DoodleService::min_refresh_interval_.
const int64_t kDefaultMinRefreshIntervalSecs = 15 * 60; // 15 minutes const int64_t kDefaultMinRefreshIntervalSecs = 15 * 60; // 15 minutes
// The maximum number of bytes to allow in a doodle image. This limits the
// downloaded data to an amount that real images are unlikely to exceed. It is a
// safeguard against server-side errors.
const int64_t kMaxImageDownloadBytes = 1024 * 1024;
} // namespace } // namespace
// static // static
...@@ -43,7 +52,8 @@ DoodleService::DoodleService( ...@@ -43,7 +52,8 @@ DoodleService::DoodleService(
std::unique_ptr<base::OneShotTimer> expiry_timer, std::unique_ptr<base::OneShotTimer> expiry_timer,
std::unique_ptr<base::Clock> clock, std::unique_ptr<base::Clock> clock,
std::unique_ptr<base::TickClock> tick_clock, std::unique_ptr<base::TickClock> tick_clock,
base::Optional<base::TimeDelta> override_min_refresh_interval) base::Optional<base::TimeDelta> override_min_refresh_interval,
std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher)
: pref_service_(pref_service), : pref_service_(pref_service),
fetcher_(std::move(fetcher)), fetcher_(std::move(fetcher)),
expiry_timer_(std::move(expiry_timer)), expiry_timer_(std::move(expiry_timer)),
...@@ -52,12 +62,18 @@ DoodleService::DoodleService( ...@@ -52,12 +62,18 @@ DoodleService::DoodleService(
min_refresh_interval_( min_refresh_interval_(
override_min_refresh_interval.has_value() override_min_refresh_interval.has_value()
? override_min_refresh_interval.value() ? override_min_refresh_interval.value()
: base::TimeDelta::FromSeconds(kDefaultMinRefreshIntervalSecs)) { : base::TimeDelta::FromSeconds(kDefaultMinRefreshIntervalSecs)),
image_fetcher_(std::move(image_fetcher)) {
DCHECK(pref_service_); DCHECK(pref_service_);
DCHECK(fetcher_); DCHECK(fetcher_);
DCHECK(expiry_timer_); DCHECK(expiry_timer_);
DCHECK(clock_); DCHECK(clock_);
DCHECK(tick_clock_); DCHECK(tick_clock_);
DCHECK(image_fetcher_);
image_fetcher_->SetImageDownloadLimit(kMaxImageDownloadBytes);
image_fetcher_->SetDataUseServiceName(
data_use_measurement::DataUseUserData::DOODLE);
base::Time expiry_date = base::Time::FromInternalValue( base::Time expiry_date = base::Time::FromInternalValue(
pref_service_->GetInt64(prefs::kCachedConfigExpiry)); pref_service_->GetInt64(prefs::kCachedConfigExpiry));
...@@ -78,6 +94,24 @@ DoodleService::~DoodleService() = default; ...@@ -78,6 +94,24 @@ DoodleService::~DoodleService() = default;
void DoodleService::Shutdown() {} void DoodleService::Shutdown() {}
void DoodleService::GetImage(const ImageCallback& callback) {
if (!cached_config_.has_value()) {
callback.Run(gfx::Image());
return;
}
// If there is a CTA image, that means the main image is animated. Show the
// non-animated CTA image first, and load the animated one only when the
// user requests it.
bool has_cta = cached_config_->large_cta_image.has_value();
const GURL& image_url = has_cta ? cached_config_->large_cta_image->url
: cached_config_->large_image.url;
image_fetcher_->StartOrQueueNetworkRequest(
image_url.spec(), image_url,
base::Bind(&DoodleService::ImageFetched, base::Unretained(this),
callback));
}
void DoodleService::AddObserver(Observer* observer) { void DoodleService::AddObserver(Observer* observer) {
observers_.AddObserver(observer); observers_.AddObserver(observer);
} }
...@@ -258,4 +292,21 @@ void DoodleService::DoodleExpired() { ...@@ -258,4 +292,21 @@ void DoodleService::DoodleExpired() {
HandleNewConfig(DoodleState::NO_DOODLE, base::TimeDelta(), base::nullopt); HandleNewConfig(DoodleState::NO_DOODLE, base::TimeDelta(), base::nullopt);
} }
void DoodleService::ImageFetched(
const ImageCallback& callback,
const std::string& id,
const gfx::Image& image,
const image_fetcher::RequestMetadata& metadata) {
if (image.IsEmpty()) {
DLOG(WARNING) << "Failed to download doodle image";
} else {
// TODO(treib): Rename this to "Doodle.*" after we've decided what to do
// about crbug.com/719513.
UMA_HISTOGRAM_BOOLEAN("NewTabPage.LogoImageDownloaded",
metadata.from_http_cache);
}
callback.Run(image);
}
} // namespace doodle } // namespace doodle
...@@ -6,7 +6,9 @@ ...@@ -6,7 +6,9 @@
#define COMPONENTS_DOODLE_DOODLE_SERVICE_H_ #define COMPONENTS_DOODLE_DOODLE_SERVICE_H_
#include <memory> #include <memory>
#include <string>
#include "base/callback_forward.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/observer_list.h" #include "base/observer_list.h"
#include "base/optional.h" #include "base/optional.h"
...@@ -21,6 +23,15 @@ ...@@ -21,6 +23,15 @@
class PrefRegistrySimple; class PrefRegistrySimple;
class PrefService; class PrefService;
namespace gfx {
class Image;
}
namespace image_fetcher {
class ImageFetcher;
struct RequestMetadata;
}
namespace doodle { namespace doodle {
class DoodleService : public KeyedService { class DoodleService : public KeyedService {
...@@ -31,6 +42,8 @@ class DoodleService : public KeyedService { ...@@ -31,6 +42,8 @@ class DoodleService : public KeyedService {
virtual void OnDoodleConfigUpdated(const base::Optional<DoodleConfig>&) = 0; virtual void OnDoodleConfigUpdated(const base::Optional<DoodleConfig>&) = 0;
}; };
using ImageCallback = base::Callback<void(const gfx::Image& image)>;
static void RegisterProfilePrefs(PrefRegistrySimple* pref_registry); static void RegisterProfilePrefs(PrefRegistrySimple* pref_registry);
// All pointer parameters must be non-null. If |min_refresh_interval| doesn't // All pointer parameters must be non-null. If |min_refresh_interval| doesn't
...@@ -40,7 +53,8 @@ class DoodleService : public KeyedService { ...@@ -40,7 +53,8 @@ class DoodleService : public KeyedService {
std::unique_ptr<base::OneShotTimer> expiry_timer, std::unique_ptr<base::OneShotTimer> expiry_timer,
std::unique_ptr<base::Clock> clock, std::unique_ptr<base::Clock> clock,
std::unique_ptr<base::TickClock> tick_clock, std::unique_ptr<base::TickClock> tick_clock,
base::Optional<base::TimeDelta> override_min_refresh_interval); base::Optional<base::TimeDelta> override_min_refresh_interval,
std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher);
~DoodleService() override; ~DoodleService() override;
// KeyedService implementation. // KeyedService implementation.
...@@ -49,6 +63,11 @@ class DoodleService : public KeyedService { ...@@ -49,6 +63,11 @@ class DoodleService : public KeyedService {
// Returns the current (cached) config, if any. // Returns the current (cached) config, if any.
const base::Optional<DoodleConfig>& config() const { return cached_config_; } const base::Optional<DoodleConfig>& config() const { return cached_config_; }
// Returns the image for the currently-cached doodle config via |callback|,
// which may be run synchronously or asynchronously. If the doodle is
// animated, this returns the static CTA image.
void GetImage(const ImageCallback& callback);
// Adds a new observer to the service. It'll only be called when the config // Adds a new observer to the service. It'll only be called when the config
// changes; to get the current (cached) config, call |config()|. // changes; to get the current (cached) config, call |config()|.
void AddObserver(Observer* observer); void AddObserver(Observer* observer);
...@@ -105,6 +124,11 @@ class DoodleService : public KeyedService { ...@@ -105,6 +124,11 @@ class DoodleService : public KeyedService {
// Callback for the expiry timer. // Callback for the expiry timer.
void DoodleExpired(); void DoodleExpired();
void ImageFetched(const ImageCallback& callback,
const std::string& id,
const gfx::Image& image,
const image_fetcher::RequestMetadata& metadata);
PrefService* pref_service_; PrefService* pref_service_;
// The fetcher for getting fresh DoodleConfigs from the network. // The fetcher for getting fresh DoodleConfigs from the network.
...@@ -118,6 +142,8 @@ class DoodleService : public KeyedService { ...@@ -118,6 +142,8 @@ class DoodleService : public KeyedService {
// refresh requests are ignored for this period. // refresh requests are ignored for this period.
const base::TimeDelta min_refresh_interval_; const base::TimeDelta min_refresh_interval_;
std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
// The result of the last network fetch. // The result of the last network fetch.
base::Optional<DoodleConfig> cached_config_; base::Optional<DoodleConfig> cached_config_;
......
...@@ -12,16 +12,26 @@ ...@@ -12,16 +12,26 @@
#include "base/memory/ptr_util.h" #include "base/memory/ptr_util.h"
#include "base/memory/ref_counted.h" #include "base/memory/ref_counted.h"
#include "base/test/histogram_tester.h" #include "base/test/histogram_tester.h"
#include "base/test/mock_callback.h"
#include "base/test/simple_test_tick_clock.h" #include "base/test/simple_test_tick_clock.h"
#include "base/test/test_mock_time_task_runner.h" #include "base/test/test_mock_time_task_runner.h"
#include "base/threading/thread_task_runner_handle.h" #include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h" #include "base/time/time.h"
#include "components/image_fetcher/core/image_fetcher.h"
#include "components/image_fetcher/core/request_metadata.h"
#include "components/prefs/testing_pref_service.h" #include "components/prefs/testing_pref_service.h"
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/geometry/size.h"
#include "ui/gfx/image/image.h"
#include "ui/gfx/image/image_unittest_util.h"
using image_fetcher::ImageFetcher;
using image_fetcher::RequestMetadata;
using testing::_;
using testing::Eq; using testing::Eq;
using testing::StrictMock; using testing::StrictMock;
using testing::Not;
namespace doodle { namespace doodle {
...@@ -60,10 +70,74 @@ class MockDoodleObserver : public DoodleService::Observer { ...@@ -60,10 +70,74 @@ class MockDoodleObserver : public DoodleService::Observer {
MOCK_METHOD1(OnDoodleConfigRevalidated, void(bool)); MOCK_METHOD1(OnDoodleConfigRevalidated, void(bool));
}; };
class FakeImageFetcher : public ImageFetcher {
public:
FakeImageFetcher() = default;
~FakeImageFetcher() override = default;
void SetImageFetcherDelegate(image_fetcher::ImageFetcherDelegate*) override {
NOTREACHED();
}
void SetDataUseServiceName(DataUseServiceName) override {
// Ignored.
}
void SetImageDownloadLimit(base::Optional<int64_t>) override {
// Ignored.
}
void SetDesiredImageFrameSize(const gfx::Size&) override {
// Ignored.
}
void StartOrQueueNetworkRequest(
const std::string& id,
const GURL& url,
const ImageFetcherCallback& callback) override {
// For simplicity, the fake doesn't support multiple concurrent requests.
DCHECK(!HasPendingRequest());
pending_id_ = id;
pending_url_ = url;
pending_callback_ = callback;
}
image_fetcher::ImageDecoder* GetImageDecoder() override {
NOTREACHED();
return nullptr;
}
bool HasPendingRequest() const { return !pending_callback_.is_null(); }
const GURL& pending_url() const { return pending_url_; }
void RespondToPendingRequest(const gfx::Image& image) {
DCHECK(HasPendingRequest());
RequestMetadata metadata;
metadata.http_response_code = 200;
pending_callback_.Run(pending_id_, image, metadata);
pending_id_.clear();
pending_url_ = GURL();
pending_callback_.Reset();
}
private:
std::string pending_id_;
GURL pending_url_;
ImageFetcherCallback pending_callback_;
};
DoodleConfig CreateConfig(DoodleType type) { DoodleConfig CreateConfig(DoodleType type) {
return DoodleConfig(type, DoodleImage(GURL("https://doodle.com/image.jpg"))); return DoodleConfig(type, DoodleImage(GURL("https://doodle.com/image.jpg")));
} }
MATCHER(IsEmptyImage, "") {
return arg.IsEmpty();
}
} // namespace } // namespace
class DoodleServiceTest : public testing::Test { class DoodleServiceTest : public testing::Test {
...@@ -83,7 +157,20 @@ class DoodleServiceTest : public testing::Test { ...@@ -83,7 +157,20 @@ class DoodleServiceTest : public testing::Test {
RecreateServiceWithZeroRefreshInterval(); RecreateServiceWithZeroRefreshInterval();
} }
void DestroyService() { service_ = nullptr; } void TearDown() override { DestroyService(); }
void DestroyService() {
if (image_fetcher_) {
// Make sure we didn't receive an unexpected image request.
ASSERT_FALSE(image_fetcher_->HasPendingRequest());
}
fetcher_ = nullptr;
expiry_timer_ = nullptr;
image_fetcher_ = nullptr;
service_ = nullptr;
}
void RecreateServiceWithZeroRefreshInterval() { void RecreateServiceWithZeroRefreshInterval() {
RecreateService(/*min_refresh_interval=*/base::TimeDelta()); RecreateService(/*min_refresh_interval=*/base::TimeDelta());
...@@ -97,14 +184,18 @@ class DoodleServiceTest : public testing::Test { ...@@ -97,14 +184,18 @@ class DoodleServiceTest : public testing::Test {
auto fetcher = base::MakeUnique<FakeDoodleFetcher>(); auto fetcher = base::MakeUnique<FakeDoodleFetcher>();
fetcher_ = fetcher.get(); fetcher_ = fetcher.get();
auto image_fetcher = base::MakeUnique<FakeImageFetcher>();
image_fetcher_ = image_fetcher.get();
service_ = base::MakeUnique<DoodleService>( service_ = base::MakeUnique<DoodleService>(
&pref_service_, std::move(fetcher), std::move(expiry_timer), &pref_service_, std::move(fetcher), std::move(expiry_timer),
task_runner_->GetMockClock(), task_runner_->GetMockTickClock(), task_runner_->GetMockClock(), task_runner_->GetMockTickClock(),
refresh_interval); refresh_interval, std::move(image_fetcher));
} }
DoodleService* service() { return service_.get(); } DoodleService* service() { return service_.get(); }
FakeDoodleFetcher* fetcher() { return fetcher_; } FakeDoodleFetcher* fetcher() { return fetcher_; }
FakeImageFetcher* image_fetcher() { return image_fetcher_; }
base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); } base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); }
...@@ -120,6 +211,7 @@ class DoodleServiceTest : public testing::Test { ...@@ -120,6 +211,7 @@ class DoodleServiceTest : public testing::Test {
// Weak, owned by the service. // Weak, owned by the service.
FakeDoodleFetcher* fetcher_; FakeDoodleFetcher* fetcher_;
base::OneShotTimer* expiry_timer_; base::OneShotTimer* expiry_timer_;
FakeImageFetcher* image_fetcher_;
}; };
TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) { TEST_F(DoodleServiceTest, FetchesConfigOnRefresh) {
...@@ -597,4 +689,55 @@ TEST_F(DoodleServiceTest, DoesNotRecordMetricsWhenConfigExpires) { ...@@ -597,4 +689,55 @@ TEST_F(DoodleServiceTest, DoesNotRecordMetricsWhenConfigExpires) {
histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0); histograms.ExpectTotalCount("Doodle.ConfigDownloadTime", 0);
} }
TEST_F(DoodleServiceTest, GetImageWithEmptyConfigReturnsImmediately) {
ASSERT_THAT(service()->config(), Eq(base::nullopt));
base::MockCallback<DoodleService::ImageCallback> callback;
EXPECT_CALL(callback, Run(IsEmptyImage()));
service()->GetImage(callback.Get());
}
TEST_F(DoodleServiceTest, GetImageFetchesLargeImage) {
service()->Refresh();
DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
base::TimeDelta::FromHours(1), config);
ASSERT_THAT(service()->config(), Eq(config));
base::MockCallback<DoodleService::ImageCallback> callback;
service()->GetImage(callback.Get());
EXPECT_EQ(config.large_image.url, image_fetcher()->pending_url());
EXPECT_CALL(callback, Run(Not(IsEmptyImage())));
gfx::Image image = gfx::test::CreateImage(1, 1);
ASSERT_TRUE(image_fetcher()->HasPendingRequest());
image_fetcher()->RespondToPendingRequest(image);
}
TEST_F(DoodleServiceTest, GetImageFetchesCTAImage) {
service()->Refresh();
DoodleConfig config = CreateConfig(DoodleType::SIMPLE);
// Set a CTA image, which should take precedence over the regular image.
config.large_image.is_animated_gif = true;
config.large_cta_image = DoodleImage(GURL("https://doodle.com/cta.jpg"));
config.large_cta_image->is_cta = true;
fetcher()->ServeAllCallbacks(DoodleState::AVAILABLE,
base::TimeDelta::FromHours(1), config);
ASSERT_THAT(service()->config(), Eq(config));
base::MockCallback<DoodleService::ImageCallback> callback;
service()->GetImage(callback.Get());
// If the doodle has a CTA image, that should loaded instead of the regular
// large image.
EXPECT_EQ(config.large_cta_image->url, image_fetcher()->pending_url());
EXPECT_CALL(callback, Run(Not(IsEmptyImage())));
gfx::Image image = gfx::test::CreateImage(1, 1);
ASSERT_TRUE(image_fetcher()->HasPendingRequest());
image_fetcher()->RespondToPendingRequest(image);
}
} // namespace doodle } // namespace doodle
...@@ -66,6 +66,8 @@ void ImageFetcherImpl::StartOrQueueNetworkRequest( ...@@ -66,6 +66,8 @@ void ImageFetcherImpl::StartOrQueueNetworkRequest(
base::Unretained(this), image_url)); base::Unretained(this), image_url));
} else { } else {
// Request in progress. Register as an interested callback. // Request in progress. Register as an interested callback.
// TODO(treib,markusheintz): We're not guaranteed that the ID also matches.
// We probably have to store them all.
it->second.callbacks.push_back(callback); it->second.callbacks.push_back(callback);
} }
} }
......
...@@ -30,7 +30,7 @@ class URLRequestContextGetter; ...@@ -30,7 +30,7 @@ class URLRequestContextGetter;
namespace image_fetcher { namespace image_fetcher {
// The standard (non-test) implementation of ImageFetcher. // The standard (non-test) implementation of ImageFetcher.
class ImageFetcherImpl : public image_fetcher::ImageFetcher { class ImageFetcherImpl : public ImageFetcher {
public: public:
ImageFetcherImpl(std::unique_ptr<ImageDecoder> image_decoder, ImageFetcherImpl(std::unique_ptr<ImageDecoder> image_decoder,
net::URLRequestContextGetter* url_request_context); net::URLRequestContextGetter* url_request_context);
......
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