Skip to content

Commit

Permalink
Don't use resolved url for instance identity in ApplicationManager
Browse files Browse the repository at this point in the history 
R=yzshen@chromium.org
http://crbug.com/533085

Committed: https://crrev.com/ce9f95edbf5b420aa0f7b0a522112751b547d215
Cr-Commit-Position: refs/heads/master@{#349541}

Review URL: https://codereview.chromium.org/1346143004

Cr-Commit-Position: refs/heads/master@{#349608}
  • Loading branch information
ben authored and Commit bot committed Sep 18, 2015
1 parent 95216c6 commit 79341ad
Show file tree
Hide file tree
Showing 8 changed files with 24 additions and 85 deletions.
15 changes: 7 additions & 8 deletions mojo/package_manager/package_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,6 @@ void PackageManagerImpl::SetApplicationManager(
application_manager_ = manager;
}

GURL PackageManagerImpl::ResolveURL(const GURL& url) {
return url_resolver_.get() ? url_resolver_->ResolveMojoURL(url) : url;
}

void PackageManagerImpl::FetchRequest(
URLRequestPtr request,
const shell::Fetcher::FetchCallback& loader_callback) {
Expand All @@ -75,7 +71,6 @@ void PackageManagerImpl::FetchRequest(
}

GURL resolved_url = ResolveURL(url);

if (resolved_url.SchemeIsFile()) {
// LocalFetcher uses the network service to infer MIME types from URLs.
// Skip this for mojo URLs to avoid recursively loading the network service.
Expand Down Expand Up @@ -121,7 +116,7 @@ void PackageManagerImpl::FetchRequest(
}

bool PackageManagerImpl::HandleWithContentHandler(shell::Fetcher* fetcher,
const GURL& unresolved_url,
const GURL& url,
base::TaskRunner* task_runner,
URLResponsePtr* new_response,
GURL* content_handler_url,
Expand Down Expand Up @@ -152,11 +147,11 @@ bool PackageManagerImpl::HandleWithContentHandler(shell::Fetcher* fetcher,
}

// The response URL matches a registered content handler.
auto alias_iter = application_package_alias_.find(unresolved_url);
auto alias_iter = application_package_alias_.find(url);
if (alias_iter != application_package_alias_.end()) {
// We replace the qualifier with the one our package alias requested.
*new_response = URLResponse::New();
(*new_response)->url = unresolved_url.spec();
(*new_response)->url = url.spec();

// Why can't we use this in single process mode? Because of
// base::AtExitManager. If you link in ApplicationRunner into your code, and
Expand All @@ -176,5 +171,9 @@ bool PackageManagerImpl::HandleWithContentHandler(shell::Fetcher* fetcher,
return false;
}

GURL PackageManagerImpl::ResolveURL(const GURL& url) {
return url_resolver_.get() ? url_resolver_->ResolveMojoURL(url) : url;
}

} // namespace package_manager
} // namespace mojo
4 changes: 2 additions & 2 deletions mojo/package_manager/package_manager_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ class PackageManagerImpl : public shell::PackageManager {

// Overridden from shell::PackageManager:
void SetApplicationManager(shell::ApplicationManager* manager) override;
GURL ResolveURL(const GURL& url) override;
void FetchRequest(
URLRequestPtr request,
const shell::Fetcher::FetchCallback& loader_callback) override;
bool HandleWithContentHandler(shell::Fetcher* fetcher,
const GURL& unresolved_url,
const GURL& url,
base::TaskRunner* task_runner,
URLResponsePtr* new_response,
GURL* content_handler_url,
std::string* qualifier) override;

GURL ResolveURL(const GURL& url);

shell::ApplicationManager* application_manager_;
scoped_ptr<fetcher::URLResolver> url_resolver_;
Expand Down
49 changes: 9 additions & 40 deletions mojo/shell/application_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,36 +73,23 @@ void ApplicationManager::TerminateShellConnections() {

void ApplicationManager::ConnectToApplication(
scoped_ptr<ConnectToApplicationParams> params) {
GURL original_url = params->app_url();
URLRequestPtr original_url_request = params->TakeAppURLRequest();

TRACE_EVENT_INSTANT1("mojo_shell", "ApplicationManager::ConnectToApplication",
TRACE_EVENT_SCOPE_THREAD, "original_url",
original_url.spec());
DCHECK(original_url.is_valid());
DCHECK(original_url_request);

// We need to look for running instances based on both the unresolved and
// resolved urls.
if (ConnectToRunningApplication(&params))
return;
params->app_url().spec());
DCHECK(params->app_url().is_valid());

GURL resolved_url = package_manager_->ResolveURL(original_url);
params->SetURLInfo(resolved_url);
// Connect to an existing matching instance, if possible.
if (ConnectToRunningApplication(&params))
return;

// The application is not running, let's compute the parameters.
// NOTE: Set URL info using |original_url_request| instead of |original_url|
// because it may contain more information (e.g., it is a POST request).
params->SetURLInfo(original_url_request.Pass());
ApplicationLoader* loader = GetLoaderForURL(resolved_url);
ApplicationLoader* loader = GetLoaderForURL(params->app_url());
if (loader) {
ConnectToApplicationWithLoader(&params, resolved_url, loader);
GURL url = params->app_url();
loader->Load(url, CreateInstance(params.Pass(), nullptr));
return;
}

original_url_request = params->TakeAppURLRequest();
URLRequestPtr original_url_request = params->TakeAppURLRequest();
auto callback =
base::Bind(&ApplicationManager::HandleFetchCallback,
weak_ptr_factory_.GetWeakPtr(), base::Passed(&params));
Expand All @@ -120,16 +107,6 @@ bool ApplicationManager::ConnectToRunningApplication(
return true;
}

void ApplicationManager::ConnectToApplicationWithLoader(
scoped_ptr<ConnectToApplicationParams>* params,
const GURL& resolved_url,
ApplicationLoader* loader) {
if (!(*params)->app_url().SchemeIs("mojo"))
(*params)->SetURLInfo(resolved_url);

loader->Load(resolved_url, CreateInstance(params->Pass(), nullptr));
}

InterfaceRequest<Application> ApplicationManager::CreateInstance(
scoped_ptr<ConnectToApplicationParams> params,
ApplicationInstance** resulting_instance) {
Expand All @@ -153,10 +130,8 @@ InterfaceRequest<Application> ApplicationManager::CreateInstance(

ApplicationInstance* ApplicationManager::GetApplicationInstance(
const Identity& identity) const {
const auto& instance_it = identity_to_instance_.find(identity);
if (instance_it != identity_to_instance_.end())
return instance_it->second;
return nullptr;
const auto& it = identity_to_instance_.find(identity);
return it != identity_to_instance_.end() ? it->second : nullptr;
}

void ApplicationManager::HandleFetchCallback(
Expand Down Expand Up @@ -186,15 +161,9 @@ void ApplicationManager::HandleFetchCallback(
// We already checked if the application was running before we fetched it, but
// it might have started while the fetch was outstanding. We don't want to
// have two copies of the app running, so check again.
//
// Also, it's possible the original URL was redirected to an app that is
// already running.
if (ConnectToRunningApplication(&params))
return;

if (params->app_url().scheme() != "mojo")
params->SetURLInfo(fetcher->GetURL());

Identity originator_identity = params->originator_identity();
CapabilityFilter originator_filter = params->originator_filter();
CapabilityFilter filter = params->filter();
Expand Down
8 changes: 0 additions & 8 deletions mojo/shell/application_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,6 @@ class ApplicationManager {
// Takes the contents of |params| only when it returns true.
bool ConnectToRunningApplication(
scoped_ptr<ConnectToApplicationParams>* params);
// |resolved_url| is the URL to load by |loader| (if loader is not null). It
// may be different from |(*params)->app_url()| because of mappings and
// resolution rules.
// Takes the contents of |params| only when it returns true.
void ConnectToApplicationWithLoader(
scoped_ptr<ConnectToApplicationParams>* params,
const GURL& resolved_url,
ApplicationLoader* loader);

InterfaceRequest<Application> CreateInstance(
scoped_ptr<ConnectToApplicationParams> params,
Expand Down
10 changes: 0 additions & 10 deletions mojo/shell/application_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,16 +474,6 @@ class AMTestPackageManager : public TestPackageManager {
void set_mime_type(const std::string& mime_type) { mime_type_ = mime_type; }

// TestPackageManager:
GURL ResolveURL(const GURL& url) override {
GURL resolved_url = url;
// The shell automatically map mojo URLs.
if (resolved_url.scheme() == "mojo") {
url::Replacements<char> replacements;
replacements.SetScheme("file", url::Component(0, 4));
resolved_url = resolved_url.ReplaceComponents(replacements);
}
return resolved_url;
}
void FetchRequest(URLRequestPtr request,
const Fetcher::FetchCallback& loader_callback) override {
if (create_test_fetcher_)
Expand Down
14 changes: 4 additions & 10 deletions mojo/shell/package_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace shell {
class ApplicationManager;

// A class implementing this interface assists Shell::ConnectToApplication in
// resolving URLs and fetching the applications located therein.
// fetching the applications located therein.
class PackageManager {
public:
PackageManager() {}
Expand All @@ -26,21 +26,15 @@ class PackageManager {
// associated ApplicationManager.
virtual void SetApplicationManager(ApplicationManager* manager) = 0;

// Resolves |url| to its canonical form. e.g. for mojo: urls, returns a file:
// url with a path ending in .mojo.
virtual GURL ResolveURL(const GURL& url) = 0;

// Asks the delegate to fetch the specified url. |url| must be unresolved,
// i.e. ResolveURL() above must not have been called on it.
// Asks the delegate to fetch the specified url.
// TODO(beng): figure out how not to expose Fetcher at all at this layer.
virtual void FetchRequest(
URLRequestPtr request,
const Fetcher::FetchCallback& loader_callback) = 0;

// Determine if a content handler should handle the response received by
// |fetcher|.
// |unresolved_url| is the url requested initially by a call to
// Shell::ConnectToApplication() before any resolution is applied.
// |url| is the url requested initially by a call to ConnectToApplication().
// |task_runner| is a base::TaskRunner* that can be used to post callbacks.
// |new_response| is the response that should be passed to
// ContentHandler::StartApplication(), unchanged if the return value is false.
Expand All @@ -49,7 +43,7 @@ class PackageManager {
// |qualifier| is the Identity qualifier that the content handler application
// instance should be associated with, unchanged if the return value is false.
virtual bool HandleWithContentHandler(Fetcher* fetcher,
const GURL& unresolved_url,
const GURL& url,
base::TaskRunner* task_runner,
URLResponsePtr* new_response,
GURL* content_handler_url,
Expand Down
6 changes: 1 addition & 5 deletions mojo/shell/test_package_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,13 @@ void TestPackageManager::RegisterContentHandler(
void TestPackageManager::SetApplicationManager(ApplicationManager* manager) {
}

GURL TestPackageManager::ResolveURL(const GURL& url) {
return url;
}

void TestPackageManager::FetchRequest(
URLRequestPtr request,
const Fetcher::FetchCallback& loader_callback) {}

bool TestPackageManager::HandleWithContentHandler(
Fetcher* fetcher,
const GURL& unresolved_url,
const GURL& url,
base::TaskRunner* task_runner,
URLResponsePtr* new_response,
GURL* content_handler_url,
Expand Down
3 changes: 1 addition & 2 deletions mojo/shell/test_package_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,11 @@ class TestPackageManager : public PackageManager {

// Overridden from PackageManager:
void SetApplicationManager(ApplicationManager* manager) override;
GURL ResolveURL(const GURL& url) override;
void FetchRequest(
URLRequestPtr request,
const Fetcher::FetchCallback& loader_callback) override;
bool HandleWithContentHandler(Fetcher* fetcher,
const GURL& unresolved_url,
const GURL& url,
base::TaskRunner* task_runner,
URLResponsePtr* new_response,
GURL* content_handler_url,
Expand Down

0 comments on commit 79341ad

Please sign in to comment.