Skip to content

Commit

Permalink
Reland "[DevUI DFM] Change install flow to use NavigationThrottle."
Browse files Browse the repository at this point in the history
This reverts commit 168860e.

Reason for reland: Fixed problem: The target source_set("dev_ui_loader")
had missing dependencies, and doesn't reach.
  third_party/blink/public/mojom:mojom_platform
Two ways to fix this are:
  A: Add the missing dependencies to chrome/browser/dev_ui/android:dev_ui_loader
  B: Remove chrome/browser/dev_ui/android:dev_ui_loader, and fold its
     {sources, deps} into chrome/browser:browser.
We're choosing B for simplicity, but may adopt A eventually.

Original change's description:
> Revert "[DevUI DFM] Change install flow to use NavigationThrottle."
>
> Reason for revert: Seems to have broken android-archive-rel compile, though this may be exposing some pre-existing bad dependency elsewhere: https://ci.chromium.org/p/chromium/builders/ci/android-archive-rel/5717
>
> Change-Id: I7eff36d7e01c31a76f39250a255d27972b0962d9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864392
> Commit-Queue: Peter Kasting <pkasting@chromium.org>

> Original change's description:
> > [DevUI DFM] Change install flow to use NavigationThrottle.
> >
> > Bug: 927131,987040,1013522
> > Change-Id: I5c384671c858d26a303afc58c03f4314c8ef0117
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1854524
> > Commit-Queue: Samuel Huang <huangs@chromium.org>

TBR=pkasting@chromium.org,creis@chromium.org,huangs@chromium.org,thestig@chromium.org,dbeam@chromium.org,agrieve@chromium.org,tiborg@chromium.org

Bug: 927131, 987040, 1013522
Change-Id: I526f3194b0115171721a53fca60fa6ffa6e3c441
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864411
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706514}
  • Loading branch information
samuelhuang authored and Commit Bot committed Oct 16, 2019
1 parent 36e460f commit 3a1ce1c
Show file tree
Hide file tree
Showing 26 changed files with 538 additions and 380 deletions.
14 changes: 9 additions & 5 deletions chrome/android/modules/dev_ui/provider/dev_ui_module_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,28 @@

