Commit 20a56d8f authored by Ce Chen's avatar Ce Chen Committed by Commit Bot

[omnibox] Add OnDeviceModelUpdateListener and update updater id.

The instance of this class is accessed as an NoDestructor by the model
component installer to notify the corresponding provider when model
update is finished, to replace the current polling mechanism over
DIR_ON_DEVICE_HEAD_SUGGEST in the provider.

Consequently remove DIR_ON_DEVICE_HEAD_SUGGEST since it is no longer
needed. May remove component updater from omnibox later if it is not
needed neither.

New component updater ID was previously approved in
https://crrev.com/c/1710827.

Tested on Nexus 5X for both new model download and pre installed model
discovery.

Note I cannot test on iOS, as looks like the Component needs to be
registered at ios/chrome/app/main_controller.mm rather than
chrome/browser/chrome_browser_main.cc for iOS (though I'm not sure
what is the best way to register the component in iOS as
chrome/browser/component_updater is currently not visible in
ios/chrome/app/). However if I register at main_controller.mm, my dev
app will crash at ComponentUnpacker::BeginUnzipping() when calling
unzipper_->Unzip(...) before ComponentUnpacker::EndUnzipping(), with
error http://screenshot/NqFrd299dnq and http://screenshot/g9RjyC34NaQ.

