commit fce18322d66ea6e67275e13242dae2a8c06d3ae2
Author: Yuzu Saijo <yuzus@chromium.org>
Date: Thu May 14 05:02:09 2020 +0000
[content] Manage ManifestManagerHost per-document
This CL converts ManifestManagerHost class away from being owned by
WebContents and makes it a part of RenderDocumentHostUserData.
We used to create an instance per-WebContents, but now it is created for
document, and only for main-frame.
Bug: 1077170
Change-Id: I2a185a6cd6f290d3b904d359b55638bf09eaa79f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2147485
Commit-Queue: Yuzu Saijo <yuzus@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Sreeja Kamishetty <sreejakshetty@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768647}
diff --git b/content/browser/devtools/protocol/page_handler.cc a/content/browser/devtools/protocol/page_handler.cc
index b1821434b975..929b63ab875e 100644
--- b/content/browser/devtools/protocol/page_handler.cc
+++ a/content/browser/devtools/protocol/page_handler.cc
@@ -961,14 +961,14 @@ Response PageHandler::SetDownloadBehavior(const std::string& behavior,
void PageHandler::GetAppManifest(
std::unique_ptr<GetAppManifestCallback> callback) {
- if (!host_) {
+ WebContentsImpl* web_contents = GetWebContents();
+ if (!web_contents || !web_contents->GetManifestManagerHost()) {
callback->sendFailure(Response::ServerError("Cannot retrieve manifest"));
return;
}
- ManifestManagerHost::GetOrCreateForCurrentDocument(host_->GetMainFrame())
- ->RequestManifestDebugInfo(base::BindOnce(&PageHandler::GotManifest,
- weak_factory_.GetWeakPtr(),
- std::move(callback)));
+ web_contents->GetManifestManagerHost()->RequestManifestDebugInfo(
+ base::BindOnce(&PageHandler::GotManifest, weak_factory_.GetWeakPtr(),
+ std::move(callback)));
}
WebContentsImpl* PageHandler::GetWebContents() {
diff --git b/content/browser/frame_host/render_document_host_user_data_browsertest.cc a/content/browser/frame_host/render_document_host_user_data_browsertest.cc
index 09dff7842517..290e5509b448 100644
--- b/content/browser/frame_host/render_document_host_user_data_browsertest.cc
+++ a/content/browser/frame_host/render_document_host_user_data_browsertest.cc
@@ -88,7 +88,7 @@ class RenderDocumentHostUserDataTest : public ContentBrowserTest {
// Test basic functionality of RenderDocumentHostUserData.
IN_PROC_BROWSER_TEST_F(RenderDocumentHostUserDataTest,
- GetCreateAndDeleteForCurrentDocument) {
+ GetAndCreateForCurrentDocument) {
ASSERT_TRUE(embedded_test_server()->Start());
GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));
@@ -104,14 +104,8 @@ IN_PROC_BROWSER_TEST_F(RenderDocumentHostUserDataTest,
// 3) Create Data and check that GetForCurrentDocument shouldn't return null
// now.
Data::CreateForCurrentDocument(rfh_a);
- base::WeakPtr<Data> created_data =
- Data::GetForCurrentDocument(rfh_a)->GetWeakPtr();
- EXPECT_TRUE(created_data);
-
- // 4) Delete Data and check that GetForCurrentDocument should return null.
- Data::DeleteForCurrentDocument(rfh_a);
- EXPECT_FALSE(created_data);
- EXPECT_FALSE(Data::GetForCurrentDocument(rfh_a));
+ data = Data::GetForCurrentDocument(rfh_a);
+ EXPECT_TRUE(data);
}
// Tests that RenderDocumentHostUserData objects are different for each
diff --git b/content/browser/frame_host/render_frame_host_impl.cc a/content/browser/frame_host/render_frame_host_impl.cc
index 30bc648d74ef..d10d99df7f1f 100644
--- b/content/browser/frame_host/render_frame_host_impl.cc
+++ a/content/browser/frame_host/render_frame_host_impl.cc
@@ -75,7 +75,6 @@
#include "content/browser/loader/navigation_url_loader_impl.h"
#include "content/browser/loader/prefetch_url_loader_service.h"
#include "content/browser/log_console_message.h"
-#include "content/browser/manifest/manifest_manager_host.h"
#include "content/browser/media/capture/audio_mirroring_manager.h"
#include "content/browser/media/media_interface_proxy.h"
#include "content/browser/media/webaudio/audio_context_manager_impl.h"
@@ -6146,15 +6145,6 @@ void RenderFrameHostImpl::SetUpMojoIfNeeded() {
std::make_unique<ActiveURLMessageFilter>(impl));
},
base::Unretained(this)));
-
- associated_registry_->AddInterface(base::BindRepeating(
- [](RenderFrameHostImpl* impl,
- mojo::PendingAssociatedReceiver<
- blink::mojom::ManifestUrlChangeObserver> receiver) {
- ManifestManagerHost::GetOrCreateForCurrentDocument(impl)
- ->BindObserver(std::move(receiver));
- },
- base::Unretained(this)));
}
associated_registry_->AddInterface(base::BindRepeating(
diff --git b/content/browser/frame_host/render_frame_host_impl.h a/content/browser/frame_host/render_frame_host_impl.h
index bfd421386795..2bba134e0dc0 100644
--- b/content/browser/frame_host/render_frame_host_impl.h
+++ a/content/browser/frame_host/render_frame_host_impl.h
@@ -1595,10 +1595,6 @@ class CONTENT_EXPORT RenderFrameHostImpl
document_associated_data_.SetUserData(key, std::move(data));
}
- void RemoveRenderDocumentHostUserData(const void* key) {
- document_associated_data_.RemoveUserData(key);
- }
-
// Returns the child RenderFrameHostImpl if |child_frame_routing_id| is an
// immediate child of this FrameTreeNode. |child_frame_routing_id| is
// considered untrusted, so the renderer process is killed if it refers to a
diff --git b/content/browser/manifest/manifest_manager_host.cc a/content/browser/manifest/manifest_manager_host.cc
index 68ea016c62eb..b063e0d1e98e 100644
--- b/content/browser/manifest/manifest_manager_host.cc
+++ a/content/browser/manifest/manifest_manager_host.cc
@@ -9,34 +9,25 @@
#include "base/bind.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
#include "services/service_manager/public/cpp/interface_provider.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "third_party/blink/public/common/manifest/manifest.h"
namespace content {
-ManifestManagerHost::ManifestManagerHost(RenderFrameHost* render_frame_host)
- : manifest_manager_frame_(render_frame_host) {
- // Check that |manifest_manager_frame_| is a main frame.
- DCHECK(!manifest_manager_frame_->GetParent());
-}
+ManifestManagerHost::ManifestManagerHost(WebContents* web_contents)
+ : WebContentsObserver(web_contents),
+ manifest_url_change_observer_receivers_(web_contents, this) {}
ManifestManagerHost::~ManifestManagerHost() {
OnConnectionError();
}
-void ManifestManagerHost::BindObserver(
- mojo::PendingAssociatedReceiver<blink::mojom::ManifestUrlChangeObserver>
- receiver) {
- manifest_url_change_observer_receiver_.Bind(std::move(receiver));
-}
-
-ManifestManagerHost* ManifestManagerHost::GetOrCreateForCurrentDocument(
- RenderFrameHostImpl* rfh) {
- DCHECK(rfh->is_main_frame());
- if (!GetForCurrentDocument(rfh))
- CreateForCurrentDocument(rfh);
- return GetForCurrentDocument(rfh);
+void ManifestManagerHost::RenderFrameDeleted(
+ RenderFrameHost* render_frame_host) {
+ if (render_frame_host == manifest_manager_frame_)
+ OnConnectionError();
}
void ManifestManagerHost::GetManifest(GetManifestCallback callback) {
@@ -54,7 +45,11 @@ void ManifestManagerHost::RequestManifestDebugInfo(
}
blink::mojom::ManifestManager& ManifestManagerHost::GetManifestManager() {
+ if (manifest_manager_frame_ != web_contents()->GetMainFrame())
+ OnConnectionError();
+
if (!manifest_manager_) {
+ manifest_manager_frame_ = web_contents()->GetMainFrame();
manifest_manager_frame_->GetRemoteInterfaces()->GetInterface(
manifest_manager_.BindNewPipeAndPassReceiver());
manifest_manager_.set_disconnect_handler(base::BindOnce(
@@ -64,6 +59,8 @@ blink::mojom::ManifestManager& ManifestManagerHost::GetManifestManager() {
}
void ManifestManagerHost::OnConnectionError() {
+ manifest_manager_frame_ = nullptr;
+ manifest_manager_.reset();
std::vector<GetManifestCallback> callbacks;
for (CallbackMap::iterator it(&callbacks_); !it.IsAtEnd(); it.Advance()) {
callbacks.push_back(std::move(*it.GetCurrentValue()));
@@ -71,10 +68,6 @@ void ManifestManagerHost::OnConnectionError() {
callbacks_.Clear();
for (auto& callback : callbacks)
std::move(callback).Run(GURL(), blink::Manifest());
-
- if (GetForCurrentDocument(manifest_manager_frame_)) {
- DeleteForCurrentDocument(manifest_manager_frame_);
- }
}
void ManifestManagerHost::OnRequestManifestResponse(
@@ -88,16 +81,12 @@ void ManifestManagerHost::OnRequestManifestResponse(
void ManifestManagerHost::ManifestUrlChanged(
const base::Optional<GURL>& manifest_url) {
- if (!manifest_manager_frame_->IsCurrent())
+ if (manifest_url_change_observer_receivers_.GetCurrentTargetFrame() !=
+ web_contents()->GetMainFrame()) {
return;
-
- // TODO(yuzus): |NotifyManifestUrlChanged| should start taking a
- // |RenderFrameHost| parameter.
- WebContents* web_contents =
- WebContents::FromRenderFrameHost(manifest_manager_frame_);
- static_cast<WebContentsImpl*>(web_contents)
+ }
+ static_cast<WebContentsImpl*>(web_contents())
->NotifyManifestUrlChanged(manifest_url);
}
-RENDER_DOCUMENT_HOST_USER_DATA_KEY_IMPL(ManifestManagerHost)
} // namespace content
diff --git b/content/browser/manifest/manifest_manager_host.h a/content/browser/manifest/manifest_manager_host.h
index 57f51dc9fad7..3dc0bbf6e1ad 100644
--- b/content/browser/manifest/manifest_manager_host.h
+++ a/content/browser/manifest/manifest_manager_host.h
@@ -8,8 +8,8 @@
#include "base/callback_forward.h"
#include "base/containers/id_map.h"
#include "base/macros.h"
-#include "content/public/browser/render_document_host_user_data.h"
-#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_receiver_set.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "third_party/blink/public/mojom/manifest/manifest_manager.mojom.h"
#include "third_party/blink/public/mojom/manifest/manifest_observer.mojom.h"
@@ -21,16 +21,16 @@ struct Manifest;
namespace content {
class RenderFrameHost;
-class RenderFrameHostImpl;
+class WebContents;
// ManifestManagerHost is a helper class that allows callers to get the Manifest
// associated with the main frame of the observed WebContents. It handles the
// IPC messaging with the child process.
// TODO(mlamouri): keep a cached version and a dirty bit here.
-class ManifestManagerHost
- : public RenderDocumentHostUserData<ManifestManagerHost>,
- public blink::mojom::ManifestUrlChangeObserver {
+class ManifestManagerHost : public WebContentsObserver,
+ public blink::mojom::ManifestUrlChangeObserver {
public:
+ explicit ManifestManagerHost(WebContents* web_contents);
~ManifestManagerHost() override;
using GetManifestCallback =
@@ -44,18 +44,10 @@ class ManifestManagerHost
void RequestManifestDebugInfo(
blink::mojom::ManifestManager::RequestManifestDebugInfoCallback callback);
- void BindObserver(
- mojo::PendingAssociatedReceiver<blink::mojom::ManifestUrlChangeObserver>
- receiver);
-
- static ManifestManagerHost* GetOrCreateForCurrentDocument(
- RenderFrameHostImpl* rfh);
+ // WebContentsObserver
+ void RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
private:
- explicit ManifestManagerHost(RenderFrameHost* render_frame_host);
-
- friend class RenderDocumentHostUserData<ManifestManagerHost>;
-
using CallbackMap = base::IDMap<std::unique_ptr<GetManifestCallback>>;
blink::mojom::ManifestManager& GetManifestManager();
@@ -68,14 +60,13 @@ class ManifestManagerHost
// blink::mojom::ManifestUrlChangeObserver:
void ManifestUrlChanged(const base::Optional<GURL>& manifest_url) override;
- RenderFrameHost* manifest_manager_frame_;
+ RenderFrameHost* manifest_manager_frame_ = nullptr;
mojo::Remote<blink::mojom::ManifestManager> manifest_manager_;
CallbackMap callbacks_;
- mojo::AssociatedReceiver<blink::mojom::ManifestUrlChangeObserver>
- manifest_url_change_observer_receiver_{this};
+ WebContentsFrameReceiverSet<blink::mojom::ManifestUrlChangeObserver>
+ manifest_url_change_observer_receivers_;
- RENDER_DOCUMENT_HOST_USER_DATA_KEY_DECL();
DISALLOW_COPY_AND_ASSIGN(ManifestManagerHost);
};
diff --git b/content/browser/web_contents/web_contents_impl.cc a/content/browser/web_contents/web_contents_impl.cc
index 024415bb096e..115f480ce8c1 100644
--- b/content/browser/web_contents/web_contents_impl.cc
+++ a/content/browser/web_contents/web_contents_impl.cc
@@ -2122,6 +2122,8 @@ void WebContentsImpl::Init(const WebContents::CreateParams& params) {
screen_orientation_provider_.reset(new ScreenOrientationProvider(this));
+ manifest_manager_host_.reset(new ManifestManagerHost(this));
+
#if defined(OS_ANDROID)
DateTimeChooserAndroid::CreateForWebContents(this);
#endif
@@ -4202,10 +4204,7 @@ bool WebContentsImpl::WasEverAudible() {
}
void WebContentsImpl::GetManifest(GetManifestCallback callback) {
- // TODO(yuzus, 1061899): Move this function to RenderFrameHostImpl.
- ManifestManagerHost* manifest_manager_host =
- ManifestManagerHost::GetOrCreateForCurrentDocument(GetMainFrame());
- manifest_manager_host->GetManifest(std::move(callback));
+ manifest_manager_host_->GetManifest(std::move(callback));
}
void WebContentsImpl::ExitFullscreen(bool will_cause_resize) {
diff --git b/content/browser/web_contents/web_contents_impl.h a/content/browser/web_contents/web_contents_impl.h
index 59510b1f8744..e86be96fc23b 100644
--- b/content/browser/web_contents/web_contents_impl.h
+++ a/content/browser/web_contents/web_contents_impl.h
@@ -102,6 +102,7 @@ class DisplayCutoutHostImpl;
class FindRequestManager;
class JavaScriptDialogManager;
class JavaScriptDialogNavigationDeferrer;
+class ManifestManagerHost;
class MediaWebContentsObserver;
class NFCHost;
class PluginContentOriginAllowlist;
@@ -311,6 +312,10 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
void NotifyManifestUrlChanged(const base::Optional<GURL>& manifest_url);
+ ManifestManagerHost* GetManifestManagerHost() const {
+ return manifest_manager_host_.get();
+ }
+
#if defined(OS_ANDROID)
void SetMainFrameImportance(ChildProcessImportance importance);
#endif
@@ -1897,6 +1902,8 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
std::unique_ptr<ScreenOrientationProvider> screen_orientation_provider_;
+ std::unique_ptr<ManifestManagerHost> manifest_manager_host_;
+
// The accessibility mode for all frames. This is queried when each frame
// is created, and broadcast to all frames when it changes.
ui::AXMode accessibility_mode_;
diff --git b/content/public/browser/render_document_host_user_data.cc a/content/public/browser/render_document_host_user_data.cc
index 3b58bf8a3c5e..b1b385455e61 100644
--- b/content/public/browser/render_document_host_user_data.cc
+++ a/content/public/browser/render_document_host_user_data.cc
@@ -23,8 +23,4 @@ void SetRenderDocumentHostUserData(
key, std::move(data));
}
-void RemoveRenderDocumentHostUserData(RenderFrameHost* rfh, const void* key) {
- static_cast<RenderFrameHostImpl*>(rfh)->RemoveRenderDocumentHostUserData(key);
-}
-
} // namespace content
diff --git b/content/public/browser/render_document_host_user_data.h a/content/public/browser/render_document_host_user_data.h
index a138fd60aa2a..f55f24f60992 100644
--- b/content/public/browser/render_document_host_user_data.h
+++ a/content/public/browser/render_document_host_user_data.h
@@ -22,9 +22,6 @@ CONTENT_EXPORT void SetRenderDocumentHostUserData(
const void* key,
std::unique_ptr<base::SupportsUserData::Data> data);
-CONTENT_EXPORT void RemoveRenderDocumentHostUserData(RenderFrameHost* rfh,
- const void* key);
-
// This class approximates the lifetime of a single blink::Document in the
// browser process. At the moment RenderFrameHost can correspond to multiple
// blink::Documents (when RenderFrameHost is reused for same-process
@@ -85,12 +82,6 @@ class RenderDocumentHostUserData : public base::SupportsUserData::Data {
return static_cast<T*>(GetRenderDocumentHostUserData(rfh, UserDataKey()));
}
- static void DeleteForCurrentDocument(RenderFrameHost* rfh) {
- DCHECK(rfh);
- DCHECK(GetForCurrentDocument(rfh));
- RemoveRenderDocumentHostUserData(rfh, UserDataKey());
- }
-
static const void* UserDataKey() { return &T::kUserDataKey; }
};