Skip to content

GitLab

T
tangled
Commit e3086692 authored by Roger Tawa's avatar Roger Tawa Committed by Commit Bot
Browse files
Options

Revert "Allow (auto-installed) external web apps" 

This reverts commit b43fa6db.

Reason for revert:
Caused three bugs open for flaky tests:
https://bugs.chromium.org/p/chromium/issues/detail?id=869043
https://bugs.chromium.org/p/chromium/issues/detail?id=868769
https://bugs.chromium.org/p/chromium/issues/detail?id=868839

The speculation is that new calls to ScanDirForExternalWebApps() are not being done on the correct thread.

Original change's description:
> Allow (auto-installed) external web apps
> 
> These are the Web App analogs of external extensions, described at
> https://developer.chrome.com/apps/external_extensions
> 
> On start up, *.json files (often but not necessarily named
> external_extensions.json) are scanned in a number of directories. Prior
> to this CL, such .json files can install regular extensions (.crx files)
> or, on Chrome OS, Android apps. New in this CL is being able to install
> Web Apps (sometimes known as Progressive Web Apps or PWAs).
> 
> For example, the chrome::DIR_USER_EXTERNAL_EXTENSIONS path (defined in
> chrome/common/chrome_paths.h) can correspond to file system directory
> like "$HOME/.config/chromium/test-user/.config/chromium/External
> Extensions". Placing a foo_bar.json file containing:
> 
> {
>   "web_app_manifest_url": "https://www.chromestatus.com/static/manifest.json",
>   "web_app_start_url": "https://www.chromestatus.com/features"
> }
> 
> in that directory will install the Chrome Platform Status web app.
> 
> Bug: 855281
> Change-Id: I716fead81d407076a0dda32f5c0b3e8869351c83
> Reviewed-on: https://chromium-review.googlesource.com/1127214
> Commit-Queue: Nigel Tao <nigeltao@chromium.org>
> Reviewed-by: Dominick Ng <dominickn@chromium.org>
> Reviewed-by: Alexey Baskakov <loyso@chromium.org>
> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578944}

