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

MacPWAs: Decouple AppShimHost and AppShimHostBootstrap

Update AppShimHandler::OnShimLaunch to take a AppShimHostBootstrap,
and use that to create an AppShimHost, rather than having the
caller create the AppShimHost.
* Have the method for AppShimHost creation be in the
  AppShimHandler::Delegate class for overriding in tests.
* This sets us up to have the AppShimHost be created before the app shim
  process has completed launching (and thereby be able to queue up
  creating windows in the app shim process).

Add to AppShimHandler::Host's public interface an OnBootstrapConnected
function, where the AppShimHandler::Host takes ownership of the app
shim process.
* This is currently always called immediately after the AppShimHost's
  creation, but that will change in the next patch.

Make the tests rely more heavily on the real classes instead of fake
stubs. In particular, I plan to remove AppShimHandler::Host as an
abstract interface because the tests cover much more ground when run on
the real test (and the one real implementation, AppShimHost, has no
other public methods).

Make one subtle change in the tests. In the event of a duplicate app
launch, the app's windows will be focused by the existing AppShimHost,
because we don't create an AppShimHost in the event of any failure. This
has the same user-visible behavior (because the windows being focused
are the same).

Re-land of crrev.com/603821
TBR=dominickn

Bug: 896917
Change-Id: I4efc493d048e898d86aca736244a8e5a5672ccef
Reviewed-on: https://chromium-review.googlesource.com/c/1309222Reviewed-by: default avatarccameron <ccameron@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604153}
parent 350228ed
......@@ -5,6 +5,7 @@
#ifndef CHROME_BROWSER_APPS_APP_SHIM_APP_SHIM_HANDLER_MAC_H_
#define CHROME_BROWSER_APPS_APP_SHIM_APP_SHIM_HANDLER_MAC_H_
#include <memory>
#include <string>
#include <vector>
......@@ -15,14 +16,23 @@ namespace views {
class BridgeFactoryHost;
} // namespace views
class AppShimHostBootstrap;
namespace apps {
// Registrar, and interface for services that can handle interactions with OSX
// shim processes.
class AppShimHandler {
public:
// TODO(ccameron): Remove this virtual interface and always use AppShimHost
// directly.
// https://crbug.com/896917
class Host {
public:
// Invoked when the app shim process has finished launching. The |bootstrap|
// object owns the lifetime of the app shim process.
virtual void OnBootstrapConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) = 0;
// Invoked when the app is successfully launched.
virtual void OnAppLaunchComplete(AppShimLaunchResult result) = 0;
// Invoked when the app is closed in the browser process.
......@@ -75,9 +85,8 @@ class AppShimHandler {
// |launch_type| indicates the type of launch.
// |files|, if non-empty, holds an array of files paths given as arguments, or
// dragged onto the app bundle or dock icon.
virtual void OnShimLaunch(Host* host,
AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) = 0;
virtual void OnShimLaunch(
std::unique_ptr<AppShimHostBootstrap> bootstrap) = 0;
// Invoked by the shim host when the connection to the shim process is closed.
virtual void OnShimClose(Host* host) = 0;
......
......@@ -48,24 +48,15 @@ void AppShimHostBootstrap::ChannelError(uint32_t custom_reason,
delete this;
}
apps::AppShimHandler* AppShimHostBootstrap::GetHandler() {
return apps::AppShimHandler::GetForAppMode(app_id_);
}
chrome::mojom::AppShimHostRequest
AppShimHostBootstrap::GetLaunchAppShimHostRequest() {
return std::move(app_shim_host_request_);
}
apps::AppShimLaunchType AppShimHostBootstrap::GetLaunchType() const {
return launch_type_;
}
const std::vector<base::FilePath>& AppShimHostBootstrap::GetLaunchFiles()
const {
return files_;
}
apps::AppShimHandler::Host* AppShimHostBootstrap::GetHostForTesting() {
return connected_host_;
}
void AppShimHostBootstrap::LaunchApp(
chrome::mojom::AppShimHostRequest app_shim_host_request,
const base::FilePath& profile_dir,
......@@ -80,6 +71,8 @@ void AppShimHostBootstrap::LaunchApp(
return;
app_shim_host_request_ = std::move(app_shim_host_request);
profile_path_ = profile_dir;
app_id_ = app_id;
launch_type_ = launch_type;
files_ = files;
launch_app_callback_ = std::move(callback);
......@@ -90,13 +83,23 @@ void AppShimHostBootstrap::LaunchApp(
has_received_launch_app_ = true;
std::unique_ptr<AppShimHostBootstrap> deleter(this);
// |connected_host_| takes ownership of itself and |this|.
connected_host_ = new AppShimHost(app_id, profile_dir);
connected_host_->OnBootstrapConnected(std::move(deleter));
// |handler| takes ownership of |this| now.
apps::AppShimHandler* handler = GetHandler();
if (handler)
handler->OnShimLaunch(std::move(deleter));
// |handler| can only be NULL after AppShimHostManager is destroyed. Since
// this only happens at shutdown, do nothing here.
}
void AppShimHostBootstrap::OnLaunchAppComplete(
apps::AppShimLaunchResult result,
void AppShimHostBootstrap::OnLaunchAppSucceeded(
chrome::mojom::AppShimRequest app_shim_request) {
std::move(launch_app_callback_).Run(result, std::move(app_shim_request));
std::move(launch_app_callback_)
.Run(apps::APP_SHIM_LAUNCH_SUCCESS, std::move(app_shim_request));
}
void AppShimHostBootstrap::OnLaunchAppFailed(apps::AppShimLaunchResult result) {
// Because there will be users of the AppShim interface in failure, just
// return a dummy request.
chrome::mojom::AppShimPtr dummy_ptr;
std::move(launch_app_callback_).Run(result, mojo::MakeRequest(&dummy_ptr));
}
......@@ -25,19 +25,20 @@ class AppShimHostBootstrap : public chrome::mojom::AppShimHostBootstrap {
static void CreateForChannel(mojo::PlatformChannelEndpoint endpoint);
~AppShimHostBootstrap() override;
void OnLaunchAppComplete(apps::AppShimLaunchResult result,
chrome::mojom::AppShimRequest app_shim_request);
void OnLaunchAppSucceeded(chrome::mojom::AppShimRequest app_shim_request);
void OnLaunchAppFailed(apps::AppShimLaunchResult result);
chrome::mojom::AppShimHostRequest GetLaunchAppShimHostRequest();
apps::AppShimLaunchType GetLaunchType() const;
const std::vector<base::FilePath>& GetLaunchFiles() const;
apps::AppShimHandler::Host* GetHostForTesting();
const std::string& GetAppId() const { return app_id_; }
const base::FilePath& GetProfilePath() const { return profile_path_; }
apps::AppShimLaunchType GetLaunchType() const { return launch_type_; }
const std::vector<base::FilePath>& GetLaunchFiles() const { return files_; }
protected:
AppShimHostBootstrap();
void ServeChannel(mojo::PlatformChannelEndpoint endpoint);
void ChannelError(uint32_t custom_reason, const std::string& description);
virtual apps::AppShimHandler* GetHandler();
// chrome::mojom::AppShimHostBootstrap.
void LaunchApp(chrome::mojom::AppShimHostRequest app_shim_host_request,
......@@ -54,14 +55,12 @@ class AppShimHostBootstrap : public chrome::mojom::AppShimHostBootstrap {
// yet.
bool has_received_launch_app_ = false;
chrome::mojom::AppShimHostRequest app_shim_host_request_;
base::FilePath profile_path_;
std::string app_id_;
apps::AppShimLaunchType launch_type_;
std::vector<base::FilePath> files_;
LaunchAppCallback launch_app_callback_;
// The AppShimHost that has taken ownership of this object. Weak, set in
// LaunchApp.
AppShimHost* connected_host_ = nullptr;
THREAD_CHECKER(thread_checker_);
DISALLOW_COPY_AND_ASSIGN(AppShimHostBootstrap);
};
......
......@@ -58,9 +58,10 @@ AppShimHost::AppShimHost(const std::string& app_id,
AppShimHost::~AppShimHost() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
if (handler)
handler->OnShimClose(this);
// Ensure that we send a response to the bootstrap even if we failed to finish
// loading.
if (bootstrap_ && !has_sent_on_launch_complete_)
bootstrap_->OnLaunchAppFailed(apps::APP_SHIM_LAUNCH_APP_NOT_FOUND);
}
void AppShimHost::ChannelError(uint32_t custom_reason,
......@@ -72,9 +73,18 @@ void AppShimHost::ChannelError(uint32_t custom_reason,
void AppShimHost::Close() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
// Note that we must call GetAppShimHandler here and not in the destructor
// because some tests override the method.
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimClose(this);
delete this;
}
apps::AppShimHandler* AppShimHost::GetAppShimHandler() const {
return apps::AppShimHandler::GetForAppMode(app_id_);
}
////////////////////////////////////////////////////////////////////////////////
// AppShimHost, chrome::mojom::AppShimHost
......@@ -87,33 +97,26 @@ void AppShimHost::OnBootstrapConnected(
host_binding_.Bind(bootstrap_->GetLaunchAppShimHostRequest());
host_binding_.set_connection_error_with_reason_handler(
base::BindOnce(&AppShimHost::ChannelError, base::Unretained(this)));
apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
if (handler)
handler->OnShimLaunch(this, bootstrap_->GetLaunchType(),
bootstrap_->GetLaunchFiles());
// |handler| can only be NULL after AppShimHostManager is destroyed. Since
// this only happens at shutdown, do nothing here.
}
void AppShimHost::FocusApp(apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimFocus(this, focus_type, files);
}
void AppShimHost::SetAppHidden(bool hidden) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimSetHidden(this, hidden);
}
void AppShimHost::QuitApp() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
apps::AppShimHandler* handler = GetAppShimHandler();
if (handler)
handler->OnShimQuit(this);
}
......@@ -122,11 +125,13 @@ void AppShimHost::QuitApp() {
// AppShimHost, apps::AppShimHandler::Host
void AppShimHost::OnAppLaunchComplete(apps::AppShimLaunchResult result) {
if (!has_sent_on_launch_complete_) {
DCHECK(!has_sent_on_launch_complete_);
DCHECK(bootstrap_);
bootstrap_->OnLaunchAppComplete(result, std::move(app_shim_request_));
if (result == apps::APP_SHIM_LAUNCH_SUCCESS)
bootstrap_->OnLaunchAppSucceeded(std::move(app_shim_request_));
else
bootstrap_->OnLaunchAppFailed(result);
has_sent_on_launch_complete_ = true;
}
}
void AppShimHost::OnAppClosed() {
......
......@@ -33,32 +33,37 @@ class AppShimHost : public chrome::mojom::AppShimHost,
public apps::AppShimHandler::Host {
public:
AppShimHost(const std::string& app_id, const base::FilePath& profile_path);
~AppShimHost() override;
void OnBootstrapConnected(std::unique_ptr<AppShimHostBootstrap> bootstrap);
// apps::AppShimHandler::Host overrides:
void OnBootstrapConnected(
std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
void OnAppLaunchComplete(apps::AppShimLaunchResult result) override;
void OnAppClosed() override;
void OnAppHide() override;
void OnAppUnhideWithoutActivation() override;
void OnAppRequestUserAttention(apps::AppShimAttentionType type) override;
base::FilePath GetProfilePath() const override;
std::string GetAppId() const override;
views::BridgeFactoryHost* GetViewsBridgeFactoryHost() const override;
protected:
// AppShimHost is owned by itself. It will delete itself in Close (called on
// channel error and OnAppClosed).
~AppShimHost() override;
void ChannelError(uint32_t custom_reason, const std::string& description);
// Closes the channel and destroys the AppShimHost.
void Close();
// Return the AppShimHandler for this app (virtual for tests).
virtual apps::AppShimHandler* GetAppShimHandler() const;
// chrome::mojom::AppShimHost.
void FocusApp(apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) override;
void SetAppHidden(bool hidden) override;
void QuitApp() override;
// apps::AppShimHandler::Host overrides:
void OnAppLaunchComplete(apps::AppShimLaunchResult result) override;
void OnAppClosed() override;
void OnAppHide() override;
void OnAppUnhideWithoutActivation() override;
void OnAppRequestUserAttention(apps::AppShimAttentionType type) override;
base::FilePath GetProfilePath() const override;
std::string GetAppId() const override;
views::BridgeFactoryHost* GetViewsBridgeFactoryHost() const override;
mojo::Binding<chrome::mojom::AppShimHost> host_binding_;
chrome::mojom::AppShimPtr app_shim_;
chrome::mojom::AppShimRequest app_shim_request_;
......
......@@ -10,6 +10,7 @@
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/memory/weak_ptr.h"
#include "base/single_thread_task_runner.h"
#include "base/test/scoped_task_environment.h"
#include "base/test/test_simple_task_runner.h"
......@@ -62,6 +63,22 @@ class TestingAppShim : public chrome::mojom::AppShim {
DISALLOW_COPY_AND_ASSIGN(TestingAppShim);
};
class TestingAppShimHost : public AppShimHost {
public:
TestingAppShimHost(const std::string& app_id,
const base::FilePath& profile_path)
: AppShimHost(app_id, profile_path), test_weak_factory_(this) {}
base::WeakPtr<TestingAppShimHost> GetWeakPtr() {
return test_weak_factory_.GetWeakPtr();
}
private:
~TestingAppShimHost() override {}
base::WeakPtrFactory<TestingAppShimHost> test_weak_factory_;
DISALLOW_COPY_AND_ASSIGN(TestingAppShimHost);
};
class TestingAppShimHostBootstrap : public AppShimHostBootstrap {
public:
explicit TestingAppShimHostBootstrap(
......@@ -76,6 +93,8 @@ class TestingAppShimHostBootstrap : public AppShimHostBootstrap {
return test_weak_factory_.GetWeakPtr();
}
using AppShimHostBootstrap::LaunchApp;
private:
base::WeakPtrFactory<TestingAppShimHostBootstrap> test_weak_factory_;
DISALLOW_COPY_AND_ASSIGN(TestingAppShimHostBootstrap);
......@@ -91,7 +110,7 @@ class AppShimHostTest : public testing::Test,
~AppShimHostTest() override {
if (host_)
delete host_.get();
host_->OnAppClosed();
DCHECK(!host_);
}
......@@ -99,16 +118,15 @@ class AppShimHostTest : public testing::Test,
scoped_refptr<base::SingleThreadTaskRunner> task_runner() {
return task_runner_;
}
TestingAppShimHostBootstrap* host() { return host_.get(); }
chrome::mojom::AppShimHostBootstrap* GetBootstrapMojoHost() {
return host_.get();
}
apps::AppShimHandler::Host* host() { return host_.get(); }
chrome::mojom::AppShimHost* GetMojoHost() { return host_ptr_.get(); }
void LaunchApp(apps::AppShimLaunchType launch_type) {
GetBootstrapMojoHost()->LaunchApp(
mojo::MakeRequest(&host_ptr_), base::FilePath(kTestProfileDir),
kTestAppId, launch_type, std::vector<base::FilePath>(),
// Ownership of TestingAppShimHostBootstrap will be transferred to its host.
(new TestingAppShimHostBootstrap(shim_->GetHostBootstrapRequest()))
->LaunchApp(mojo::MakeRequest(&host_ptr_),
base::FilePath(kTestProfileDir), kTestAppId, launch_type,
std::vector<base::FilePath>(),
shim_->GetLaunchAppCallback());
}
......@@ -120,13 +138,17 @@ class AppShimHostTest : public testing::Test,
void SimulateDisconnect() { host_ptr_.reset(); }
protected:
void OnShimLaunch(Host* host,
apps::AppShimLaunchType launch_type,
const std::vector<base::FilePath>& file) override {
void OnShimLaunch(std::unique_ptr<AppShimHostBootstrap> bootstrap) override {
++launch_count_;
if (launch_type == apps::APP_SHIM_LAUNCH_NORMAL)
if (bootstrap->GetLaunchType() == apps::APP_SHIM_LAUNCH_NORMAL)
++launch_now_count_;
host->OnAppLaunchComplete(launch_result_);
// Maintain only a weak reference to |host_| because it is owned by itself
// and will delete itself upon closing.
host_ = (new TestingAppShimHost(bootstrap->GetAppId(),
bootstrap->GetProfilePath()))
->GetWeakPtr();
host_->OnBootstrapConnected(std::move(bootstrap));
host_->OnAppLaunchComplete(launch_result_);
}
void OnShimClose(Host* host) override { ++close_count_; }
......@@ -152,9 +174,6 @@ class AppShimHostTest : public testing::Test,
void SetUp() override {
testing::Test::SetUp();
shim_.reset(new TestingAppShim());
TestingAppShimHostBootstrap* host =
new TestingAppShimHostBootstrap(shim_->GetHostBootstrapRequest());
host_ = host->GetWeakPtr();
}
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
......@@ -162,9 +181,11 @@ class AppShimHostTest : public testing::Test,
std::unique_ptr<TestingAppShim> shim_;
std::unique_ptr<AppShimHostBootstrap> launched_bootstrap_;
// AppShimHost will destroy itself in AppShimHost::Close, so use a weak
// pointer here to avoid lifetime issues.
base::WeakPtr<TestingAppShimHostBootstrap> host_;
base::WeakPtr<TestingAppShimHost> host_;
chrome::mojom::AppShimHostPtr host_ptr_;
DISALLOW_COPY_AND_ASSIGN(AppShimHostTest);
......@@ -176,18 +197,13 @@ class AppShimHostTest : public testing::Test,
TEST_F(AppShimHostTest, TestLaunchAppWithHandler) {
apps::AppShimHandler::RegisterHandler(kTestAppId, this);
LaunchApp(apps::APP_SHIM_LAUNCH_NORMAL);
EXPECT_EQ(kTestAppId, host()->GetHostForTesting()->GetAppId());
EXPECT_EQ(kTestAppId, host()->GetAppId());
EXPECT_EQ(apps::APP_SHIM_LAUNCH_SUCCESS, GetLaunchResult());
EXPECT_EQ(1, launch_count_);
EXPECT_EQ(1, launch_now_count_);
EXPECT_EQ(0, focus_count_);
EXPECT_EQ(0, close_count_);
// A second OnAppLaunchComplete is ignored.
host()->GetHostForTesting()->OnAppLaunchComplete(
apps::APP_SHIM_LAUNCH_APP_NOT_FOUND);
EXPECT_EQ(apps::APP_SHIM_LAUNCH_SUCCESS, GetLaunchResult());
GetMojoHost()->FocusApp(apps::APP_SHIM_FOCUS_NORMAL,
std::vector<base::FilePath>());
RunUntilIdle();
......@@ -207,7 +223,7 @@ TEST_F(AppShimHostTest, TestLaunchAppWithHandler) {
TEST_F(AppShimHostTest, TestNoLaunchNow) {
apps::AppShimHandler::RegisterHandler(kTestAppId, this);
LaunchApp(apps::APP_SHIM_LAUNCH_REGISTER_ONLY);
EXPECT_EQ(kTestAppId, host()->GetHostForTesting()->GetAppId());
EXPECT_EQ(kTestAppId, host()->GetAppId());
EXPECT_EQ(apps::APP_SHIM_LAUNCH_SUCCESS, GetLaunchResult());
EXPECT_EQ(1, launch_count_);
EXPECT_EQ(0, launch_now_count_);
......
......@@ -13,6 +13,7 @@
#include "base/run_loop.h"
#include "base/threading/thread_restrictions.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/test/app_shim_host_manager_test_api_mac.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
......@@ -99,9 +100,10 @@ TestShimClient::TestShimClient()
// Browser Test for AppShimHostManager to test IPC interactions across the
// UNIX domain socket.
class AppShimHostManagerBrowserTest : public InProcessBrowserTest,
public apps::AppShimHandler {
public apps::AppShimHandler,
public chrome::mojom::AppShimHost {
public:
AppShimHostManagerBrowserTest() {}
AppShimHostManagerBrowserTest() : binding_(this) {}
protected:
// Wait for OnShimLaunch, then send a quit, and wait for the response. Used to
......@@ -113,23 +115,32 @@ class AppShimHostManagerBrowserTest : public InProcessBrowserTest,
void TearDownOnMainThread() override;
// AppShimHandler overrides:
void OnShimLaunch(apps::AppShimHandler::Host* host,
apps::AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) override;
void OnShimLaunch(std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
void OnShimClose(apps::AppShimHandler::Host* host) override {}
void OnShimFocus(apps::AppShimHandler::Host* host,
apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) override {}
void OnShimSetHidden(apps::AppShimHandler::Host* host, bool hidden) override {
}
void OnShimQuit(apps::AppShimHandler::Host* host) override;
void OnShimQuit(apps::AppShimHandler::Host* host) override {}
std::unique_ptr<TestShimClient> test_client_;
std::vector<base::FilePath> last_launch_files_;
apps::AppShimLaunchType last_launch_type_ = apps::APP_SHIM_LAUNCH_NUM_TYPES;
private:
// chrome::mojom::AppShimHost.
void FocusApp(apps::AppShimFocusType focus_type,
const std::vector<base::FilePath>& files) override {}
void SetAppHidden(bool hidden) override {}
void QuitApp() override {
++quit_count_;
runner_->Quit();
}
std::unique_ptr<base::RunLoop> runner_;
mojo::Binding<chrome::mojom::AppShimHost> binding_;
chrome::mojom::AppShimPtr app_shim_ptr_;
int launch_count_ = 0;
int quit_count_ = 0;
......@@ -146,7 +157,7 @@ void AppShimHostManagerBrowserTest::RunAndExitGracefully() {
runner_ = std::make_unique<base::RunLoop>();
test_client_->host()->QuitApp();
EXPECT_EQ(0, quit_count_);
runner_->Run(); // Will stop in OnShimQuit().
runner_->Run(); // Will stop in QuitApp().
EXPECT_EQ(1, quit_count_);
test_client_.reset();
......@@ -162,19 +173,13 @@ void AppShimHostManagerBrowserTest::TearDownOnMainThread() {
}
void AppShimHostManagerBrowserTest::OnShimLaunch(
apps::AppShimHandler::Host* host,
apps::AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) {
host->OnAppLaunchComplete(apps::APP_SHIM_LAUNCH_SUCCESS);
std::unique_ptr<AppShimHostBootstrap> bootstrap) {
++launch_count_;
last_launch_type_ = launch_type;
last_launch_files_ = files;
runner_->Quit();
}
binding_.Bind(bootstrap->GetLaunchAppShimHostRequest());
last_launch_type_ = bootstrap->GetLaunchType();
last_launch_files_ = bootstrap->GetLaunchFiles();
void AppShimHostManagerBrowserTest::OnShimQuit(
apps::AppShimHandler::Host* host) {
++quit_count_;
bootstrap->OnLaunchAppSucceeded(mojo::MakeRequest(&app_shim_ptr_));
runner_->Quit();
}
......
......@@ -3,6 +3,8 @@
// found in the LICENSE file.
#import <Cocoa/Cocoa.h>
#include <memory>
#include <utility>
#include <vector>
#include "apps/app_lifetime_monitor_factory.h"
......@@ -24,6 +26,7 @@
#include "base/threading/thread_restrictions.h"
#include "build/build_config.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_manager_mac.h"
#include "chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h"
#include "chrome/browser/apps/platform_apps/app_browsertest_util.h"
......@@ -95,13 +98,11 @@ class WindowedAppShimLaunchObserver : public apps::AppShimHandler {
}
// AppShimHandler overrides:
void OnShimLaunch(Host* host,
apps::AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) override {
void OnShimLaunch(std::unique_ptr<AppShimHostBootstrap> bootstrap) override {
// Remove self and pass through to the default handler.
apps::AppShimHandler::RemoveHandler(app_mode_id_);
apps::AppShimHandler::GetForAppMode(app_mode_id_)
->OnShimLaunch(host, launch_type, files);
->OnShimLaunch(std::move(bootstrap));
observed_ = true;
if (run_loop_.get())
run_loop_->Quit();
......
......@@ -8,7 +8,10 @@
#include <vector>
#include "apps/switches.h"
#include "base/bind.h"
#include "base/macros.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 "chrome/browser/apps/app_shim/app_shim_host_manager_mac.h"
#include "chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h"
#include "chrome/browser/apps/platform_apps/app_browsertest_util.h"
......@@ -28,32 +31,14 @@ namespace apps {
namespace {
class FakeHost : public apps::AppShimHandler::Host {
// Test class used to expose protected methods of AppShimHostBootstrap.
class TestAppShimHostBootstrap : public AppShimHostBootstrap {
public:
FakeHost(const base::FilePath& profile_path,
const std::string& app_id,
ExtensionAppShimHandler* handler)
: profile_path_(profile_path),
app_id_(app_id),
handler_(handler) {}
void OnAppLaunchComplete(AppShimLaunchResult result) override {}
void OnAppClosed() override { handler_->OnShimClose(this); }
void OnAppHide() override {}
void OnAppUnhideWithoutActivation() override {}
void OnAppRequestUserAttention(AppShimAttentionType type) override {}
base::FilePath GetProfilePath() const override { return profile_path_; }
std::string GetAppId() const override { return app_id_; }
views::BridgeFactoryHost* GetViewsBridgeFactoryHost() const override {
return nullptr;
}
TestAppShimHostBootstrap() {}
using AppShimHostBootstrap::LaunchApp;
private:
base::FilePath profile_path_;
std::string app_id_;
ExtensionAppShimHandler* handler_;
DISALLOW_COPY_AND_ASSIGN(FakeHost);
DISALLOW_COPY_AND_ASSIGN(TestAppShimHostBootstrap);
};
// Starts an app without a browser window using --load_and_launch_app and
......@@ -76,13 +61,14 @@ class AppShimQuitTest : public PlatformAppBrowserTest {
extensions::ExtensionRegistry::Get(profile());
extension_id_ =
GetExtensionByPath(registry->enabled_extensions(), app_path_)->id();
host_.reset(new FakeHost(profile()->GetPath().BaseName(),
extension_id_,
handler_));
handler_->OnShimLaunch(host_.get(),
chrome::mojom::AppShimHostPtr host_ptr;
(new TestAppShimHostBootstrap)
->LaunchApp(mojo::MakeRequest(&host_ptr),
profile()->GetPath().BaseName(), extension_id_,
APP_SHIM_LAUNCH_REGISTER_ONLY,
std::vector<base::FilePath>());
EXPECT_EQ(host_.get(), handler_->FindHost(profile(), extension_id_));
std::vector<base::FilePath>(),
base::BindOnce(&AppShimQuitTest::DoShimLaunchDone,
base::Unretained(this)));
// Focus the app window.
NSWindow* window = [[NSApp windows] objectAtIndex:0];
......@@ -90,6 +76,9 @@ class AppShimQuitTest : public PlatformAppBrowserTest {
content::RunAllPendingInMessageLoop();
}
void DoShimLaunchDone(apps::AppShimLaunchResult result,
chrome::mojom::AppShimRequest app_shim_request) {}
void SetUpCommandLine(base::CommandLine* command_line) override {
PlatformAppBrowserTest::SetUpCommandLine(command_line);
// Simulate an app shim initiated launch, i.e. launch app but not browser.
......@@ -104,7 +93,6 @@ class AppShimQuitTest : public PlatformAppBrowserTest {
base::FilePath app_path_;
ExtensionAppShimHandler* handler_;
std::string extension_id_;
std::unique_ptr<FakeHost> host_;
DISALLOW_COPY_AND_ASSIGN(AppShimQuitTest);
};
......
......@@ -17,9 +17,7 @@ class AppsPageShimHandler : public apps::AppShimHandler {
AppsPageShimHandler() {}
// AppShimHandler:
void OnShimLaunch(apps::AppShimHandler::Host* host,
apps::AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) override;
void OnShimLaunch(std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
void OnShimClose(apps::AppShimHandler::Host* host) override;
void OnShimFocus(apps::AppShimHandler::Host* host,
apps::AppShimFocusType focus_type,
......
......@@ -6,6 +6,7 @@
#import "base/mac/foundation_util.h"
#import "chrome/browser/app_controller_mac.h"
#include "chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_attributes_entry.h"
......@@ -53,15 +54,13 @@ void OpenAppsPage(Profile* fallback_profile) {
} // namespace
void AppsPageShimHandler::OnShimLaunch(
apps::AppShimHandler::Host* host,
apps::AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) {
std::unique_ptr<AppShimHostBootstrap> bootstrap) {
AppController* controller =
base::mac::ObjCCastStrict<AppController>([NSApp delegate]);
OpenAppsPage([controller lastProfile]);
// Always close the shim process immediately.
host->OnAppLaunchComplete(apps::APP_SHIM_LAUNCH_DUPLICATE_HOST);
bootstrap->OnLaunchAppFailed(apps::APP_SHIM_LAUNCH_DUPLICATE_HOST);
}
void AppsPageShimHandler::OnShimClose(apps::AppShimHandler::Host* host) {}
......
......@@ -8,9 +8,12 @@
#include "apps/app_lifetime_monitor_factory.h"
#include "apps/launcher.h"
#include "base/callback.h"
#include "base/files/file_path.h"
#include "base/logging.h"
#include "base/macros.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 "chrome/browser/apps/app_shim/app_shim_host_manager_mac.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chrome_notification_types.h"
......@@ -58,18 +61,6 @@ namespace {
typedef AppWindowRegistry::AppWindowList AppWindowList;
void ProfileLoadedCallback(base::Callback<void(Profile*)> callback,
Profile* profile,
Profile::CreateStatus status) {
if (status == Profile::CREATE_STATUS_INITIALIZED) {
callback.Run(profile);
return;
}
// This should never get an error since it only loads existing profiles.
DCHECK_EQ(Profile::CREATE_STATUS_CREATED, status);
}
void SetAppHidden(Profile* profile, const std::string& app_id, bool hidden) {
AppWindowList windows =
AppWindowRegistry::Get(profile)->GetAppWindowsForApp(app_id);
......@@ -183,13 +174,10 @@ Profile* ExtensionAppShimHandler::Delegate::ProfileForPath(
void ExtensionAppShimHandler::Delegate::LoadProfileAsync(
const base::FilePath& path,
base::Callback<void(Profile*)> callback) {
base::OnceCallback<void(Profile*)> callback) {
ProfileManager* profile_manager = g_browser_process->profile_manager();
base::FilePath full_path = profile_manager->user_data_dir().Append(path);
profile_manager->CreateProfileAsync(
full_path,
base::Bind(&ProfileLoadedCallback, callback),
base::string16(), std::string());
profile_manager->LoadProfileByPath(full_path, false, std::move(callback));
}
bool ExtensionAppShimHandler::Delegate::IsProfileLockedForPath(
......@@ -211,6 +199,12 @@ const Extension* ExtensionAppShimHandler::Delegate::MaybeGetAppExtension(
return ExtensionAppShimHandler::MaybeGetAppExtension(context, extension_id);
}
AppShimHandler::Host* ExtensionAppShimHandler::Delegate::CreateHost(
const std::string& app_id,
const base::FilePath& profile_path) {
return new AppShimHost(app_id, profile_path);
}
void ExtensionAppShimHandler::Delegate::EnableExtension(
Profile* profile,
const std::string& extension_id,
......@@ -418,13 +412,12 @@ void ExtensionAppShimHandler::OnChromeWillHide() {
}
void ExtensionAppShimHandler::OnShimLaunch(
Host* host,
AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) {
const std::string& app_id = host->GetAppId();
std::unique_ptr<AppShimHostBootstrap> bootstrap) {
const std::string& app_id = bootstrap->GetAppId();
AppShimLaunchType launch_type = bootstrap->GetLaunchType();
DCHECK(crx_file::id_util::IdIsValid(app_id));
const base::FilePath& profile_path = host->GetProfilePath();
const base::FilePath& profile_path = bootstrap->GetProfilePath();
DCHECK(!profile_path.empty());
if (!delegate_->ProfileExistsForPath(profile_path)) {
......@@ -432,33 +425,30 @@ void ExtensionAppShimHandler::OnShimLaunch(
// TODO(jackhou): Add some UI for this case and remove the LOG.
LOG(ERROR) << "Requested directory is not a known profile '"
<< profile_path.value() << "'.";
host->OnAppLaunchComplete(APP_SHIM_LAUNCH_PROFILE_NOT_FOUND);
bootstrap->OnLaunchAppFailed(APP_SHIM_LAUNCH_PROFILE_NOT_FOUND);
return;
}
if (delegate_->IsProfileLockedForPath(profile_path)) {
LOG(WARNING) << "Requested profile is locked. Showing User Manager.";
host->OnAppLaunchComplete(APP_SHIM_LAUNCH_PROFILE_LOCKED);
bootstrap->OnLaunchAppFailed(APP_SHIM_LAUNCH_PROFILE_LOCKED);
delegate_->LaunchUserManager();
return;
}
Profile* profile = delegate_->ProfileForPath(profile_path);
if (profile) {
OnProfileLoaded(host, launch_type, files, profile);
return;
}
OnProfileLoaded(std::move(bootstrap), profile);
} else {
// If the profile is not loaded, this must have been a launch by the shim.
// Load the profile asynchronously, the host will be registered in
// OnProfileLoaded.
DCHECK_EQ(APP_SHIM_LAUNCH_NORMAL, launch_type);
delegate_->LoadProfileAsync(
profile_path,
base::Bind(&ExtensionAppShimHandler::OnProfileLoaded,
weak_factory_.GetWeakPtr(),
host, launch_type, files));
base::BindOnce(&ExtensionAppShimHandler::OnProfileLoaded,
weak_factory_.GetWeakPtr(), std::move(bootstrap)));
}
// Return now. OnAppLaunchComplete will be called when the app is activated.
}
......@@ -503,23 +493,26 @@ void ExtensionAppShimHandler::CloseBrowsersForApp(const std::string& app_id) {
}
void ExtensionAppShimHandler::OnProfileLoaded(
Host* host,
AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files,
std::unique_ptr<AppShimHostBootstrap> bootstrap,
Profile* profile) {
const std::string& app_id = host->GetAppId();
const std::string& app_id = bootstrap->GetAppId();
AppShimLaunchType launch_type = bootstrap->GetLaunchType();
const std::vector<base::FilePath>& files = bootstrap->GetLaunchFiles();
// The first host to claim this (profile, app_id) becomes the main host.
// For any others, focus or relaunch the app.
if (!hosts_.insert(make_pair(make_pair(profile, app_id), host)).second) {
Host*& host = hosts_[make_pair(profile, app_id)];
if (!host) {
host = delegate_->CreateHost(app_id, bootstrap->GetProfilePath());
} else {
OnShimFocus(host,
launch_type == APP_SHIM_LAUNCH_NORMAL ?
APP_SHIM_FOCUS_REOPEN : APP_SHIM_FOCUS_NORMAL,
files);
host->OnAppLaunchComplete(APP_SHIM_LAUNCH_DUPLICATE_HOST);
bootstrap->OnLaunchAppFailed(APP_SHIM_LAUNCH_DUPLICATE_HOST);
return;
}
host->OnBootstrapConnected(std::move(bootstrap));
if (launch_type != APP_SHIM_LAUNCH_NORMAL) {
host->OnAppLaunchComplete(APP_SHIM_LAUNCH_SUCCESS);
return;
......
......@@ -12,6 +12,7 @@
#include <vector>
#include "apps/app_lifetime_monitor.h"
#include "base/callback_forward.h"
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
......@@ -52,7 +53,7 @@ class ExtensionAppShimHandler : public AppShimHandler,
virtual bool ProfileExistsForPath(const base::FilePath& path);
virtual Profile* ProfileForPath(const base::FilePath& path);
virtual void LoadProfileAsync(const base::FilePath& path,
base::Callback<void(Profile*)> callback);
base::OnceCallback<void(Profile*)> callback);
virtual bool IsProfileLockedForPath(const base::FilePath& path);
virtual extensions::AppWindowRegistry::AppWindowList GetWindows(
......@@ -62,6 +63,8 @@ class ExtensionAppShimHandler : public AppShimHandler,
virtual const extensions::Extension* MaybeGetAppExtension(
content::BrowserContext* context,
const std::string& extension_id);
virtual Host* CreateHost(const std::string& app_id,
const base::FilePath& profile_path);
virtual void EnableExtension(Profile* profile,
const std::string& extension_id,
const base::Callback<void()>& callback);
......@@ -119,9 +122,7 @@ class ExtensionAppShimHandler : public AppShimHandler,
void OnChromeWillHide();
// AppShimHandler overrides:
void OnShimLaunch(Host* host,
AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files) override;
void OnShimLaunch(std::unique_ptr<AppShimHostBootstrap> bootstrap) override;
void OnShimClose(Host* host) override;
void OnShimFocus(Host* host,
AppShimFocusType focus_type,
......@@ -173,9 +174,7 @@ class ExtensionAppShimHandler : public AppShimHandler,
// This is passed to Delegate::LoadProfileAsync for shim-initiated launches
// where the profile was not yet loaded.
void OnProfileLoaded(Host* host,
AppShimLaunchType launch_type,
const std::vector<base::FilePath>& files,
void OnProfileLoaded(std::unique_ptr<AppShimHostBootstrap> bootstrap,
Profile* profile);
// This is passed to Delegate::EnableViaPrompt for shim-initiated launches
......
......@@ -5,6 +5,7 @@
#include "extensions/browser/app_window/native_app_window.h"
#import <Cocoa/Cocoa.h>
#include <memory>
#import "base/mac/foundation_util.h"
#import "base/mac/mac_util.h"
......@@ -12,6 +13,8 @@
#import "base/mac/scoped_nsobject.h"
#import "base/mac/sdk_forward_declarations.h"
#include "base/macros.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 "chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h"
#include "chrome/browser/apps/app_shim/test/app_shim_host_manager_test_api_mac.h"
#include "chrome/browser/apps/platform_apps/app_browsertest_util.h"
......@@ -139,19 +142,19 @@ IN_PROC_BROWSER_TEST_F(NativeAppWindowCocoaBrowserTest, HideShowWithApp) {
namespace {
class MockAppShimHost : public apps::AppShimHandler::Host {
class MockAppShimHost : public AppShimHost {
public:
MockAppShimHost() {}
MockAppShimHost()
: AppShimHost("app", base::FilePath("Profile")), weak_factory_(this) {}
~MockAppShimHost() override {}
MOCK_METHOD1(OnAppLaunchComplete, void(apps::AppShimLaunchResult));
MOCK_METHOD0(OnAppClosed, void());
MOCK_METHOD0(OnAppHide, void());
MOCK_METHOD0(OnAppUnhideWithoutActivation, void());
MOCK_METHOD1(OnAppRequestUserAttention, void(apps::AppShimAttentionType));
MOCK_CONST_METHOD0(GetProfilePath, base::FilePath());
MOCK_CONST_METHOD0(GetAppId, std::string());
MOCK_CONST_METHOD0(GetViewsBridgeFactoryHost, views::BridgeFactoryHost*());
base::WeakPtr<MockAppShimHost> GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
private:
base::WeakPtrFactory<MockAppShimHost> weak_factory_;
};
class MockExtensionAppShimHandler : public apps::ExtensionAppShimHandler {
......@@ -176,7 +179,8 @@ IN_PROC_BROWSER_TEST_F(NativeAppWindowCocoaBrowserTest,
test_api.SetExtensionAppShimHandler(
std::unique_ptr<apps::ExtensionAppShimHandler>(
mock)); // Takes ownership.
MockAppShimHost mock_host;
base::WeakPtr<MockAppShimHost> mock_host =
(new MockAppShimHost)->GetWeakPtr();
SetUpAppWithWindows(1);
extensions::AppWindowRegistry::AppWindowList windows =
......@@ -191,24 +195,28 @@ IN_PROC_BROWSER_TEST_F(NativeAppWindowCocoaBrowserTest,
EXPECT_FALSE([ns_window isVisible]);
// Show notifies the shim to unhide.
EXPECT_CALL(mock_host, OnAppUnhideWithoutActivation());
EXPECT_CALL(*mock, FindHost(_, _)).WillOnce(Return(&mock_host));
EXPECT_CALL(*mock_host, OnAppUnhideWithoutActivation());
EXPECT_CALL(*mock, FindHost(_, _)).WillOnce(Return(mock_host.get()));
app_window->Show(extensions::AppWindow::SHOW_ACTIVE);
EXPECT_TRUE([ns_window isVisible]);
testing::Mock::VerifyAndClearExpectations(mock);
testing::Mock::VerifyAndClearExpectations(&mock_host);
testing::Mock::VerifyAndClearExpectations(mock_host.get());
// HideWithApp
native_window->HideWithApp();
EXPECT_FALSE([ns_window isVisible]);
// Activate does the same.
EXPECT_CALL(mock_host, OnAppUnhideWithoutActivation());
EXPECT_CALL(*mock, FindHost(_, _)).WillOnce(Return(&mock_host));
EXPECT_CALL(*mock_host, OnAppUnhideWithoutActivation());
EXPECT_CALL(*mock, FindHost(_, _)).WillOnce(Return(mock_host.get()));
native_window->Activate();
EXPECT_TRUE([ns_window isVisible]);
testing::Mock::VerifyAndClearExpectations(mock);
testing::Mock::VerifyAndClearExpectations(&mock_host);
testing::Mock::VerifyAndClearExpectations(mock_host.get());
// Ensure that the mock object be deleted.
mock_host->OnAppClosed();
DCHECK(!mock_host);
}
// Test that NativeAppWindow and AppWindow fullscreen state is updated when
......
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