Commit 3b636e32 authored by Yuzu Saijo's avatar Yuzu Saijo Committed by Commit Bot

[content] Record UKM events without using WebContents::GetMainFrame()

This CL fixes the potential problem of wrong frame association for
sending UKM.

When we access WebContents::GetMainFrame(), it returns a current frame.
But for DisplayCutoutHostImpl::RecordUKMEvents(), we would like to send
the events to the frame which could have been navigated away.

With this CL, the frame association is included when we queue the events
so that we do not use WebContents::GetMainFrame() any more.

Bug: 1061899
Change-Id: Ib1a9d616812b38098332e86f6604ef837791f459
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2198329Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Reviewed-by: default avatarAlexander Timin <altimin@chromium.org>
Reviewed-by: default avatarSreeja Kamishetty <sreejakshetty@chromium.org>
Commit-Queue: Yuzu Saijo <yuzus@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770051}
parent 17280dfe
...@@ -47,7 +47,7 @@ void DisplayCutoutHostImpl::DidExitFullscreen() { ...@@ -47,7 +47,7 @@ void DisplayCutoutHostImpl::DidExitFullscreen() {
SetCurrentRenderFrameHost(nullptr); SetCurrentRenderFrameHost(nullptr);
} }
void DisplayCutoutHostImpl::DidStartNavigation( void DisplayCutoutHostImpl::DidFinishNavigation(
NavigationHandle* navigation_handle) { NavigationHandle* navigation_handle) {
// If the navigation is not in the main frame or if we are a same document // If the navigation is not in the main frame or if we are a same document
// navigation then we should stop now. // navigation then we should stop now.
...@@ -57,16 +57,6 @@ void DisplayCutoutHostImpl::DidStartNavigation( ...@@ -57,16 +57,6 @@ void DisplayCutoutHostImpl::DidStartNavigation(
} }
RecordPendingUKMEvents(); RecordPendingUKMEvents();
}
void DisplayCutoutHostImpl::DidFinishNavigation(
NavigationHandle* navigation_handle) {
// If the navigation is not in the main frame or if we are a same document
// navigation then we should stop now.
if (!navigation_handle->IsInMainFrame() ||
navigation_handle->IsSameDocument()) {
return;
}
// If we finish a main frame navigation and the |WebDisplayMode| is // If we finish a main frame navigation and the |WebDisplayMode| is
// fullscreen then we should make the main frame the current // fullscreen then we should make the main frame the current
...@@ -182,6 +172,7 @@ void DisplayCutoutHostImpl::MaybeQueueUKMEvent(RenderFrameHost* frame) { ...@@ -182,6 +172,7 @@ void DisplayCutoutHostImpl::MaybeQueueUKMEvent(RenderFrameHost* frame) {
// Adds the UKM event to the list of pending events. // Adds the UKM event to the list of pending events.
PendingUKMEvent pending_event; PendingUKMEvent pending_event;
pending_event.source_id = frame->GetPageUkmSourceId();
pending_event.is_main_frame = !frame->GetParent(); pending_event.is_main_frame = !frame->GetParent();
pending_event.applied_value = applied_value; pending_event.applied_value = applied_value;
pending_event.supplied_value = supplied_value; pending_event.supplied_value = supplied_value;
...@@ -192,12 +183,8 @@ void DisplayCutoutHostImpl::MaybeQueueUKMEvent(RenderFrameHost* frame) { ...@@ -192,12 +183,8 @@ void DisplayCutoutHostImpl::MaybeQueueUKMEvent(RenderFrameHost* frame) {
} }
void DisplayCutoutHostImpl::RecordPendingUKMEvents() { void DisplayCutoutHostImpl::RecordPendingUKMEvents() {
// TODO(crbug.com/1061899): The code here should take an explicit reference
// to the corresponding frame instead of using the current main frame.
for (const auto& event : pending_ukm_events_) { for (const auto& event : pending_ukm_events_) {
ukm::builders::Layout_DisplayCutout_StateChanged builder( ukm::builders::Layout_DisplayCutout_StateChanged builder(event.source_id);
web_contents_impl_->GetMainFrame()->GetPageUkmSourceId());
builder.SetIsMainFrame(event.is_main_frame); builder.SetIsMainFrame(event.is_main_frame);
builder.SetViewportFit_Applied(static_cast<int>(event.applied_value)); builder.SetViewportFit_Applied(static_cast<int>(event.applied_value));
builder.SetViewportFit_Supplied(static_cast<int>(event.supplied_value)); builder.SetViewportFit_Supplied(static_cast<int>(event.supplied_value));
......
...@@ -27,7 +27,6 @@ class DisplayCutoutHostImpl : public blink::mojom::DisplayCutoutHost { ...@@ -27,7 +27,6 @@ class DisplayCutoutHostImpl : public blink::mojom::DisplayCutoutHost {
// Called by WebContents when various events occur. // Called by WebContents when various events occur.
void DidAcquireFullscreen(RenderFrameHost* rfh); void DidAcquireFullscreen(RenderFrameHost* rfh);
void DidExitFullscreen(); void DidExitFullscreen();
void DidStartNavigation(NavigationHandle* navigation_handle);
void DidFinishNavigation(NavigationHandle* navigation_handle); void DidFinishNavigation(NavigationHandle* navigation_handle);
void RenderFrameDeleted(RenderFrameHost* rfh); void RenderFrameDeleted(RenderFrameHost* rfh);
void RenderFrameCreated(RenderFrameHost* rfh); void RenderFrameCreated(RenderFrameHost* rfh);
...@@ -39,6 +38,7 @@ class DisplayCutoutHostImpl : public blink::mojom::DisplayCutoutHost { ...@@ -39,6 +38,7 @@ class DisplayCutoutHostImpl : public blink::mojom::DisplayCutoutHost {
private: private:
// Stores the data for a pending UKM event. // Stores the data for a pending UKM event.
struct PendingUKMEvent { struct PendingUKMEvent {
ukm::SourceId source_id;
bool is_main_frame; bool is_main_frame;
blink::mojom::ViewportFit applied_value; blink::mojom::ViewportFit applied_value;
blink::mojom::ViewportFit supplied_value; blink::mojom::ViewportFit supplied_value;
......
...@@ -4301,9 +4301,6 @@ void WebContentsImpl::DidStartNavigation(NavigationHandle* navigation_handle) { ...@@ -4301,9 +4301,6 @@ void WebContentsImpl::DidStartNavigation(NavigationHandle* navigation_handle) {
for (auto& observer : observers_) for (auto& observer : observers_)
observer.DidStartNavigation(navigation_handle); observer.DidStartNavigation(navigation_handle);
if (display_cutout_host_impl_)
display_cutout_host_impl_->DidStartNavigation(navigation_handle);
if (navigation_handle->IsInMainFrame()) { if (navigation_handle->IsInMainFrame()) {
// When the browser is started with about:blank as the startup URL, focus // When the browser is started with about:blank as the startup URL, focus
// the location bar (which will also select its contents) so people can // the location bar (which will also select its contents) so people can
......
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