TBR=ortuno@chromium.org,loyso@chromium.org,dominickn@chromium.org,nigeltao@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 855281
Change-Id: I976928b151910bcc39ea372a6f80d9d833fc8253
Reviewed-on: https://chromium-review.googlesource.com/1155496Reviewed-by: default avatarRoger Tawa <rogerta@chromium.org>
Commit-Queue: Roger Tawa <rogerta@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579247}
parent 7d7da7f2
Hide whitespace changes
Inline Side-by-side
Showing with 1 addition and 303 deletions
chrome/browser/web_applications/bookmark_apps/BUILD.gn
View file @ e3086692
......@@ -8,8 +8,6 @@ assert(enable_extensions)
source_set("bookmark_apps") {
sources = [
"external_web_apps.cc",
"external_web_apps.h",
"policy/web_app_policy_constants.cc",
"policy/web_app_policy_constants.h",
"policy/web_app_policy_manager.cc",
......@@ -27,7 +25,6 @@ source_set("unit_tests") {
testonly = true
sources = [
"external_web_apps_unittest.cc",
"policy/web_app_policy_manager_unittest.cc",
]
......
chrome/browser/web_applications/bookmark_apps/external_web_apps.cc deleted 100644 → 0
View file @ 7d7da7f2
// Copyright 2018 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/web_applications/bookmark_apps/external_web_apps.h"
#include <memory>
#include <string>
#include <utility>
#include "base/callback.h"
#include "base/files/file_enumerator.h"
#include "base/json/json_file_value_serializer.h"
#include "base/threading/thread_restrictions.h"
#include "url/gurl.h"
namespace web_app {
static constexpr char kWebAppManifestUrl[] = "web_app_manifest_url";
static constexpr char kWebAppStartUrl[] = "web_app_start_url";
void ScanDirForExternalWebApps(base::FilePath dir,
ScanDirForExternalWebAppsCallback callback) {
base::AssertBlockingAllowed();
base::FilePath::StringType extension(FILE_PATH_LITERAL(".json"));
base::FileEnumerator json_files(dir,
false, // Recursive.
base::FileEnumerator::FILES);
std::vector<web_app::PendingAppManager::AppInfo> app_infos;
for (base::FilePath file = json_files.Next(); !file.empty();
file = json_files.Next()) {
if (!file.MatchesExtension(extension)) {
continue;
}
JSONFileValueDeserializer deserializer(file);
std::string error_msg;
std::unique_ptr<base::Value> value =
deserializer.Deserialize(nullptr, &error_msg);
if (!value) {
VLOG(2) << file.value() << " was not valid JSON: " << error_msg;
continue;
}
if (value->type() != base::Value::Type::DICTIONARY) {
VLOG(2) << file.value() << " was not a dictionary as the top level";
continue;
}
std::unique_ptr<base::DictionaryValue> dict_value =
base::DictionaryValue::From(std::move(value));
std::string manifest_url_str;
if (!dict_value->GetString(kWebAppStartUrl, &manifest_url_str) ||
manifest_url_str.empty() || !GURL(manifest_url_str).is_valid()) {
VLOG(2) << file.value() << " had an invalid " << kWebAppManifestUrl;
continue;
}
std::string start_url_str;
if (!dict_value->GetString(kWebAppStartUrl, &start_url_str) ||
start_url_str.empty()) {
VLOG(2) << file.value() << " had an invalid " << kWebAppStartUrl;
continue;
}
GURL start_url(start_url_str);
if (!start_url.is_valid()) {
VLOG(2) << file.value() << " had an invalid " << kWebAppStartUrl;
continue;
}
app_infos.emplace_back(
std::move(start_url),
web_app::PendingAppManager::LaunchContainer::kWindow);
}
std::move(callback).Run(std::move(app_infos));
}
} // namespace web_app
chrome/browser/web_applications/bookmark_apps/external_web_apps.h deleted 100644 → 0
View file @ 7d7da7f2
// Copyright 2018 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_WEB_APPLICATIONS_BOOKMARK_APPS_EXTERNAL_WEB_APPS_H_
#define CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_EXTERNAL_WEB_APPS_H_
#include <vector>
#include "base/callback_forward.h"
#include "base/files/file_path.h"
#include "chrome/browser/web_applications/components/pending_app_manager.h"
namespace web_app {
using ScanDirForExternalWebAppsCallback =
base::OnceCallback<void(std::vector<web_app::PendingAppManager::AppInfo>)>;
// Scans the given directory (non-recursively) for *.json files that define
// "external web apps", the Web App analogs of "external extensions", described
// at https://developer.chrome.com/apps/external_extensions
//
// This function performs file I/O, and must not be scheduled on UI threads.
void ScanDirForExternalWebApps(base::FilePath dir,
ScanDirForExternalWebAppsCallback callback);
} // namespace web_app
#endif // CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_EXTERNAL_WEB_APPS_H_
chrome/browser/web_applications/bookmark_apps/external_web_apps_unittest.cc deleted 100644 → 0
View file @ 7d7da7f2
// Copyright (c) 2018 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/web_applications/bookmark_apps/external_web_apps.h"
#include <algorithm>
#include <vector>
#include "base/bind.h"
#include "base/callback.h"
#include "base/path_service.h"
#include "chrome/common/chrome_paths.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"
namespace {
static constexpr char kWebAppDefaultApps[] = "web_app_default_apps";
// Returns the chrome/test/data/web_app_default_apps/sub_dir directory that
// holds the *.json data files from which ScanDirForExternalWebApps should
// extract URLs from.
static base::FilePath test_dir(const char* sub_dir) {
base::FilePath dir;
if (!base::PathService::Get(chrome::DIR_TEST_DATA, &dir)) {
ADD_FAILURE()
<< "base::PathService::Get could not resolve chrome::DIR_TEST_DATA";
}
return dir.AppendASCII(kWebAppDefaultApps).AppendASCII(sub_dir);
}
using AppInfos = std::vector<web_app::PendingAppManager::AppInfo>;
} // namespace
class ScanDirForExternalWebAppsTest : public testing::Test {};
TEST_F(ScanDirForExternalWebAppsTest, GoodJson) {
// The good_json directory contains two good JSON files:
// chrome_platform_status.json and google_io_2016.json.
web_app::ScanDirForExternalWebApps(
test_dir("good_json"), base::BindOnce([](AppInfos app_infos) {
static std::string urls[] = {
"https://www.chromestatus.com/features",
"https://events.google.com/io2016/?utm_source=web_app_manifest",
};
EXPECT_EQ(2u, app_infos.size());
for (const auto& url : urls) {
EXPECT_NE(
app_infos.end(),
std::find(
app_infos.begin(), app_infos.end(),
web_app::PendingAppManager::AppInfo(
GURL(url),
web_app::PendingAppManager::LaunchContainer::kWindow)));
}
}));
}
TEST_F(ScanDirForExternalWebAppsTest, BadJson) {
// The bad_json directory contains one (malformed) JSON file.
web_app::ScanDirForExternalWebApps(test_dir("bad_json"),
base::BindOnce([](AppInfos app_infos) {
EXPECT_EQ(0u, app_infos.size());
}));
}
TEST_F(ScanDirForExternalWebAppsTest, TxtButNoJson) {
// The txt_but_no_json directory contains one file, and the contents of that
// file is valid JSON, but that file's name does not end with ".json".
web_app::ScanDirForExternalWebApps(test_dir("txt_but_no_json"),
base::BindOnce([](AppInfos app_infos) {
EXPECT_EQ(0u, app_infos.size());
}));
}
TEST_F(ScanDirForExternalWebAppsTest, MixedJson) {
// The mixed_json directory contains one empty JSON file, one malformed JSON
// file and one good JSON file. ScanDirForExternalWebApps should still pick
// up that one good JSON file: polytimer.json.
web_app::ScanDirForExternalWebApps(
test_dir("mixed_json"), base::BindOnce([](AppInfos app_infos) {
EXPECT_EQ(1u, app_infos.size());
if (app_infos.size() == 1) {
EXPECT_EQ(app_infos[0].url.spec(),
std::string("https://polytimer.rocks/?homescreen=1"));
}
}));
}
chrome/browser/web_applications/web_app_provider.cc
View file @ e3086692
......@@ -4,14 +4,8 @@
#include "chrome/browser/web_applications/web_app_provider.h"
#include "base/bind.h"
#include "base/path_service.h"
#include "base/task_scheduler/post_task.h"
#include "build/build_config.h"
#include "chrome/browser/web_applications/bookmark_apps/external_web_apps.h"
#include "chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h"
#include "chrome/browser/web_applications/web_app_provider_factory.h"
#include "chrome/common/chrome_paths.h"
namespace web_app {
......@@ -23,70 +17,9 @@ WebAppProvider* WebAppProvider::Get(Profile* profile) {
WebAppProvider::WebAppProvider(PrefService* pref_service)
: web_app_policy_manager_(
std::make_unique<WebAppPolicyManager>(pref_service)) {
#if defined(OS_CHROMEOS)
// As of mid 2018, only Chrome OS has default web apps or external web apps.
// In the future, we might open external web apps to other flavors.
ScanForExternalWebApps();
#endif // defined(OS_CHROMEOS)
// TODO(nigeltao): install default web apps as per http://crbug.com/855281
}
WebAppProvider::~WebAppProvider() = default;
void WebAppProvider::ScanForExternalWebApps() {
#if !defined(OS_CHROMEOS)
// chrome::DIR_STANDALONE_EXTERNAL_EXTENSIONS is only defined for OS_LINUX.
// OS_CHROMEOS is a variant of OS_LINUX.
#else
// For manual testing, it can be useful to s/STANDALONE/USER/, as writing to
// "$HOME/.config/chromium/test-user/.config/chromium/External Extensions"
// does not require root ACLs, unlike "/usr/share/chromium/extensions".
//
// TODO(nigeltao): do we want to append a sub-directory name, analogous to
// the "arc" in "/usr/share/chromium/extensions/arc" as per
// chrome/browser/ui/app_list/arc/arc_default_app_list.cc? Or should we not
// sort "system apps" into directories based on their platform (e.g. ARC,
// PWA, etc.), and instead examine the JSON contents (e.g. an "activity"
// key means ARC, "web_app_start_url" key means PWA, etc.)?
base::FilePath dir;
if (!base::PathService::Get(chrome::DIR_STANDALONE_EXTERNAL_EXTENSIONS,
&dir)) {
LOG(ERROR) << "ScanForExternalWebApps: base::PathService::Get failed";
return;
}
auto callback =
base::BindOnce(&WebAppProvider::ScanForExternalWebAppsCallback,
weak_ptr_factory_.GetWeakPtr());
base::PostTaskWithTraits(FROM_HERE,
{base::MayBlock(), base::TaskPriority::BEST_EFFORT},
base::BindOnce(&web_app::ScanDirForExternalWebApps,
dir, std::move(callback)));
#endif // !defined(OS_CHROMEOS)
}
void WebAppProvider::ScanForExternalWebAppsCallback(
std::vector<web_app::PendingAppManager::AppInfo> app_infos) {
// TODO(nigeltao/ortuno): we shouldn't need a *policy* manager to get to a
// PendingAppManager. As ortuno@ says, "We should refactor PendingAppManager
// to no longer be owned by WebAppPolicyManager. I made it that way since at
// the time, WebAppPolicyManager was its only client."
//
// TODO(nigeltao/ortuno): drop the const_cast. Either
// WebAppPolicyManager::pending_app_manager should lose the const or we
// should acquire a (non-const) PendingAppManager by other means.
auto& pending_app_manager = const_cast<web_app::PendingAppManager&>(
web_app_policy_manager_->pending_app_manager());
// TODO(nigeltao/ortuno): confirm that the PendingAppManager callee is
// responsible for filtering out already-installed apps.
//
// TODO(nigeltao/ortuno): does the PendingAppManager care which thread we're
// on (e.g. "the UI thread", "the File thread") when we call into it? Do we
// need to bounce to another thread from here? Note that, in the long term,
// we might not be in the browser process, so there might not be the concept
// of "the UI thread".
pending_app_manager.ProcessAppOperations(std::move(app_infos));
}
} // namespace web_app
chrome/browser/web_applications/web_app_provider.h
View file @ e3086692
......@@ -6,11 +6,8 @@
#define CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_PROVIDER_H_
#include <memory>
#include <vector>
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "chrome/browser/web_applications/components/pending_app_manager.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/prefs/pref_service.h"
......@@ -32,14 +29,8 @@ class WebAppProvider : public KeyedService {
~WebAppProvider() override;
private:
void ScanForExternalWebApps();
void ScanForExternalWebAppsCallback(
std::vector<web_app::PendingAppManager::AppInfo>);
std::unique_ptr<WebAppPolicyManager> web_app_policy_manager_;
base::WeakPtrFactory<WebAppProvider> weak_ptr_factory_{this};
DISALLOW_COPY_AND_ASSIGN(WebAppProvider);
};
......
chrome/test/data/web_app_default_apps/good_json/chrome_platform_status.json deleted 100644 → 0
View file @ 7d7da7f2
{
"web_app_manifest_url": "https://www.chromestatus.com/static/manifest.json",
"web_app_start_url": "https://www.chromestatus.com/features"
}
chrome/test/data/web_app_default_apps/good_json/google_io_2016.json deleted 100644 → 0
View file @ 7d7da7f2
{
"web_app_manifest_url": "https://events.google.com/io2016/manifest.json",
"web_app_start_url": "https://events.google.com/io2016/?utm_source=web_app_manifest"
}
chrome/test/data/web_app_default_apps/mixed_json/malformed1.json deleted 100644 → 0
View file @ 7d7da7f2
{
"web_app_manifest_url": "https://www.chromestatus.com/static/manifest.json",
"web_app_start_url": "https://www.chromestatus.com/features",
"this is malformed JSON because there's no end quote
}
chrome/test/data/web_app_default_apps/mixed_json/polytimer.json deleted 100644 → 0
View file @ 7d7da7f2
{
"web_app_manifest_url": "https://polytimer.rocks/manifest.json",
"web_app_start_url": "https://polytimer.rocks/?homescreen=1"
}
chrome/test/data/web_app_default_apps/txt_but_no_json/chrome_platform_status.txt deleted 100644 → 0
View file @ 7d7da7f2
{
"web_app_manifest_url": "https://www.chromestatus.com/static/manifest.json",
"web_app_start_url": "https://www.chromestatus.com/features"
}
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