Skip to content

Commit

Permalink
WebApp: Upstream BookmarkAppTabHelper into WebAppTabHelper
Browse files Browse the repository at this point in the history
This CL merges WebAppTabHelperBase, BookmarkAppTabHelper and WebAppTabHelper
into one class WebAppTabHelper. This is feasible now that
BookmarkAppTabHelper has minimal firsthand dependencies on Extension code.

This is a pure refactor with no behavioural changes.

Bug: 926083
Change-Id: Ie8b714639acd12773699d534f6724a55d19b5d02
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1736620
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685916}
  • Loading branch information
alancutter authored and Commit Bot committed Aug 12, 2019
1 parent a09fddb commit f585b4d
Show file tree
Hide file tree
Showing 34 changed files with 226 additions and 311 deletions.
6 changes: 3 additions & 3 deletions chrome/browser/extensions/browsertest_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include "chrome/browser/web_applications/components/web_app_constants.h"
#include "chrome/browser/web_applications/components/web_app_helpers.h"
#include "chrome/browser/web_applications/components/web_app_provider_base.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper_base.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/web_application_info.h"
#include "chrome/test/base/ui_test_utils.h"
Expand Down Expand Up @@ -112,8 +112,8 @@ Browser* LaunchBrowserForAppInTab(Profile* profile,
AppLaunchSource::kSourceTest));
DCHECK(web_contents);

web_app::WebAppTabHelperBase* tab_helper =
web_app::WebAppTabHelperBase::FromWebContents(web_contents);
web_app::WebAppTabHelper* tab_helper =
web_app::WebAppTabHelper::FromWebContents(web_contents);
DCHECK(tab_helper);
DCHECK_EQ(extension_app->id(), tab_helper->app_id());

Expand Down
8 changes: 0 additions & 8 deletions chrome/browser/extensions/extension_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include "chrome/browser/extensions/shared_module_service.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/webui/extensions/extension_icon_source.h"
#include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
#include "chrome/common/chrome_features.h"
Expand Down Expand Up @@ -366,12 +365,5 @@ const Extension* GetPwaForSecureActiveTab(Browser* browser) {
web_contents->GetMainFrame()->GetLastCommittedURL());
}

bool IsWebContentsInAppWindow(content::WebContents* web_contents) {
// TODO(loyso): Unify this check as a util (including
// MaybeCreateHostedAppController).
Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
return browser && browser->app_controller();
}

} // namespace util
} // namespace extensions
5 changes: 0 additions & 5 deletions chrome/browser/extensions/extension_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class DictionaryValue;

namespace content {
class BrowserContext;
class WebContents;
}

namespace gfx {
Expand Down Expand Up @@ -118,10 +117,6 @@ const Extension* GetInstalledPwaForUrl(
// if there are none or the tab's is not secure.
const Extension* GetPwaForSecureActiveTab(Browser* browser);

// Returns true if the |web_contents| belongs to a browser that is a windowed
// app.
bool IsWebContentsInAppWindow(content::WebContents* web_contents);

} // namespace util
} // namespace extensions

Expand Down
10 changes: 5 additions & 5 deletions chrome/browser/ui/extensions/application_launch.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/web_applications/components/web_app_helpers.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper_base.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper.h"
#include "chrome/browser/web_launch/web_launch_files_helper.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
Expand Down Expand Up @@ -255,8 +255,8 @@ WebContents* OpenApplicationTab(const AppLaunchParams& launch_params,
}

