Commit b9241eaa authored by Christopher Cameron's avatar Christopher Cameron Committed by Commit Bot

Separate out AppShimHost::Client interface from AppShimHandler

AppShimHost repeatedly queries AppShimHandler::Get to call out of
itself. In practice, these always go to the ExtensionAppShimHandler
that owns the AppShimHost.

Remove these functions from AppShimHandler and put them in the
AppShimHost::Client interface. Make the AppShimHost take a Client
at creation time (which is safe, now that AppShimHost is owned by
ExtensionAppShimHandler). Document that this is the interface between
AppShimHost and ExtensionAppShimHandler.

Bug: 982024
Change-Id: I954ce6f639596c4c930b701ed9770258852d0e58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1772667
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: default avatarDominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691037}
parent 4ad192a6
......@@ -14,14 +14,10 @@
#include "base/process/process.h"
#include "chrome/common/mac/app_shim_launch.h"
class AppShimHost;
class AppShimHostBootstrap;
namespace apps {
using ShimLaunchedCallback = base::OnceCallback<void(base::Process)>;
using ShimTerminatedCallback = base::OnceClosure;
// Registrar, and interface for services that can handle interactions with OSX
// shim processes.
class AppShimHandler {
......@@ -31,13 +27,6 @@ class AppShimHandler {
// Set or un-set the default handler.
static void Set(AppShimHandler* handler);
// Request that the handler launch the app shim process.
virtual void OnShimLaunchRequested(
AppShimHost* host,
bool recreate_shims,
apps::ShimLaunchedCallback launched_callback,
apps::ShimTerminatedCallback terminated_callback) = 0;
// Invoked by the AppShimHostBootstrap when a shim process has connected to
// the browser process. This will connect to (creating, if needed) an
// AppShimHost. |bootstrap| must have OnConnectedToHost or
......@@ -45,23 +34,6 @@ class AppShimHandler {
virtual void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) = 0;
// Invoked by the shim host when the connection to the shim process is closed.
// This is also called when we give up on trying to get a shim to connect.
virtual void OnShimProcessDisconnected(AppShimHost* host) = 0;
// Invoked by the shim host when the shim process receives a focus event.
// |files|, if non-empty, holds an array of files dragged onto the app bundle
// or dock icon.
virtual void OnShimFocus(AppShimHost* host,
AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) = 0;
// Invoked by the shim host when the shim process is hidden or shown.
virtual void OnShimSetHidden(AppShimHost* host, bool hidden) = 0;
// Invoked by the shim host when the shim process receives a quit event.
virtual void OnShimQuit(AppShimHost* host) = 0;
protected:
AppShimHandler() {}
virtual ~AppShimHandler() {}
......
......@@ -8,6 +8,7 @@
#include <utility>
#include "base/bind.h"
#include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
#include "mojo/public/cpp/system/message_pipe.h"
// static
......
......@@ -18,6 +18,10 @@
#include "mojo/public/cpp/platform/platform_channel_endpoint.h"
#include "mojo/public/cpp/system/isolated_connection.h"
namespace apps {
class AppShimHandler;
} // namespace apps
class AppShimHostBootstrap : public chrome::mojom::AppShimHostBootstrap {
public:
// Creates a new server-side mojo channel at |endpoint|, which contains a
......
......@@ -9,18 +9,20 @@
#include "base/bind.h"
#include "base/files/file_path.h"
#include "base/logging.h"
#include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
#include "chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h"
#include "chrome/browser/apps/app_shim/app_shim_host_mac.h"
#include "components/remote_cocoa/browser/application_host.h"
#include "components/remote_cocoa/common/application.mojom.h"
#include "content/public/browser/browser_thread.h"
#include "mojo/public/cpp/bindings/interface_request.h"
#include "ui/base/ui_base_features.h"
AppShimHost::AppShimHost(const std::string& app_id,
AppShimHost::AppShimHost(AppShimHost::Client* client,
const std::string& app_id,
const base::FilePath& profile_path,
bool uses_remote_views)
: host_binding_(this),
: client_(client),
host_binding_(this),
app_shim_request_(mojo::MakeRequest(&app_shim_)),
launch_shim_has_been_called_(false),
app_id_(app_id),
......@@ -51,22 +53,14 @@ void AppShimHost::ChannelError(uint32_t custom_reason,
<< " description: " << description;
// OnShimProcessDisconnected will delete |this|.
if (apps::AppShimHandler* handler = GetAppShimHandler())
handler->OnShimProcessDisconnected(this);
}
apps::AppShimHandler* AppShimHost::GetAppShimHandler() const {
return apps::AppShimHandler::Get();
client_->OnShimProcessDisconnected(this);
}
void AppShimHost::LaunchShimInternal(bool recreate_shims) {
DCHECK(launch_shim_has_been_called_);
DCHECK(!bootstrap_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (!handler)
return;
launch_weak_factory_.InvalidateWeakPtrs();
handler->OnShimLaunchRequested(
client_->OnShimLaunchRequested(
this, recreate_shims,
base::BindOnce(&AppShimHost::OnShimProcessLaunched,
launch_weak_factory_.GetWeakPtr(), recreate_shims),
......@@ -110,8 +104,7 @@ void AppShimHost::OnShimProcessTerminated(bool recreate_shims_requested) {
DLOG(ERROR) << "Failed to launch recreated shim, giving up.";
// OnShimProcessDisconnected will delete |this|.
if (apps::AppShimHandler* handler = GetAppShimHandler())
handler->OnShimProcessDisconnected(this);
client_->OnShimProcessDisconnected(this);
}
////////////////////////////////////////////////////////////////////////////////
......@@ -142,12 +135,9 @@ void AppShimHost::LaunchShim() {
return;
launch_shim_has_been_called_ = true;
apps::AppShimHandler* handler = GetAppShimHandler();
if (!handler)
return;
if (bootstrap_) {
// If there is a connected app shim process, focus the app windows.
handler->OnShimFocus(this, apps::APP_SHIM_FOCUS_NORMAL,
client_->OnShimFocus(this, apps::APP_SHIM_FOCUS_NORMAL,
std::vector<base::FilePath>());
} else {
// Otherwise, attempt to launch whatever app shims we find.
......@@ -158,23 +148,17 @@ void AppShimHost::LaunchShim() {
void AppShimHost::FocusApp(apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimFocus(this, focus_type, files);
client_->OnShimFocus(this, focus_type, files);
}
void AppShimHost::SetAppHidden(bool hidden) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimSetHidden(this, hidden);
client_->OnShimSetHidden(this, hidden);
}
void AppShimHost::QuitApp() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimQuit(this);
client_->OnShimQuit(this);
}
void AppShimHost::OnAppHide() {
......
......@@ -13,10 +13,14 @@
#include "base/memory/weak_ptr.h"
#include "base/process/process.h"
#include "base/threading/thread_checker.h"
#include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
#include "chrome/common/mac/app_shim.mojom.h"
#include "mojo/public/cpp/bindings/binding.h"
namespace apps {
using ShimLaunchedCallback = base::OnceCallback<void(base::Process)>;
using ShimTerminatedCallback = base::OnceClosure;
} // namespace apps
namespace remote_cocoa {
class ApplicationHost;
} // namespace remote_cocoa
......@@ -24,12 +28,45 @@ class ApplicationHost;
class AppShimHostBootstrap;
// This is the counterpart to AppShimController in
// chrome/app/chrome_main_app_mode_mac.mm. The AppShimHost owns itself, and is
// destroyed when the app it corresponds to is closed or when the channel
// connected to the app shim is closed.
// chrome/app/chrome_main_app_mode_mac.mm. The AppShimHost is owned by the
// ExtensionAppShimHandler, which implements its client interface.
class AppShimHost : public chrome::mojom::AppShimHost {
public:
AppShimHost(const std::string& app_id,
// The interface through which the AppShimHost interacts with
// ExtensionAppShimHandler.
class Client {
public:
// Request that the handler launch the app shim process.
virtual void OnShimLaunchRequested(
AppShimHost* host,
bool recreate_shims,
apps::ShimLaunchedCallback launched_callback,
apps::ShimTerminatedCallback terminated_callback) = 0;
// Invoked by the shim host when the connection to the shim process is
// closed. This is also called when we give up on trying to get a shim to
// connect.
virtual void OnShimProcessDisconnected(AppShimHost* host) = 0;
// Invoked by the shim host when the shim process receives a focus event.
// |files|, if non-empty, holds an array of files dragged onto the app
// bundle or dock icon.
virtual void OnShimFocus(AppShimHost* host,
apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) = 0;
// Invoked by the shim host when the shim process is hidden or shown. This
// is used only for non-RemoteCocoa.
virtual void OnShimSetHidden(AppShimHost* host, bool hidden) = 0;
// Invoked by the shim host when the shim process receives a quit event.
// This is used only for non-RemoteCocoa and could potentially be merged
// with OnShimProcessDisconnected.
virtual void OnShimQuit(AppShimHost* host) = 0;
};
AppShimHost(Client* client,
const std::string& app_id,
const base::FilePath& profile_path,
bool uses_remote_views);
......@@ -72,10 +109,6 @@ class AppShimHost : public chrome::mojom::AppShimHost {
// Return the app shim interface.
chrome::mojom::AppShim* GetAppShim() const;
protected:
// Return the AppShimHandler for this app (virtual for tests).
virtual apps::AppShimHandler* GetAppShimHandler() const;
private:
void ChannelError(uint32_t custom_reason, const std::string& description);
......@@ -96,6 +129,9 @@ class AppShimHost : public chrome::mojom::AppShimHost {
void SetAppHidden(bool hidden) override;
void QuitApp() override;
// Weak, owns |this|.
Client* const client_;
mojo::Binding<chrome::mojom::AppShimHost> host_binding_;
chrome::mojom::AppShimPtr app_shim_;
chrome::mojom::AppShimRequest app_shim_request_;
......
......@@ -17,6 +17,7 @@
#include "base/single_thread_task_runner.h"
#include "base/test/task_environment.h"
#include "base/test/test_simple_task_runner.h"
#include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
#include "chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h"
#include "chrome/common/mac/app_shim_param_traits.h"
#include "ipc/ipc_message.h"
......@@ -67,9 +68,13 @@ class TestingAppShim : public chrome::mojom::AppShim {
class TestingAppShimHost : public AppShimHost {
public:
TestingAppShimHost(const std::string& app_id,
TestingAppShimHost(Client* client,
const std::string& app_id,
const base::FilePath& profile_path)
: AppShimHost(app_id, profile_path, false /* uses_remote_views */) {}
: AppShimHost(client,
app_id,
profile_path,
false /* uses_remote_views */) {}
~TestingAppShimHost() override {}
private:
......@@ -101,7 +106,8 @@ const char kTestAppId[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
const char kTestProfileDir[] = "Profile 1";
class AppShimHostTest : public testing::Test,
public apps::AppShimHandler {
public apps::AppShimHandler,
public AppShimHost::Client {
public:
AppShimHostTest() { task_runner_ = base::ThreadTaskRunnerHandle::Get(); }
~AppShimHostTest() override {}
......@@ -130,17 +136,13 @@ class AppShimHostTest : public testing::Test,
void SimulateDisconnect() { host_ptr_.reset(); }
protected:
void OnShimLaunchRequested(
AppShimHost* host,
bool recreate_shims,
apps::ShimLaunchedCallback launched_callback,
apps::ShimTerminatedCallback terminated_callback) override {}
// AppShimHandler:
void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override {
++launch_count_;
if (bootstrap->GetLaunchType() == apps::APP_SHIM_LAUNCH_NORMAL)
++launch_now_count_;
host_ = std::make_unique<TestingAppShimHost>(bootstrap->GetAppId(),
host_ = std::make_unique<TestingAppShimHost>(this, bootstrap->GetAppId(),
bootstrap->GetProfilePath());
if (launch_result_ == apps::APP_SHIM_LAUNCH_SUCCESS)
host_->OnBootstrapConnected(std::move(bootstrap));
......@@ -148,20 +150,23 @@ class AppShimHostTest : public testing::Test,
bootstrap->OnFailedToConnectToHost(launch_result_);
}
// AppShimHost::Client:
void OnShimLaunchRequested(
AppShimHost* host,
bool recreate_shims,
apps::ShimLaunchedCallback launched_callback,
apps::ShimTerminatedCallback terminated_callback) override {}
void OnShimProcessDisconnected(AppShimHost* host) override {
DCHECK_EQ(host, host_.get());
host_ = nullptr;
++close_count_;
}
void OnShimFocus(AppShimHost* host,
apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& file) override {
++focus_count_;
}
void OnShimSetHidden(AppShimHost* host, bool hidden) override {}
void OnShimQuit(AppShimHost* host) override { ++quit_count_; }
apps::AppShimLaunchResult launch_result_ = apps::APP_SHIM_LAUNCH_SUCCESS;
......@@ -192,7 +197,6 @@ class AppShimHostTest : public testing::Test,
DISALLOW_COPY_AND_ASSIGN(AppShimHostTest);
};
} // namespace
TEST_F(AppShimHostTest, TestLaunchAppWithHandler) {
......
......@@ -73,7 +73,7 @@ class AppShimInteractiveTest : public extensions::PlatformAppBrowserTest {
};
// Watches for an app shim to connect.
class WindowedAppShimLaunchObserver : public apps::AppShimHandler {
class WindowedAppShimLaunchObserver : public apps::ExtensionAppShimHandler {
public:
WindowedAppShimLaunchObserver(const std::string& app_id)
: app_mode_id_(app_id),
......@@ -83,7 +83,6 @@ class WindowedAppShimLaunchObserver : public apps::AppShimHandler {
void StartObserving() {
observed_ = false;
default_app_shim_handler_ = apps::AppShimHandler::Get();
apps::AppShimHandler::Set(this);
}
......@@ -95,26 +94,24 @@ class WindowedAppShimLaunchObserver : public apps::AppShimHandler {
run_loop_->Run();
}
// AppShimHandler overrides:
// AppShimHandler:
void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override {
ExtensionAppShimHandler::OnShimProcessConnected(std::move(bootstrap));
observed_ = true;
if (run_loop_.get())
run_loop_->Quit();
}
// AppShimHost::Client:
void OnShimLaunchRequested(
AppShimHost* host,
bool recreate_shims,
apps::ShimLaunchedCallback launch_callback,
apps::ShimTerminatedCallback terminated_callback) override {
apps::AppShimHandler::Set(default_app_shim_handler_);
apps::AppShimHandler::Get()->OnShimLaunchRequested(
ExtensionAppShimHandler::OnShimLaunchRequested(
host, recreate_shims, std::move(launch_callback),
std::move(terminated_callback));
apps::AppShimHandler::Set(this);
}
void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override {
// Remove self and pass through to the default handler.
apps::AppShimHandler::Set(default_app_shim_handler_);
apps::AppShimHandler::Get()->OnShimProcessConnected(std::move(bootstrap));
observed_ = true;
if (run_loop_.get())
run_loop_->Quit();
}
void OnShimProcessDisconnected(AppShimHost* host) override {}
void OnShimFocus(AppShimHost* host,
......@@ -122,9 +119,7 @@ class WindowedAppShimLaunchObserver : public apps::AppShimHandler {
const std::vector<base::FilePath>& files) override {}
void OnShimSetHidden(AppShimHost* host, bool hidden) override {}
void OnShimQuit(AppShimHost* host) override {
// Remove self and pass through to the default handler.
apps::AppShimHandler::Set(default_app_shim_handler_);
apps::AppShimHandler::Get()->OnShimQuit(host);
ExtensionAppShimHandler::OnShimQuit(host);
observed_ = true;
if (run_loop_.get())
run_loop_->Quit();
......@@ -134,7 +129,6 @@ class WindowedAppShimLaunchObserver : public apps::AppShimHandler {
std::string app_mode_id_;
bool observed_;
std::unique_ptr<base::RunLoop> run_loop_;
apps::AppShimHandler* default_app_shim_handler_ = nullptr;
DISALLOW_COPY_AND_ASSIGN(WindowedAppShimLaunchObserver);
};
......
......@@ -120,19 +120,8 @@ class AppShimListenerBrowserTest : public InProcessBrowserTest,
void TearDownOnMainThread() override;
// AppShimHandler overrides:
void OnShimLaunchRequested(
::AppShimHost* host,
bool recreate_shims,
apps::ShimLaunchedCallback launched_callback,
apps::ShimTerminatedCallback terminated_callback) override {}
void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
void OnShimProcessDisconnected(::AppShimHost* host) override {}
void OnShimFocus(::AppShimHost* host,
apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) override {}
void OnShimSetHidden(::AppShimHost* host, bool hidden) override {}
void OnShimQuit(::AppShimHost* host) override {}
std::unique_ptr<TestShimClient> test_client_;
std::vector<base::FilePath> last_launch_files_;
......
......@@ -315,10 +315,11 @@ bool ExtensionAppShimHandler::Delegate::AllowShimToConnect(
}
std::unique_ptr<AppShimHost> ExtensionAppShimHandler::Delegate::CreateHost(
AppShimHost::Client* client,
Profile* profile,
const extensions::Extension* extension) {
return std::make_unique<AppShimHost>(extension->id(), profile->GetPath(),
UsesRemoteViews(extension));
return std::make_unique<AppShimHost>(
client, extension->id(), profile->GetPath(), UsesRemoteViews(extension));
}
void ExtensionAppShimHandler::Delegate::EnableExtension(
......@@ -427,7 +428,7 @@ AppShimHost* ExtensionAppShimHandler::FindOrCreateHost(
return nullptr;
ProfileState& profile_state = apps_[extension->id()].profiles[profile];
if (!profile_state.host)
profile_state.host = delegate_->CreateHost(profile, extension);
profile_state.host = delegate_->CreateHost(this, profile, extension);
return profile_state.host.get();
}
......
......@@ -16,6 +16,7 @@
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
#include "chrome/browser/apps/app_shim/app_shim_host_mac.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_list_observer.h"
#include "content/public/browser/notification_observer.h"
......@@ -42,6 +43,7 @@ namespace apps {
// This app shim handler that handles events for app shims that correspond to an
// extension.
class ExtensionAppShimHandler : public AppShimHandler,
public AppShimHost::Client,
public content::NotificationObserver,
public AppLifetimeMonitor::Observer,
public BrowserListObserver {
......@@ -68,6 +70,7 @@ class ExtensionAppShimHandler : public AppShimHandler,
virtual bool AllowShimToConnect(Profile* profile,
const extensions::Extension* extension);
virtual std::unique_ptr<AppShimHost> CreateHost(
AppShimHost::Client* client,
Profile* profile,
const extensions::Extension* extension);
virtual void EnableExtension(Profile* profile,
......@@ -136,14 +139,16 @@ class ExtensionAppShimHandler : public AppShimHandler,
// Called by AppControllerMac when Chrome hides.
void OnChromeWillHide();
// AppShimHandler overrides:
// AppShimHandler:
void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
// AppShimHost::Client:
void OnShimLaunchRequested(
AppShimHost* host,
bool recreate_shims,
base::OnceCallback<void(base::Process)> launched_callback,
base::OnceClosure terminated_callback) override;
void OnShimProcessConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
void OnShimProcessDisconnected(AppShimHost* host) override;
void OnShimFocus(AppShimHost* host,
AppShimFocusType focus_type,
......
......@@ -107,6 +107,7 @@ class MockDelegate : public ExtensionAppShimHandler::Delegate {
host_for_create_ = std::move(host_for_create);
}
std::unique_ptr<AppShimHost> CreateHost(
AppShimHost::Client* client,
Profile* profile,
const extensions::Extension* extension) override {
DCHECK(host_for_create_);
......@@ -221,14 +222,13 @@ class TestHost : public AppShimHost {
TestHost(const base::FilePath& profile_path,
const std::string& app_id,
TestingExtensionAppShimHandler* handler)
: AppShimHost(app_id, profile_path, false /* uses_remote_views */),
handler_(handler),
: AppShimHost(handler,
app_id,
profile_path,
false /* uses_remote_views */),
test_weak_factory_(this) {}
~TestHost() override {}
// Override the GetAppShimHandler for testing.
apps::AppShimHandler* GetAppShimHandler() const override { return handler_; }
// Record whether or not OnBootstrapConnected has been called.
void OnBootstrapConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override {
......@@ -243,7 +243,6 @@ class TestHost : public AppShimHost {
}
private:
TestingExtensionAppShimHandler* handler_;
bool did_connect_to_host_ = false;
base::WeakPtrFactory<TestHost> test_weak_factory_;
......
......@@ -150,8 +150,9 @@ namespace {
class MockAppShimHost : public AppShimHost {
public:
MockAppShimHost()
: AppShimHost("app",
MockAppShimHost(AppShimHost::Client* client)
: AppShimHost(client,
"app",
base::FilePath("Profile"),
false /* uses_remote_views */) {}
~MockAppShimHost() override {}
......@@ -184,7 +185,7 @@ IN_PROC_BROWSER_TEST_F(NativeAppWindowCocoaBrowserTest,
std::unique_ptr<apps::ExtensionAppShimHandler>(
mock)); // Takes ownership.
std::unique_ptr<MockAppShimHost> mock_host =
std::make_unique<MockAppShimHost>();
std::make_unique<MockAppShimHost>(mock);
SetUpAppWithWindows(1);
extensions::AppWindowRegistry::AppWindowList windows =
......
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