Bug: 925072
Change-Id: I99f0baf2bef23a7927847d93467f4be8878931dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1711030Reviewed-by: default avatarTommy Li <tommycli@chromium.org>
Reviewed-by: default avatarJoshua Pawlicki <waffles@chromium.org>
Commit-Queue: Ce Chen <cch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680561}
parent d59edfa8
......@@ -9,29 +9,23 @@
#include <string>
#include <vector>
#include "base/bind.h"
#include "base/files/file_util.h"
#include "base/path_service.h"
#include "base/strings/string_util.h"
#include "base/task/post_task.h"
#include "chrome/browser/browser_process.h"
#include "components/component_updater/component_installer.h"
#include "components/component_updater/component_updater_paths.h"
#include "components/component_updater/component_updater_service.h"
#include "components/omnibox/browser/on_device_model_update_listener.h"
#include "components/omnibox/common/omnibox_features.h"
namespace component_updater {
namespace {
// CRX hash. The extension id is: feejiaigafnbpeeogmhmjfmkcjplcneb.
// CRX hash. The extension id is: obedbbhbpmojnkanicioggnmelmoomoc.
const uint8_t kOnDeviceHeadSuggestPublicKeySHA256[32] = {
0x54, 0x49, 0x80, 0x86, 0x05, 0xd1, 0xf4, 0x4e, 0x6c, 0x7c, 0x95,
0xca, 0x29, 0xfb, 0x2d, 0x41, 0xe5, 0x12, 0x55, 0x83, 0x59, 0x58,
0x50, 0x41, 0x02, 0x6b, 0xb9, 0x06, 0x2c, 0xe0, 0xf8, 0x34};
void UpdateModelDirectory(const base::FilePath& file_path) {
base::PathService::Override(DIR_ON_DEVICE_HEAD_SUGGEST, file_path);
}
0xe1, 0x43, 0x11, 0x71, 0xfc, 0xe9, 0xda, 0x0d, 0x82, 0x8e, 0x66,
0xdc, 0x4b, 0xce, 0xec, 0xe2, 0xa3, 0xb0, 0x47, 0x00, 0x3d, 0xb8,
0xcf, 0x8e, 0x0f, 0x4a, 0x73, 0xa1, 0x89, 0x1f, 0x5f, 0x38};
// Normalizes and returns current application locale, i.e capitalizes all
// letters and removes all hyphens and underscores in the locale string,
......@@ -87,9 +81,9 @@ void OnDeviceHeadSuggestInstallerPolicy::ComponentReady(
const base::Version& version,
const base::FilePath& install_dir,
std::unique_ptr<base::DictionaryValue> manifest) {
base::PostTaskWithTraits(FROM_HERE,
{base::TaskPriority::BEST_EFFORT, base::MayBlock()},
base::BindOnce(&UpdateModelDirectory, install_dir));
auto* listener = OnDeviceModelUpdateListener::GetInstance();
if (listener)
listener->OnModelUpdate(install_dir);
}
base::FilePath OnDeviceHeadSuggestInstallerPolicy::GetRelativeInstallDir()
......
......@@ -32,7 +32,6 @@ enum {
DIR_RECOVERY_BASE, // The Recovery.
DIR_SWIFT_SHADER, // The SwiftShader.
DIR_SUPERVISED_USER_WHITELISTS, // The Supervised user whitelists.
DIR_ON_DEVICE_HEAD_SUGGEST, // The On Device Head Suggest model.
PATH_END
};
......
......@@ -169,6 +169,8 @@ jumbo_static_library("browser") {
"on_device_head_provider.h",
"on_device_head_serving.cc",
"on_device_head_serving.h",
"on_device_model_update_listener.cc",
"on_device_model_update_listener.h",
"remote_suggestions_service.cc",
"remote_suggestions_service.h",
"scored_history_match.cc",
......
......@@ -99,6 +99,8 @@ class AutocompleteProviderClient {
// tab.
virtual base::Time GetCurrentVisitTimestamp() const = 0;
// TODO(crbug/925072): clean up component update service if it's confirmed
// it's not needed for on device head provider.
// The component update service instance which will be used by on device
// suggestion provider to observe the model update event.
virtual component_updater::ComponentUpdateService*
......
......@@ -14,8 +14,6 @@
#include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h"
#include "base/trace_event/trace_event.h"
#include "build/build_config.h"
#include "components/component_updater/component_updater_paths.h"
#include "components/omnibox/browser/autocomplete_provider_listener.h"
#include "components/omnibox/browser/base_search_provider.h"
#include "components/omnibox/browser/on_device_head_provider.h"
......@@ -27,7 +25,6 @@
namespace {
const int kBaseRelevance = 99;
const size_t kMaxRequestId = std::numeric_limits<size_t>::max() - 1;
constexpr char kOnDeviceHeadComponentId[] = "feejiaigafnbpeeogmhmjfmkcjplcneb";
bool IsDefaultSearchProviderGoogle(
const TemplateURLService* template_url_service) {
......@@ -81,17 +78,13 @@ OnDeviceHeadProvider::OnDeviceHeadProvider(
listener_(listener),
serving_(nullptr),
task_runner_(base::SequencedTaskRunnerHandle::Get()),
on_device_search_request_id_(0),
observer_(this) {
if (client_ != nullptr) {
auto* component_update_service = client_->GetComponentUpdateService();
if (component_update_service != nullptr) {
observer_.Add(component_update_service);
}
on_device_search_request_id_(0) {
auto* model_update_listener = OnDeviceModelUpdateListener::GetInstance();
if (model_update_listener) {
model_update_subscription_ = model_update_listener->AddModelUpdateCallback(
base::BindRepeating(&OnDeviceHeadProvider::OnModelUpdate,
weak_ptr_factory_.GetWeakPtr()));
}
task_runner_->PostTask(
FROM_HERE, base::BindOnce(&OnDeviceHeadProvider::LoadPreInstalledModel,
weak_ptr_factory_.GetWeakPtr()));
}
OnDeviceHeadProvider::~OnDeviceHeadProvider() {
......@@ -129,6 +122,9 @@ void OnDeviceHeadProvider::Start(const AutocompleteInput& input,
if (minimal_changes)
return;
// Load the new model first before fulfilling the request if it's available.
MaybeResetServingInstanceFromNewModel();
matches_.clear();
if (!input.text().empty() && serving_) {
done_ = false;
......@@ -168,68 +164,21 @@ void OnDeviceHeadProvider::Stop(bool clear_cached_results,
done_ = true;
}
std::string OnDeviceHeadProvider::GetModelFilenameFromInstalledDirectory()
const {
// The model file name always ends with "_index.bin"
base::FileEnumerator model_enum(installed_directory_, false,
base::FileEnumerator::FILES,
FILE_PATH_LITERAL("*_index.bin"));
base::FilePath model_file_path = model_enum.Next();
std::string model_filename;
if (!model_file_path.empty()) {
#if defined(OS_WIN)
model_filename = base::WideToUTF8(model_file_path.value());
#else
model_filename = model_file_path.value();
#endif // defined(OS_WIN)
}
return model_filename;
}
// static
void OnDeviceHeadProvider::OverrideEnumDirOnDeviceHeadSuggestForTest(
const base::FilePath file_path) {
base::PathService::Override(component_updater::DIR_ON_DEVICE_HEAD_SUGGEST,
file_path);
}
// TODO(crbug/925072): Consider to add a listener here and in
// OnDeviceHeadSuggestInstallerPolicy::ComponentReady to load the on device
// model; see discussion at https://chromium-review.googlesource.com/1686677.
void OnDeviceHeadProvider::LoadPreInstalledModel() {
// Retry the loading for at most 3 times or until the model is successfully
// loaded.
for (int i = 3;;) {
CreateOnDeviceHeadServingInstance();
--i;
if (serving_ || i <= 0)
return;
base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(15));
}
}
void OnDeviceHeadProvider::DeleteInstalledDirectory() {
// TODO(crbug/925072): Maybe log event if deletion fails.
if (base::PathExists(installed_directory_))
base::DeleteFile(installed_directory_, true);
installed_directory_.clear();
void OnDeviceHeadProvider::OnModelUpdate(
const std::string& new_model_filename) {
if (new_model_filename != current_model_filename_ &&
new_model_filename_.empty())
new_model_filename_ = new_model_filename;
}
void OnDeviceHeadProvider::CreateOnDeviceHeadServingInstance() {
base::FilePath file_path;
base::PathService::Get(component_updater::DIR_ON_DEVICE_HEAD_SUGGEST,
&file_path);
void OnDeviceHeadProvider::MaybeResetServingInstanceFromNewModel() {
if (new_model_filename_.empty())
return;
if (!file_path.empty() && file_path != installed_directory_) {
DeleteInstalledDirectory();
installed_directory_ = file_path;
serving_ = OnDeviceHeadServing::Create(
GetModelFilenameFromInstalledDirectory(), provider_max_matches_);
}
serving_ =
OnDeviceHeadServing::Create(new_model_filename_, provider_max_matches_);
current_model_filename_ = new_model_filename_;
new_model_filename_.clear();
}
void OnDeviceHeadProvider::AddProviderInfo(ProvidersInfo* provider_info) const {
......@@ -296,16 +245,3 @@ void OnDeviceHeadProvider::SearchDone(
done_ = true;
listener_->OnProviderUpdate(true);
}
// We use component updater to fetch the on device model, which will fire
// Events::COMPONENT_UPDATED when the download is finished. In this case we will
// reload the new model and maybe clean up the old one.
void OnDeviceHeadProvider::OnEvent(Events event, const std::string& id) {
if (event != Events::COMPONENT_UPDATED || id != kOnDeviceHeadComponentId)
return;
task_runner_->PostTask(
FROM_HERE,
base::BindOnce(&OnDeviceHeadProvider::CreateOnDeviceHeadServingInstance,
weak_ptr_factory_.GetWeakPtr()));
}
......@@ -7,12 +7,12 @@
#include <memory>
#include "base/callback_list.h"
#include "base/files/file_path.h"
#include "base/scoped_observer.h"
#include "components/component_updater/component_updater_service.h"
#include "components/omnibox/browser/autocomplete_provider.h"
#include "components/omnibox/browser/autocomplete_provider_client.h"
#include "components/omnibox/browser/on_device_head_serving.h"
#include "components/omnibox/browser/on_device_model_update_listener.h"
class AutocompleteProviderListener;
......@@ -23,8 +23,7 @@ class AutocompleteProviderListener;
// greater than 99, such that its matches will not show before any other
// providers; However the relevance can be changed to any arbitrary value by
// Finch when the input is not classified as a URL.
class OnDeviceHeadProvider : public AutocompleteProvider,
public component_updater::ServiceObserver {
class OnDeviceHeadProvider : public AutocompleteProvider {
public:
static OnDeviceHeadProvider* Create(AutocompleteProviderClient* client,
AutocompleteProviderListener* listener);
......@@ -58,27 +57,13 @@ class OnDeviceHeadProvider : public AutocompleteProvider,
// fetches by DoSearch and then calls OnProviderUpdate.
void SearchDone(std::unique_ptr<OnDeviceHeadProviderParams> params);
// Helper function which finds the model and return its filename from the
// model installed directory.
std::string GetModelFilenameFromInstalledDirectory() const;
// Used by OnDeviceModelUpdateListener to notify this provider when new model
// is available.
void OnModelUpdate(const std::string& new_model_filename);
// Helper function only for unit tests to set the test model directory.
static void OverrideEnumDirOnDeviceHeadSuggestForTest(
const base::FilePath file_path);
// The function to load pre installed model from DIR_ON_DEVICE_HEAD_SUGGEST
// which will be called during provider's initialization.
void LoadPreInstalledModel();
// Clears up the directory which contains the current model.
void DeleteInstalledDirectory();
// Required by component_updater::ServiceObserver.
void OnEvent(Events event, const std::string& id) override;
// Creates the on device head serving service from a local head model, which
// can return up to |provider_max_matches_| suggestions.
void CreateOnDeviceHeadServingInstance();
// Resets |serving_| if new model is available and cleans up the old model if
// it exists.
void MaybeResetServingInstanceFromNewModel();
AutocompleteProviderClient* client_;
AutocompleteProviderListener* listener_;
......@@ -96,13 +81,17 @@ class OnDeviceHeadProvider : public AutocompleteProvider,
// AutocompleteController.
size_t on_device_search_request_id_;
// Tracks component update service for model update.
ScopedObserver<component_updater::ComponentUpdateService,
OnDeviceHeadProvider>
observer_;
// The filename for the on device model currently being used.
std::string current_model_filename_;
// The filename for the new model updated by Component Updater.
std::string new_model_filename_;
// The directory where the on device model and its manifest are installed.
base::FilePath installed_directory_;
// Owns the subscription after adding the model update callback to the
// listener such that the callback can be removed automatically from the
// listener on provider's deconstruction.
std::unique_ptr<OnDeviceModelUpdateListener::UpdateSubscription>
model_update_subscription_;
base::WeakPtrFactory<OnDeviceHeadProvider> weak_ptr_factory_{this};
......
......@@ -29,7 +29,7 @@ class OnDeviceHeadProviderTest : public testing::Test,
client_.reset(new FakeAutocompleteProviderClient());
SetTestOnDeviceHeadModel();
provider_ = OnDeviceHeadProvider::Create(client_.get(), this);
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
}
void TearDown() override {
......@@ -49,7 +49,18 @@ class OnDeviceHeadProviderTest : public testing::Test,
// The same test model also used in ./on_device_head_serving_unittest.cc.
file_path = file_path.AppendASCII("components/test/data/omnibox");
ASSERT_TRUE(base::PathExists(file_path));
OnDeviceHeadProvider::OverrideEnumDirOnDeviceHeadSuggestForTest(file_path);
auto* update_listener = OnDeviceModelUpdateListener::GetInstance();
if (update_listener)
update_listener->OnModelUpdate(file_path);
scoped_task_environment_.RunUntilIdle();
}
void ResetServingInstance() {
if (provider_) {
provider_->serving_.reset();
provider_->current_model_filename_.clear();
provider_->new_model_filename_.clear();
}
}
base::test::ScopedTaskEnvironment scoped_task_environment_;
......@@ -58,38 +69,39 @@ class OnDeviceHeadProviderTest : public testing::Test,
};
TEST_F(OnDeviceHeadProviderTest, ServingInstanceNotCreated) {
AutocompleteInput input(base::UTF8ToUTF16("a"),
AutocompleteInput input(base::UTF8ToUTF16("M"),
metrics::OmniboxEventProto::OTHER,
TestSchemeClassifier());
input.set_want_asynchronous_matches(true);
ResetServingInstance();
EXPECT_CALL(*client_.get(), IsOffTheRecord()).WillOnce(Return(false));
EXPECT_CALL(*client_.get(), SearchSuggestEnabled()).WillOnce(Return(true));
provider_->Start(input, false);
if (!provider_->done())
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(provider_->matches().empty());
EXPECT_TRUE(provider_->done());
}
TEST_F(OnDeviceHeadProviderTest, RejectSynchronousRequest) {
AutocompleteInput input(base::UTF8ToUTF16("a"),
AutocompleteInput input(base::UTF8ToUTF16("M"),
metrics::OmniboxEventProto::OTHER,
TestSchemeClassifier());
input.set_want_asynchronous_matches(false);
provider_->Start(input, false);
if (!provider_->done())
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(provider_->matches().empty());
EXPECT_TRUE(provider_->done());
}
TEST_F(OnDeviceHeadProviderTest, RejectIncognito) {
AutocompleteInput input(base::UTF8ToUTF16("a"),
AutocompleteInput input(base::UTF8ToUTF16("M"),
metrics::OmniboxEventProto::OTHER,
TestSchemeClassifier());
input.set_want_asynchronous_matches(true);
......@@ -98,7 +110,7 @@ TEST_F(OnDeviceHeadProviderTest, RejectIncognito) {
provider_->Start(input, false);
if (!provider_->done())
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(provider_->matches().empty());
EXPECT_TRUE(provider_->done());
......@@ -115,7 +127,7 @@ TEST_F(OnDeviceHeadProviderTest, NoMatches) {
provider_->Start(input, false);
if (!provider_->done())
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(provider_->matches().empty());
EXPECT_TRUE(provider_->done());
......@@ -132,7 +144,7 @@ TEST_F(OnDeviceHeadProviderTest, HasMatches) {
provider_->Start(input, false);
if (!provider_->done())
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(provider_->done());
ASSERT_EQ(3U, provider_->matches().size());
......@@ -160,7 +172,7 @@ TEST_F(OnDeviceHeadProviderTest, CancelInProgressRequest) {
provider_->Start(input2, false);
if (!provider_->done())
base::RunLoop().RunUntilIdle();
scoped_task_environment_.RunUntilIdle();
EXPECT_TRUE(provider_->done());
ASSERT_EQ(3U, provider_->matches().size());
......
// Copyright 2019 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 "components/omnibox/browser/on_device_model_update_listener.h"
#include "base/files/file_enumerator.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h"
#include "base/task_runner_util.h"
#include "build/build_config.h"
namespace {
// Helper function which finds the model and return its filename from the model
// directory.
std::string GetModelFilenameFromDirectory(const base::FilePath& model_dir) {
// The model file name always ends with "_index.bin"
base::FileEnumerator model_enum(model_dir, false, base::FileEnumerator::FILES,
FILE_PATH_LITERAL("*_index.bin"));
base::FilePath model_file_path = model_enum.Next();
std::string model_filename;
if (!model_file_path.empty()) {
#if defined(OS_WIN)
model_filename = base::WideToUTF8(model_file_path.value());
#else
model_filename = model_file_path.value();
#endif // defined(OS_WIN)
}
return model_filename;
}
} // namespace
// static
OnDeviceModelUpdateListener* OnDeviceModelUpdateListener::GetInstance() {
static base::NoDestructor<OnDeviceModelUpdateListener> listener;
return listener.get();
}
OnDeviceModelUpdateListener::OnDeviceModelUpdateListener()
: task_runner_(base::CreateSequencedTaskRunnerWithTraits(
{base::TaskPriority::BEST_EFFORT,
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN, base::MayBlock()})) {}
OnDeviceModelUpdateListener::~OnDeviceModelUpdateListener() = default;
std::unique_ptr<OnDeviceModelUpdateListener::UpdateSubscription>
OnDeviceModelUpdateListener::AddModelUpdateCallback(
ModelUpdateCallback callback) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
if (!model_filename_.empty())
callback.Run(model_filename_);
return model_update_callbacks_.Add(callback);
}
void OnDeviceModelUpdateListener::OnModelUpdate(
const base::FilePath& model_dir) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
if (!model_dir.empty() && model_dir != model_dir_) {
model_dir_ = model_dir;
base::PostTaskAndReplyWithResult(
task_runner_.get(), FROM_HERE,
base::BindOnce(&GetModelFilenameFromDirectory, model_dir),
base::BindOnce([](const std::string filename) {
if (!filename.empty()) {
GetInstance()->model_filename_ = filename;
GetInstance()->model_update_callbacks_.Notify(filename);
}
}));
}
}
void OnDeviceModelUpdateListener::ResetListenerForTest() {
model_dir_.clear();
model_filename_.clear();
}
// Copyright 2019 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 COMPONENTS_OMNIBOX_BROWSER_ON_DEVICE_MODEL_UPDATE_LISTENER_H_
#define COMPONENTS_OMNIBOX_BROWSER_ON_DEVICE_MODEL_UPDATE_LISTENER_H_
#include <memory>
#include "base/callback.h"
#include "base/callback_list.h"
#include "base/files/file_path.h"
#include "base/no_destructor.h"
#include "base/threading/thread_checker.h"
// This class is used by OnDeviceHeadSuggestComponentInstaller to notify
// OnDeviceHeadProvider when on device model update is finished.
class OnDeviceModelUpdateListener {
public:
using ModelUpdateCallback =
base::RepeatingCallback<void(const std::string& new_model_filename)>;
using UpdateCallbacks = base::CallbackList<void(const std::string&)>;
using UpdateSubscription = UpdateCallbacks::Subscription;
static OnDeviceModelUpdateListener* GetInstance();
// Adds a callback which will be run on model update. This method will also
// notify the provider immediately if a model is available.
std::unique_ptr<UpdateSubscription> AddModelUpdateCallback(
ModelUpdateCallback callback);
// Called by Component Updater when model update is completed to notify the
// on device head provider to reload the model.
void OnModelUpdate(const base::FilePath& model_dir);
std::string model_filename() const { return model_filename_; }
private:
friend class base::NoDestructor<OnDeviceModelUpdateListener>;
friend class OnDeviceHeadProviderTest;
void ResetListenerForTest();
OnDeviceModelUpdateListener();
~OnDeviceModelUpdateListener();
// The directory where the on device model resides.
base::FilePath model_dir_;
// The filename of the model.
std::string model_filename_;
// A list of callbacks which will be run on model update.
UpdateCallbacks model_update_callbacks_;
// The task runner which will be used to run file operations and
// |model_update_callbacks_|.
scoped_refptr<base::SequencedTaskRunner> task_runner_;
THREAD_CHECKER(thread_checker_);
DISALLOW_COPY_AND_ASSIGN(OnDeviceModelUpdateListener);
};
#endif // COMPONENTS_OMNIBOX_BROWSER_ON_DEVICE_MODEL_UPDATE_LISTENER_H_
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