if (extension->from_bookmark()) {
web_app::WebAppTabHelperBase* tab_helper =
web_app::WebAppTabHelperBase::FromWebContents(contents);
web_app::WebAppTabHelper* tab_helper =
web_app::WebAppTabHelper::FromWebContents(contents);
DCHECK(tab_helper);
tab_helper->SetAppId(extension->id());
}
Expand Down Expand Up @@ -422,8 +422,8 @@ WebContents* ShowApplicationWindow(const AppLaunchParams& params,
extensions::HostedAppBrowserController::SetAppPrefsForWebContents(
browser->app_controller(), web_contents);
if (extension && extension->from_bookmark()) {
web_app::WebAppTabHelperBase* tab_helper =
web_app::WebAppTabHelperBase::FromWebContents(web_contents);
web_app::WebAppTabHelper* tab_helper =
web_app::WebAppTabHelper::FromWebContents(web_contents);
DCHECK(tab_helper);
tab_helper->SetAppId(extension->id());
}
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/ui/tab_helpers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@
#include "chrome/browser/extensions/api/web_navigation/web_navigation_api.h"
#include "chrome/browser/extensions/chrome_extension_web_contents_observer.h"
#include "chrome/browser/extensions/tab_helper.h"
#include "chrome/browser/web_applications/web_app_provider.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper.h"
#include "chrome/browser/web_applications/components/web_app_utils.h"
#include "extensions/browser/view_type_utils.h"
#endif

Expand Down Expand Up @@ -338,7 +339,8 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {

#if BUILDFLAG(ENABLE_EXTENSIONS)
extensions::TabHelper::CreateForWebContents(web_contents);
web_app::WebAppProvider::CreateTabHelper(web_contents);
if (web_app::AreWebAppsEnabled(profile))
web_app::WebAppTabHelper::CreateForWebContents(web_contents);
if (SiteEngagementService::IsEnabled())
web_app::WebAppMetrics::Get(profile);
#endif
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ui/views/page_action/pwa_install_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include "chrome/browser/ui/views/extensions/pwa_confirmation_bubble_view.h"
#include "chrome/browser/ui/web_applications/web_app_dialog_utils.h"
#include "chrome/browser/web_applications/components/web_app_constants.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper_base.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper.h"
#include "chrome/grit/generated_resources.h"
#include "components/omnibox/browser/vector_icons.h"
#include "ui/base/l10n/l10n_util.h"
Expand Down Expand Up @@ -48,7 +48,7 @@ bool PwaInstallView::Update() {
show_install_button = true;

auto* web_app_tab_helper =
web_app::WebAppTabHelperBase::FromWebContents(web_contents);
web_app::WebAppTabHelper::FromWebContents(web_contents);
if (web_app_tab_helper && web_app_tab_helper->HasAssociatedApp())
show_install_button = false;

Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ui/web_applications/web_app_launch_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/web_applications/components/web_app_helpers.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper_base.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper.h"
#include "chrome/browser/web_applications/web_app.h"
#include "chrome/browser/web_applications/web_app_provider.h"
#include "chrome/browser/web_applications/web_app_registrar.h"
Expand Down Expand Up @@ -60,8 +60,8 @@ content::WebContents* ShowWebApplicationWindow(

SetWebAppPrefsForWebContents(web_contents);

web_app::WebAppTabHelperBase* tab_helper =
web_app::WebAppTabHelperBase::FromWebContents(web_contents);
web_app::WebAppTabHelper* tab_helper =
web_app::WebAppTabHelper::FromWebContents(web_contents);
DCHECK(tab_helper);
tab_helper->SetAppId(app_id);

Expand Down
7 changes: 3 additions & 4 deletions chrome/browser/ui/web_applications/web_app_metrics.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/web_applications/web_app_metrics_factory.h"
#include "chrome/browser/web_applications/components/app_registrar.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper_base.h"
#include "chrome/browser/web_applications/components/web_app_tab_helper.h"
#include "chrome/browser/web_applications/web_app_provider.h"
#include "content/public/browser/web_contents.h"

Expand Down Expand Up @@ -97,9 +97,8 @@ void WebAppMetrics::OnEngagementEvent(
engagement_type);
}

// A presence of WebAppTabHelperBase with valid app_id indicates a web app.
WebAppTabHelperBase* tab_helper =
WebAppTabHelperBase::FromWebContents(web_contents);
// A presence of WebAppTabHelper with valid app_id indicates a web app.
WebAppTabHelper* tab_helper = WebAppTabHelper::FromWebContents(web_contents);
if (!tab_helper || tab_helper->app_id().empty())
return;

Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/callback.h"
#include "build/build_config.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/extensions/application_launch.h"
#include "chrome/browser/ui/web_applications/app_browser_controller.h"
Expand Down Expand Up @@ -109,6 +110,12 @@ void WebAppUiManagerImpl::AddAppToQuickLaunchBar(const AppId& app_id) {
#endif // defined(OS_CHROMEOS)
}

bool WebAppUiManagerImpl::IsInAppWindow(
content::WebContents* web_contents) const {
return AppBrowserController::IsForWebAppBrowser(
chrome::FindBrowserWithWebContents(web_contents));
}

bool WebAppUiManagerImpl::CanReparentAppTabToWindow(
const AppId& app_id,
bool shortcut_created) const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class WebAppUiManagerImpl : public BrowserListObserver, public WebAppUiManager {
void MigrateOSAttributes(const AppId& from, const AppId& to) override;
bool CanAddAppToQuickLaunchBar() const override;
void AddAppToQuickLaunchBar(const AppId& app_id) override;
bool IsInAppWindow(content::WebContents* web_contents) const override;
bool CanReparentAppTabToWindow(const AppId& app_id,
bool shortcut_created) const override;
void ReparentAppTabToWindow(content::WebContents* contents,
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/web_applications/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ source_set("web_applications") {
"web_app_sync_bridge.h",
"web_app_sync_manager.cc",
"web_app_sync_manager.h",
"web_app_tab_helper.cc",
"web_app_tab_helper.h",
]

deps = [
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/web_applications/components/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ source_set("components") {
"web_app_shortcut_mac.mm",
"web_app_shortcut_win.cc",
"web_app_shortcut_win.h",
"web_app_tab_helper_base.cc",
"web_app_tab_helper_base.h",
"web_app_tab_helper.cc",
"web_app_tab_helper.h",
"web_app_ui_manager.h",
"web_app_url_loader.cc",
"web_app_url_loader.h",
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/web_applications/components/app_registrar.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ class AppRegistrar {
// considered external apps), then this will return true.
virtual bool WasExternalAppUninstalledByUser(const AppId& app_id) const = 0;

// Returns true if the app was installed by user, false if default installed.
virtual bool WasInstalledByUser(const AppId& app_id) const = 0;

// Returns the AppIds and URLs of apps externally installed from
// |install_source|.
virtual std::map<AppId, GURL> GetExternallyInstalledApps(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class WebAppAudioFocusIdMap {
~WebAppAudioFocusIdMap();

protected:
friend class WebAppTabHelperBase;
friend class WebAppTabHelper;

const base::UnguessableToken& CreateOrGetIdForApp(const AppId& app_id);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class PendingAppManager;
class InstallManager;
class AppRegistrar;
class WebAppPolicyManager;
class WebAppAudioFocusIdMap;
class WebAppUiManager;

class WebAppProviderBase : public KeyedService {
Expand All @@ -39,6 +40,8 @@ class WebAppProviderBase : public KeyedService {

virtual WebAppUiManager& ui_manager() = 0;

virtual WebAppAudioFocusIdMap& audio_focus_id_map() = 0;

DISALLOW_COPY_AND_ASSIGN(WebAppProviderBase);
};

Expand Down
135 changes: 135 additions & 0 deletions chrome/browser/web_applications/components/web_app_tab_helper.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
// 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/components/web_app_tab_helper.h"

#include "base/unguessable_token.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/web_applications/components/policy/web_app_policy_manager.h"
#include "chrome/browser/web_applications/components/web_app_audio_focus_id_map.h"
#include "chrome/browser/web_applications/components/web_app_provider_base.h"
#include "chrome/browser/web_applications/components/web_app_ui_manager.h"
#include "content/public/browser/media_session.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/site_instance.h"

namespace web_app {

WEB_CONTENTS_USER_DATA_KEY_IMPL(WebAppTabHelper)

WebAppTabHelper::WebAppTabHelper(content::WebContents* web_contents)
: content::WebContentsObserver(web_contents),
provider_(WebAppProviderBase::GetProviderBase(
Profile::FromBrowserContext(web_contents->GetBrowserContext()))) {
DCHECK(provider_);
observer_.Add(&provider_->registrar());
SetAppId(
FindAppIdWithUrlInScope(web_contents->GetSiteInstance()->GetSiteURL()));
}

WebAppTabHelper::~WebAppTabHelper() = default;

bool WebAppTabHelper::HasAssociatedApp() const {
return !app_id_.empty();
}

void WebAppTabHelper::SetAppId(const AppId& app_id) {
DCHECK(app_id.empty() || provider_->registrar().IsInstalled(app_id));
if (app_id_ == app_id)
return;

app_id_ = app_id;

OnAssociatedAppChanged();
}

void WebAppTabHelper::DidFinishNavigation(
content::NavigationHandle* navigation_handle) {
if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
return;

const AppId app_id = FindAppIdWithUrlInScope(navigation_handle->GetURL());
SetAppId(app_id);

ReinstallPlaceholderAppIfNecessary(navigation_handle->GetURL());
}

void WebAppTabHelper::DidCloneToNewWebContents(
content::WebContents* old_web_contents,
content::WebContents* new_web_contents) {
// When the WebContents that this is attached to is cloned, give the new clone
// a WebAppTabHelper.
CreateForWebContents(new_web_contents);
auto* new_tab_helper = FromWebContents(new_web_contents);

// Clone common state:
new_tab_helper->SetAppId(app_id());
}

bool WebAppTabHelper::IsUserInstalled() const {
return !app_id_.empty() && provider_->registrar().WasInstalledByUser(app_id_);
}

bool WebAppTabHelper::IsFromInstallButton() const {
// TODO(loyso): Use something better to record apps installed from promoted
// UIs. crbug.com/774918.
return !app_id_.empty() &&
provider_->registrar().GetAppScope(app_id_).has_value();
}

bool WebAppTabHelper::IsInAppWindow() const {
return provider_->ui_manager().IsInAppWindow(web_contents());
}

void WebAppTabHelper::OnWebAppInstalled(const AppId& installed_app_id) {
// Check if current web_contents url is in scope for the newly installed app.
AppId app_id = FindAppIdWithUrlInScope(web_contents()->GetURL());
if (app_id == installed_app_id)
SetAppId(app_id);
}

void WebAppTabHelper::OnWebAppUninstalled(const AppId& uninstalled_app_id) {
if (app_id() == uninstalled_app_id)
ResetAppId();
}

void WebAppTabHelper::OnAppRegistrarShutdown() {
ResetAppId();
}

void WebAppTabHelper::OnAppRegistrarDestroyed() {
observer_.RemoveAll();
}

void WebAppTabHelper::ResetAppId() {
app_id_.clear();

OnAssociatedAppChanged();
}

void WebAppTabHelper::OnAssociatedAppChanged() {
UpdateAudioFocusGroupId();
}

void WebAppTabHelper::UpdateAudioFocusGroupId() {
if (!app_id_.empty() && IsInAppWindow()) {
audio_focus_group_id_ =
provider_->audio_focus_id_map().CreateOrGetIdForApp(app_id_);
} else {
audio_focus_group_id_ = base::UnguessableToken::Null();
}

content::MediaSession::Get(web_contents())
->SetAudioFocusGroupId(audio_focus_group_id_);
}

void WebAppTabHelper::ReinstallPlaceholderAppIfNecessary(const GURL& url) {
provider_->policy_manager().ReinstallPlaceholderAppIfNecessary(url);
}

AppId WebAppTabHelper::FindAppIdWithUrlInScope(const GURL& url) const {
return provider_->registrar().FindAppWithUrlInScope(url).value_or(AppId());
}

} // namespace web_app

0 comments on commit f585b4d

Please sign in to comment.