Skip to content

Commit

Permalink
Migrate image service to use new download status rather than unified …
Browse files Browse the repository at this point in the history
…consent helper

Bug: 1425071
Change-Id: I2c3295147ea755b841d96bd67030fbd34f6d195e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4589410
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1153862}
  • Loading branch information
Sophie Chang authored and Chromium LUCI CQ committed Jun 6, 2023
1 parent 7860c6a commit 647ee3a
Show file tree
Hide file tree
Showing 13 changed files with 421 additions and 28 deletions.
8 changes: 7 additions & 1 deletion components/page_image_service/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ source_set("page_image_service") {
"features.h",
"image_service.cc",
"image_service.h",
"image_service_consent_helper.cc",
"image_service_consent_helper.h",
"image_service_handler.cc",
"image_service_handler.h",
"metrics_util.cc",
Expand All @@ -22,6 +24,7 @@ source_set("page_image_service") {
"//components/optimization_guide/core",
"//components/optimization_guide/proto:optimization_guide_proto",
"//components/search_engines",
"//components/sync/base",
"//components/sync/service",
"//components/unified_consent",
"//services/network/public/cpp",
Expand All @@ -31,7 +34,10 @@ source_set("page_image_service") {

source_set("unit_tests") {
testonly = true
sources = [ "image_service_unittest.cc" ]
sources = [
"image_service_consent_helper_unittest.cc",
"image_service_unittest.cc",
]
deps = [
":page_image_service",
"//base/test:test_support",
Expand Down
1 change: 1 addition & 0 deletions components/page_image_service/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ include_rules = [
"+components/keyed_service/core",
"+components/omnibox/browser",
"+components/search_engines",
"+components/sync/base",
"+components/sync/service",
"+components/sync/test",
"+components/optimization_guide",
Expand Down
6 changes: 6 additions & 0 deletions components/page_image_service/features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,10 @@ BASE_FEATURE(kImageServiceOptimizationGuideSalientImages,
"ImageServiceOptimizationGuideSalientImages",
base::FEATURE_ENABLED_BY_DEFAULT);

// Enables observing sync service for download status to determine whether
// images for already synced sync entities can be fetched.
BASE_FEATURE(kImageServiceObserveSyncDownloadStatus,
"ImageServiceObserveSyncDownloadStatus",
base::FEATURE_ENABLED_BY_DEFAULT);

} // namespace page_image_service
1 change: 1 addition & 0 deletions components/page_image_service/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace page_image_service {
BASE_DECLARE_FEATURE(kImageService);
BASE_DECLARE_FEATURE(kImageServiceSuggestPoweredImages);
BASE_DECLARE_FEATURE(kImageServiceOptimizationGuideSalientImages);
BASE_DECLARE_FEATURE(kImageServiceObserveSyncDownloadStatus);

} // namespace page_image_service

Expand Down
19 changes: 9 additions & 10 deletions components/page_image_service/image_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
#include "components/optimization_guide/proto/hints.pb.h"
#include "components/optimization_guide/proto/salient_image_metadata.pb.h"
#include "components/page_image_service/features.h"
#include "components/page_image_service/image_service_consent_helper.h"
#include "components/page_image_service/metrics_util.h"
#include "components/search_engines/search_engine_type.h"
#include "components/search_engines/template_url.h"
#include "components/search_engines/template_url_service.h"
#include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
#include "services/network/public/cpp/simple_url_loader.h"
#include "services/network/public/mojom/url_response_head.mojom.h"

Expand Down Expand Up @@ -175,13 +175,12 @@ ImageService::ImageService(
autocomplete_scheme_classifier)
: template_url_service_(template_url_service),
remote_suggestions_service_(remote_suggestions_service),
history_consent_throttle_(
unified_consent::UrlKeyedDataCollectionConsentHelper::
NewPersonalizedDataCollectionConsentHelper(sync_service)),
bookmarks_consent_throttle_(
unified_consent::UrlKeyedDataCollectionConsentHelper::
NewPersonalizedBookmarksDataCollectionConsentHelper(
sync_service)),
history_consent_helper_(std::make_unique<ImageServiceConsentHelper>(
sync_service,
syncer::ModelType::HISTORY_DELETE_DIRECTIVES)),
bookmarks_consent_helper_(std::make_unique<ImageServiceConsentHelper>(
sync_service,
syncer::ModelType::BOOKMARKS)),
autocomplete_scheme_classifier_(
std::move(autocomplete_scheme_classifier)) {
if (opt_guide && base::FeatureList::IsEnabled(
Expand Down Expand Up @@ -224,13 +223,13 @@ void ImageService::GetConsentToFetchImage(
case mojom::ClientId::Journeys:
case mojom::ClientId::JourneysSidePanel:
case mojom::ClientId::NtpQuests: {
return history_consent_throttle_.EnqueueRequest(std::move(callback));
return history_consent_helper_->EnqueueRequest(std::move(callback));
}
case mojom::ClientId::NtpRealbox:
// TODO(b/244507194): Figure out consent story for NTP realbox case.
return std::move(callback).Run(false);
case mojom::ClientId::Bookmarks: {
return bookmarks_consent_throttle_.EnqueueRequest(std::move(callback));
return bookmarks_consent_helper_->EnqueueRequest(std::move(callback));
}
}
}
Expand Down
7 changes: 4 additions & 3 deletions components/page_image_service/image_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "components/optimization_guide/core/optimization_guide_decision.h"
#include "components/page_image_service/mojom/page_image_service.mojom.h"
#include "components/sync/service/sync_service.h"
#include "components/unified_consent/consent_throttle.h"

class AutocompleteSchemeClassifier;
class RemoteSuggestionsService;
Expand All @@ -31,6 +30,8 @@ class NewOptimizationGuideDecider;

namespace page_image_service {

class ImageServiceConsentHelper;

// Through my manual testing, 16ms (which is about a frame at 60hz) allowed
// for decent aggregation without introducing any perceptible lag.
constexpr base::TimeDelta kOptimizationGuideBatchingTimeout =
Expand Down Expand Up @@ -131,10 +132,10 @@ class ImageService : public KeyedService {
raw_ptr<optimization_guide::NewOptimizationGuideDecider> opt_guide_ = nullptr;

// The History consent throttle, used for most clients.
unified_consent::ConsentThrottle history_consent_throttle_;
std::unique_ptr<ImageServiceConsentHelper> history_consent_helper_;

// The Bookmarks consent throttle.
unified_consent::ConsentThrottle bookmarks_consent_throttle_;
std::unique_ptr<ImageServiceConsentHelper> bookmarks_consent_helper_;

// Used to make proper suggest requests.
std::unique_ptr<AutocompleteSchemeClassifier> autocomplete_scheme_classifier_;
Expand Down
116 changes: 116 additions & 0 deletions components/page_image_service/image_service_consent_helper.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/page_image_service/image_service_consent_helper.h"

#include "base/feature_list.h"
#include "components/page_image_service/features.h"
#include "components/sync/service/sync_service.h"
#include "components/unified_consent/consent_throttle.h"
#include "components/unified_consent/url_keyed_data_collection_consent_helper.h"

namespace page_image_service {

namespace {

constexpr base::TimeDelta kTimeout = base::Seconds(10);

} // namespace

ImageServiceConsentHelper::ImageServiceConsentHelper(
syncer::SyncService* sync_service,
syncer::ModelType model_type)
: sync_service_(sync_service), model_type_(model_type) {
if (base::FeatureList::IsEnabled(kImageServiceObserveSyncDownloadStatus)) {
sync_service_observer_.Observe(sync_service);
} else if (model_type == syncer::ModelType::BOOKMARKS) {
consent_throttle_ = std::make_unique<unified_consent::ConsentThrottle>(
unified_consent::UrlKeyedDataCollectionConsentHelper::
NewPersonalizedBookmarksDataCollectionConsentHelper(sync_service),
kTimeout);
} else if (model_type == syncer::ModelType::HISTORY_DELETE_DIRECTIVES) {
consent_throttle_ = std::make_unique<unified_consent::ConsentThrottle>(
unified_consent::UrlKeyedDataCollectionConsentHelper::
NewPersonalizedDataCollectionConsentHelper(sync_service),
kTimeout);
} else {
NOTREACHED();
}
}

ImageServiceConsentHelper::~ImageServiceConsentHelper() = default;

void ImageServiceConsentHelper::EnqueueRequest(
base::OnceCallback<void(bool)> callback) {
if (consent_throttle_) {
consent_throttle_->EnqueueRequest(std::move(callback));
return;
}

absl::optional<bool> consent_status = GetConsentStatus();
if (consent_status.has_value()) {
std::move(callback).Run(*consent_status);
return;
}

enqueued_request_callbacks_.emplace_back(std::move(callback));
if (!request_processing_timer_.IsRunning()) {
request_processing_timer_.Start(
FROM_HERE, kTimeout,
base::BindOnce(
&ImageServiceConsentHelper::OnTimeoutExpired,
// Unretained usage here okay, because this object owns the timer.
base::Unretained(this)));
}
}

void ImageServiceConsentHelper::OnStateChanged(
syncer::SyncService* sync_service) {
CHECK_EQ(sync_service_, sync_service);
CHECK(base::FeatureList::IsEnabled(kImageServiceObserveSyncDownloadStatus));

absl::optional<bool> consent_status = GetConsentStatus();
if (!consent_status.has_value()) {
return;
}

for (auto& request_callback : enqueued_request_callbacks_) {
std::move(request_callback).Run(*consent_status);
}

enqueued_request_callbacks_.clear();
request_processing_timer_.Stop();
}

void ImageServiceConsentHelper::OnSyncShutdown(
syncer::SyncService* sync_service) {
CHECK_EQ(sync_service_, sync_service);
CHECK(base::FeatureList::IsEnabled(kImageServiceObserveSyncDownloadStatus));

sync_service_observer_.Reset();
}

absl::optional<bool> ImageServiceConsentHelper::GetConsentStatus() {
CHECK(base::FeatureList::IsEnabled(kImageServiceObserveSyncDownloadStatus));

syncer::SyncService::ModelTypeDownloadStatus download_status =
sync_service_->GetDownloadStatusFor(model_type_);
switch (download_status) {
case syncer::SyncService::ModelTypeDownloadStatus::kWaitingForUpdates:
return absl::nullopt;
case syncer::SyncService::ModelTypeDownloadStatus::kUpToDate:
return true;
case syncer::SyncService::ModelTypeDownloadStatus::kError:
return false;
}
}

void ImageServiceConsentHelper::OnTimeoutExpired() {
for (auto& request_callback : enqueued_request_callbacks_) {
std::move(request_callback).Run(false);
}
enqueued_request_callbacks_.clear();
}

} // namespace page_image_service
75 changes: 75 additions & 0 deletions components/page_image_service/image_service_consent_helper.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef COMPONENTS_PAGE_IMAGE_SERVICE_IMAGE_SERVICE_CONSENT_HELPER_H_
#define COMPONENTS_PAGE_IMAGE_SERVICE_IMAGE_SERVICE_CONSENT_HELPER_H_

#include <vector>

#include "base/memory/raw_ptr.h"
#include "base/scoped_observation.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "components/sync/base/model_type.h"
#include "components/sync/service/sync_service_observer.h"

namespace syncer {
class SyncService;
} // namespace syncer

namespace unified_consent {
class ConsentThrottle;
} // namespace unified_consent

namespace page_image_service {

// Helper class that observes SyncService for when it is appropriate to fetch
// images for synced entities that have been viewed in the past.
class ImageServiceConsentHelper : public syncer::SyncServiceObserver {
public:
ImageServiceConsentHelper(syncer::SyncService* sync_service,
syncer::ModelType model_type);
~ImageServiceConsentHelper() override;

// SyncServiceObserver:
void OnStateChanged(syncer::SyncService* sync_service) override;
void OnSyncShutdown(syncer::SyncService* sync_service) override;

// If Sync downloads for `model_type_` have already been initialized, this
// method calls `callback` synchronously with the result. If not, it will hold
// the request up until the timeout for the consent helper to initialize.
void EnqueueRequest(base::OnceCallback<void(bool)> callback);

private:
// Returns whether it is appropriate to fetch images for synced entities of
// `model_type_`. Will return nullopt if Sync Service is not ready yet.
absl::optional<bool> GetConsentStatus();

// Run periodically to sweep away old queued requests.
void OnTimeoutExpired();

// The sync service `this` is observing.
raw_ptr<syncer::SyncService> sync_service_;

// The model type `this` pertains to.
const syncer::ModelType model_type_;

// Requests waiting for the consent throttle to initialize. Requests are
// stored in the queue in order of their arrival.
std::vector<base::OnceCallback<void(bool)>> enqueued_request_callbacks_;

// Timer used to periodically process unanswered enqueued requests, and
// respond to them in the negative.
base::OneShotTimer request_processing_timer_;

// Consent throttle to be used if sync service is not being directly observed.
std::unique_ptr<unified_consent::ConsentThrottle> consent_throttle_;

base::ScopedObservation<syncer::SyncService, syncer::SyncServiceObserver>
sync_service_observer_{this};
};

} // namespace page_image_service

#endif // COMPONENTS_PAGE_IMAGE_SERVICE_IMAGE_SERVICE_CONSENT_HELPER_H_

0 comments on commit 647ee3a

Please sign in to comment.