namespace dev_ui {

// static
DevUiModuleProvider* DevUiModuleProvider::test_instance_ = nullptr;
namespace {

// Global DevUiModuleProvider instance for testing.
DevUiModuleProvider* g_test_instance = nullptr;

} // namespace

// Destructor is public to enable management by std::unique_ptr<>.
DevUiModuleProvider::~DevUiModuleProvider() = default;

// static
DevUiModuleProvider* DevUiModuleProvider::GetInstance() {
if (test_instance_)
return test_instance_;
if (g_test_instance)
return g_test_instance;

static DevUiModuleProvider instance;
return &instance;
}

// static
void DevUiModuleProvider::SetTestInstance(DevUiModuleProvider* test_instance) {
test_instance_ = test_instance;
g_test_instance = test_instance;
}

bool DevUiModuleProvider::ModuleInstalled() {
Expand Down
11 changes: 4 additions & 7 deletions chrome/android/modules/dev_ui/provider/dev_ui_module_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,27 @@ class DevUiModuleProvider {
// Returns the singleton, which can be overridden using SetTestInstance().
static DevUiModuleProvider* GetInstance();

// Overrides the singleton with caller-owned |test_instance|. Caller tests
// Overrides the singleton with caller-owned |test_instance|. Callers in tests
// are responsible for resetting this to null on cleanup.
static void SetTestInstance(DevUiModuleProvider* test_instance);

// Returns true if the DevUI module is installed.
// Returns true if the DevUI module is installed. Virtual to enable testing.
virtual bool ModuleInstalled();

// Asynchronously requests to install the DevUI module. |on_complete| is
// called after the module install is completed, and takes a bool to indicate
// whether module install is successful.
// whether module install is successful. Virtual to enable testing.
virtual void InstallModule(base::OnceCallback<void(bool)> on_complete);

// Assuming that the DevUI module is installed, loads DevUI resources if not
// already loaded.
// already loaded. Virtual to enable testing.
virtual void LoadModule();

protected:
DevUiModuleProvider();
virtual ~DevUiModuleProvider();
DevUiModuleProvider(const DevUiModuleProvider&) = delete;
DevUiModuleProvider& operator=(const DevUiModuleProvider&) = delete;

private:
static DevUiModuleProvider* test_instance_;
};

} // namespace dev_ui
Expand Down
11 changes: 5 additions & 6 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2911,13 +2911,12 @@ jumbo_split_static_library("browser") {
}

if (dfmify_dev_ui) {
# TODO(huangs): Extracting this to a separate target.
sources += [
"android/dev_ui/dev_ui_url_handler.cc",
"android/dev_ui/dev_ui_url_handler.h",
"ui/webui/android/dev_ui_loader/dev_ui_loader_message_handler.cc",
"ui/webui/android/dev_ui_loader/dev_ui_loader_message_handler.h",
"ui/webui/android/dev_ui_loader/dev_ui_loader_ui.cc",
"ui/webui/android/dev_ui_loader/dev_ui_loader_ui.h",
"dev_ui/android/dev_ui_loader_error_page.cc",
"dev_ui/android/dev_ui_loader_error_page.h",
"dev_ui/android/dev_ui_loader_throttle.cc",
"dev_ui/android/dev_ui_loader_throttle.h",
]
deps += [ "//chrome/android/modules/dev_ui/provider:native" ]
}
Expand Down
33 changes: 0 additions & 33 deletions chrome/browser/android/dev_ui/dev_ui_url_handler.h

This file was deleted.

2 changes: 1 addition & 1 deletion chrome/browser/browser_resources.grd
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@
<include name="IDR_TRANSLATE_INTERNALS_JS" file="../../components/translate/translate_internals/translate_internals.js" flattenhtml="true" compress="gzip" type="BINDATA" />
<include name="IDR_FEEDBACK_MANIFEST" file="resources\feedback\manifest.json" type="BINDATA" />
<if expr="is_android">
<include name="IDR_DEV_UI_LOADER_ERROR_HTML" file="resources/dev_ui/dev_ui_loader_error.html" compress="gzip" type="BINDATA" />
<include name="IDR_EXPLORE_SITES_INTERNALS_HTML" file="resources\explore_sites_internals\explore_sites_internals.html" allowexternalscript="true" compress="gzip" type="BINDATA" />
<include name="IDR_EXPLORE_SITES_INTERNALS_CSS" file="resources\explore_sites_internals\explore_sites_internals.css" compress="gzip" type="BINDATA" />
<include name="IDR_EXPLORE_SITES_INTERNALS_JS" file="resources\explore_sites_internals\explore_sites_internals.js" compress="gzip" type="BINDATA" />
Expand All @@ -187,7 +188,6 @@
<include name="IDR_SNIPPETS_INTERNALS_CSS" file="resources\snippets_internals\snippets_internals.css" compress="gzip" type="BINDATA" />
<include name="IDR_SNIPPETS_INTERNALS_JS" file="resources\snippets_internals\snippets_internals.js" compress="gzip" type="BINDATA" />
<include name="IDR_SNIPPETS_INTERNALS_MOJOM_LITE_JS" file="${root_gen_dir}\chrome\browser\ui\webui\snippets_internals\snippets_internals.mojom-lite.js" use_base_dir="false" type="BINDATA" compress="gzip" />
<part file="resources/dev_ui_loader/dev_ui_loader.grdp" />
</if>

<if expr="enable_supervised_users">
Expand Down
16 changes: 9 additions & 7 deletions chrome/browser/chrome_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@
#include "ui/base/resource/resource_bundle_android.h"
#include "ui/base/ui_base_paths.h"
#if BUILDFLAG(DFMIFY_DEV_UI)
#include "chrome/browser/android/dev_ui/dev_ui_url_handler.h"
#include "chrome/browser/dev_ui/android/dev_ui_loader_throttle.h"
#endif // BUILDFLAG(DFMIFY_DEV_UI)
#elif defined(OS_POSIX)
#include "chrome/browser/chrome_browser_main_posix.h"
Expand Down Expand Up @@ -3349,12 +3349,6 @@ void ChromeContentBrowserClient::BrowserURLHandlerCreated(
// Handler to rewrite chrome://newtab on Android.
handler->AddHandlerPair(&chrome::android::HandleAndroidNativePageURL,
BrowserURLHandler::null_handler());
#if BUILDFLAG(DFMIFY_DEV_UI)
// Handler to rewrite chrome:// URLs in the DevUI DFM, if not installed.
handler->AddHandlerPair(&chrome::android::HandleDfmifiedDevUiPageURL,
BrowserURLHandler::null_handler());
#endif // BUILDFLAG(DFMIFY_DEV_UI)

#else // defined(OS_ANDROID)
// Handler to rewrite chrome://newtab for InstantExtended.
handler->AddHandlerPair(&search::HandleNewTabURLRewrite,
Expand Down Expand Up @@ -3832,6 +3826,14 @@ ChromeContentBrowserClient::CreateThrottlesForNavigation(
handle, navigation_interception::SynchronyMode::kAsync));
}
throttles.push_back(InterceptOMADownloadNavigationThrottle::Create(handle));

#if BUILDFLAG(DFMIFY_DEV_UI)
// If the DevUI DFM is already installed, then this is a no-op, except for the
// side effect of ensuring that the DevUI DFM is loaded.
MaybeAddThrottle(&throttles,
dev_ui::DevUiLoaderThrottle::MaybeCreateThrottleFor(handle));
#endif // BUILDFLAG(DFMIFY_DEV_UI)

#elif BUILDFLAG(ENABLE_EXTENSIONS)
if (handle->IsInMainFrame()) {
// Redirect some navigations to apps that have registered matching URL
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/dev_ui/OWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
huangs@chromium.org
tiborg@chromium.org
File renamed without changes.
33 changes: 33 additions & 0 deletions chrome/browser/dev_ui/android/dev_ui_loader_error_page.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// 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 "chrome/browser/dev_ui/android/dev_ui_loader_error_page.h"

#include "chrome/grit/browser_resources.h"
#include "components/strings/grit/components_strings.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/resource/resource_bundle.h"
#include "ui/base/template_expressions.h"
#include "ui/strings/grit/ui_strings.h"

namespace dev_ui {

std::string BuildErrorPageHtml() {
ui::TemplateReplacements replacements;
replacements["h1"] =
l10n_util::GetStringUTF8(IDS_DEV_UI_LOADER_ERROR_HEADING);
replacements["p"] =
l10n_util::GetStringUTF8(IDS_ERRORPAGES_SUGGESTION_LIST_HEADER);
replacements["li-1"] =
l10n_util::GetStringUTF8(IDS_DEV_UI_LOADER_ERROR_SUGGEST_RELOAD);
replacements["li-2"] =
l10n_util::GetStringUTF8(IDS_DEV_UI_LOADER_ERROR_SUGGEST_CHECK_INTERNET);

std::string source =
ui::ResourceBundle::GetSharedInstance().DecompressDataResource(
IDR_DEV_UI_LOADER_ERROR_HTML);
return ui::ReplaceTemplateExpressions(source, replacements);
}

} // namespace dev_ui
16 changes: 16 additions & 0 deletions chrome/browser/dev_ui/android/dev_ui_loader_error_page.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// 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 CHROME_BROWSER_DEV_UI_ANDROID_DEV_UI_LOADER_ERROR_PAGE_H_
#define CHROME_BROWSER_DEV_UI_ANDROID_DEV_UI_LOADER_ERROR_PAGE_H_

#include <string>

namespace dev_ui {

std::string BuildErrorPageHtml();

} // namespace dev_ui

#endif // CHROME_BROWSER_DEV_UI_ANDROID_DEV_UI_LOADER_ERROR_PAGE_H_
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,20 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/android/dev_ui/dev_ui_url_handler.h"
#include "chrome/browser/dev_ui/android/dev_ui_loader_throttle.h"

#include <string>
#include <utility>

#include "base/logging.h"
#include "chrome/android/modules/dev_ui/provider/dev_ui_module_provider.h"
#include "chrome/browser/dev_ui/android/dev_ui_loader_error_page.h"
#include "chrome/common/webui_url_constants.h"
#include "components/safe_browsing/web_ui/constants.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/common/url_constants.h"
#include "net/base/url_util.h"
#include "net/base/net_errors.h"
#include "url/gurl.h"

namespace {
Expand Down Expand Up @@ -71,30 +76,65 @@ bool IsWebUiHostInDevUiDfm(const std::string& host) {

} // namespace

namespace chrome {
namespace android {
namespace dev_ui {

bool HandleDfmifiedDevUiPageURL(
GURL* url,
content::BrowserContext* /* browser_context */) {
if (!url->SchemeIs(content::kChromeUIScheme) ||
!IsWebUiHostInDevUiDfm(url->host())) {
return false;
}
// static
bool DevUiLoaderThrottle::ShouldInstallDevUiDfm(const GURL& url) {
return url.SchemeIs(content::kChromeUIScheme) &&
IsWebUiHostInDevUiDfm(url.host());
}

// static
std::unique_ptr<content::NavigationThrottle>
DevUiLoaderThrottle::MaybeCreateThrottleFor(content::NavigationHandle* handle) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
DCHECK(handle);
if (!handle->IsInMainFrame())
return nullptr;

if (!ShouldInstallDevUiDfm(handle->GetURL()))
return nullptr;

// If module is already installed, ensure that it is loaded.
if (dev_ui::DevUiModuleProvider::GetInstance()->ModuleInstalled()) {
// Synchronously load module (if not already loaded).
dev_ui::DevUiModuleProvider::GetInstance()->LoadModule();
return false;
return nullptr;
}

return std::make_unique<DevUiLoaderThrottle>(handle);
}

DevUiLoaderThrottle::DevUiLoaderThrottle(
content::NavigationHandle* navigation_handle)
: content::NavigationThrottle(navigation_handle) {}

DevUiLoaderThrottle::~DevUiLoaderThrottle() = default;

const char* DevUiLoaderThrottle::GetNameForLogging() {
return "DevUiLoaderThrottle";
}

content::NavigationThrottle::ThrottleCheckResult
DevUiLoaderThrottle::WillStartRequest() {
if (!dev_ui::DevUiModuleProvider::GetInstance()->ModuleInstalled()) {
// Can handle multiple install requests.
dev_ui::DevUiModuleProvider::GetInstance()->InstallModule(
base::BindOnce(&DevUiLoaderThrottle::OnDevUiDfmInstallWithStatus,
weak_ptr_factory_.GetWeakPtr()));
return DEFER;
}
return PROCEED;
}

void DevUiLoaderThrottle::OnDevUiDfmInstallWithStatus(bool success) {
if (success) {
dev_ui::DevUiModuleProvider::GetInstance()->LoadModule();
Resume();
} else {
std::string html = BuildErrorPageHtml();
CancelDeferredNavigation({BLOCK_REQUEST, net::ERR_CONNECTION_FAILED, html});
}
// Create URL to the DevUI loader with "?url=<escaped original URL>" so that
// after install, the loader can redirect to the original URL.
*url = net::AppendQueryParameter(
GURL(std::string(kChromeUIDevUiLoaderURL) + "dev_ui_loader.html"), "url",
url->spec());
return true;
}

} // namespace android
} // namespace chrome
} // namespace dev_ui
54 changes: 54 additions & 0 deletions chrome/browser/dev_ui/android/dev_ui_loader_throttle.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// 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 CHROME_BROWSER_DEV_UI_ANDROID_DEV_UI_LOADER_THROTTLE_H_
#define CHROME_BROWSER_DEV_UI_ANDROID_DEV_UI_LOADER_THROTTLE_H_

#include <memory>

#include "base/memory/weak_ptr.h"
#include "content/public/browser/navigation_throttle.h"

namespace content {
class NavigationHandle;
} // namespace content

class GURL;

namespace dev_ui {

// For DevUI page navigations, if the DevUI DFM is not installed then delay
// navigation and perform installation. On success, resumes navigation. On
// failure, displays error (retries install on refresh).
class DevUiLoaderThrottle : public content::NavigationThrottle {
public:
// Determines whether visiting |url| should trigger DevUI DFM install.
static bool ShouldInstallDevUiDfm(const GURL& url);

// Creates a throttle if the DevUI DFM needs to be installed. If the DevUI DFM
// will be used, is installed, but is not loaded, then resource load takes
// place as a side effect.
static std::unique_ptr<content::NavigationThrottle> MaybeCreateThrottleFor(
content::NavigationHandle* handle);

explicit DevUiLoaderThrottle(content::NavigationHandle* navigation_handle);
~DevUiLoaderThrottle() override;
DevUiLoaderThrottle(const DevUiLoaderThrottle&) = delete;
const DevUiLoaderThrottle& operator=(const DevUiLoaderThrottle&) = delete;

// content::NavigationThrottle:
const char* GetNameForLogging() override;
ThrottleCheckResult WillStartRequest() override;

private:
// Callback for dev_ui::DevUiModuleProvider::InstallModule().
void OnDevUiDfmInstallWithStatus(bool success);

// Factory for creating references in callbacks.
base::WeakPtrFactory<DevUiLoaderThrottle> weak_ptr_factory_{this};
};

} // namespace dev_ui

#endif // CHROME_BROWSER_DEV_UI_ANDROID_DEV_UI_LOADER_THROTTLE_H_
Loading

0 comments on commit 3a1ce1c

Please sign in to comment.