Skip to content

Commit

Permalink
Rename DocumentAvailableInMainFrame to PrimaryMainDocumentElementAvai…
Browse files Browse the repository at this point in the history
…lable

In crrev.com/c/3230354, we landed the change where we only dispatch
DocumentAvailableInMainFrame for the primary main frame. This
change makes it straightforward for the API users to interpret that
the observer is only invoked for primary main frames.

We do mass rename of the DocumentAvailableInMainFrame use cases and also
remove the RenderFrameHost* parameter, which is not necessary as it can
be obtained directly from web_contents()->GetMainFrame().

The following changes were made using tools/git/mffr.py:
- DocumentAvailableInMainFrame(RenderFrameHost* render_frame_host)
     -> PrimaryMainDocumentElementAvailable
- DocumentAvailableInMainFrame(bool uses_temporary_zoom_level)
     -> MainDocumentElementAvailable(bool uses_temporary_zoom_level);
- is_document_available_in_main_document()
     -> is_main_document_element_available()

For more context, please refer to:
https://docs.google.com/document/d/1GjuE0wXRj304q61T_HFLj3CRV1mNqMWfYIz_8tKdEZY/edit?usp=sharing.

BUG= 1288029, 1237422

Change-Id: If46e69d8653b0993645887d945b4c5c05c3801c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3395378
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Owners-Override: Alexander Timin <altimin@chromium.org>
Commit-Queue: Sreeja Kamishetty <sreejakshetty@chromium.org>
Cr-Commit-Position: refs/heads/main@{#964662}
  • Loading branch information
sreejakshetty authored and Chromium LUCI CQ committed Jan 28, 2022
1 parent 86151c7 commit 4978330
Show file tree
Hide file tree
Showing 50 changed files with 146 additions and 183 deletions.
2 changes: 1 addition & 1 deletion PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -5485,7 +5485,7 @@ def CheckMPArchApiUsage(input_api, output_api):
'RenderViewReady',
'RenderViewDeleted',
'RenderViewHostChanged',
'DocumentAvailableInMainFrame',
'PrimaryMainDocumentElementAvailable',
'DocumentOnLoadCompletedInPrimaryMainFrame',
'DOMContentLoaded',
'DidFinishLoad',
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/offline_pages/background_loader_offliner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,11 @@ void BackgroundLoaderOffliner::MarkLoadStartTime() {
load_start_time_ = base::TimeTicks::Now();
}

void BackgroundLoaderOffliner::DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) {
void BackgroundLoaderOffliner::PrimaryMainDocumentElementAvailable() {
is_low_bar_met_ = true;

// Add this signal to signal_data_.
AddLoadingSignal("DocumentAvailableInMainFrame");
AddLoadingSignal("PrimaryMainDocumentElementAvailable");
}

void BackgroundLoaderOffliner::DocumentOnLoadCompletedInPrimaryMainFrame() {
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/offline_pages/background_loader_offliner.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ class BackgroundLoaderOffliner
void CanDownload(base::OnceCallback<void(bool)> callback) override;

// WebContentsObserver implementation.
void DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;
void DocumentOnLoadCompletedInPrimaryMainFrame() override;
void PrimaryMainFrameRenderProcessGone(
base::TerminationStatus status) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,7 @@ class BackgroundLoaderOfflinerTest : public testing::Test {
offliner_->SetBackgroundSnapshotControllerForTest(
std::move(snapshot_controller));
// Call complete loading.
auto* main_frame = offliner()->web_contents()->GetMainFrame();
offliner()->DocumentAvailableInMainFrame(main_frame);
offliner()->PrimaryMainDocumentElementAvailable();
offliner()->DocumentOnLoadCompletedInPrimaryMainFrame();
PumpLoop();
}
Expand Down Expand Up @@ -842,8 +841,7 @@ TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBarStartedTriesMet) {
EXPECT_TRUE(offliner()->LoadAndSave(request, completion_callback(),
progress_callback()));
// Guarantees low bar for saving is met.
auto* main_frame = offliner()->web_contents()->GetMainFrame();
offliner()->DocumentAvailableInMainFrame(main_frame);
offliner()->PrimaryMainDocumentElementAvailable();
// Timeout
EXPECT_TRUE(offliner()->HandleTimeout(kRequestId));
EXPECT_TRUE(SaveInProgress());
Expand All @@ -860,8 +858,7 @@ TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBarCompletedTriesMet) {
EXPECT_TRUE(offliner()->LoadAndSave(request, completion_callback(),
progress_callback()));
// Guarantees low bar for saving is met.
auto* main_frame = offliner()->web_contents()->GetMainFrame();
offliner()->DocumentAvailableInMainFrame(main_frame);
offliner()->PrimaryMainDocumentElementAvailable();
// Timeout
EXPECT_TRUE(offliner()->HandleTimeout(kRequestId));
EXPECT_TRUE(SaveInProgress());
Expand Down Expand Up @@ -902,8 +899,7 @@ TEST_F(BackgroundLoaderOfflinerTest, HandleTimeoutWithLowBarNoRetryLimit) {
EXPECT_TRUE(offliner()->LoadAndSave(request, completion_callback(),
progress_callback()));
// Sets lowbar.
auto* main_frame = offliner()->web_contents()->GetMainFrame();
offliner()->DocumentAvailableInMainFrame(main_frame);
offliner()->PrimaryMainDocumentElementAvailable();
// Timeout
EXPECT_FALSE(offliner()->HandleTimeout(kRequestId));
EXPECT_FALSE(SaveInProgress());
Expand Down
5 changes: 2 additions & 3 deletions chrome/browser/offline_pages/recent_tab_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -270,10 +270,9 @@ void RecentTabHelper::DidFinishNavigation(
<< " - Page can not be saved by last_n";
}

void RecentTabHelper::DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) {
void RecentTabHelper::PrimaryMainDocumentElementAvailable() {
EnsureInitialized();
snapshot_controller_->DocumentAvailableInMainFrame();
snapshot_controller_->PrimaryMainDocumentElementAvailable();
}

void RecentTabHelper::DocumentOnLoadCompletedInPrimaryMainFrame() {
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/offline_pages/recent_tab_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ class RecentTabHelper
// content::WebContentsObserver
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;
void DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;
void DocumentOnLoadCompletedInPrimaryMainFrame() override;
void WebContentsDestroyed() override;
void OnVisibilityChanged(content::Visibility visibility) override;
Expand Down
14 changes: 7 additions & 7 deletions chrome/browser/offline_pages/recent_tab_helper_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ TEST_F(RecentTabHelperTest, TwoCapturesSamePageLoad) {
NavigateAndCommit(kTestUrl);

// Set page loading state to the 1st snapshot-able stage. No capture so far.
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
EXPECT_EQ(0U, page_added_count());

Expand Down Expand Up @@ -516,7 +516,7 @@ TEST_F(RecentTabHelperTest, DISABLED_TwoCapturesWhere2ndFailsSamePageLoad) {
// Navigate and load until the 1st stage. Tab hidden should trigger a capture.
const GURL kTestUrl("http://mystery.site/foo.html");
NavigateAndCommit(kTestUrl);
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
recent_tab_helper()->OnVisibilityChanged(content::Visibility::HIDDEN);
RunUntilIdle();
Expand Down Expand Up @@ -567,7 +567,7 @@ TEST_F(RecentTabHelperTest, TwoCapturesDifferentPageLoadsSameUrl) {
// Reload the same URL until the page is minimally loaded. The previous
// snapshot should have been removed.
NavigateAndCommitTyped(kTestUrl);
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
EXPECT_EQ(1U, page_added_count());
EXPECT_EQ(1U, model_removed_count());
Expand Down Expand Up @@ -671,7 +671,7 @@ TEST_F(RecentTabHelperTest, TwoLastNAndTwoDownloadCapturesSamePage) {
// check that two last_n snapshots were created but only one was kept.
const GURL kTestUrl("http://mystery.site/foo.html");
NavigateAndCommit(kTestUrl);
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
recent_tab_helper()->OnVisibilityChanged(content::Visibility::HIDDEN);
RunUntilIdle();
Expand Down Expand Up @@ -754,7 +754,7 @@ TEST_F(RecentTabHelperTest, DownloadRequestEarlyInLoad) {
ASSERT_EQ(0U, GetAllPages().size());

// Minimally load the page. First capture should occur.
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
ASSERT_EQ(1U, GetAllPages().size());
const OfflinePageItem& early_page = GetAllPages()[0];
Expand Down Expand Up @@ -782,7 +782,7 @@ TEST_F(RecentTabHelperTest, DownloadRequestEarlyInLoad) {
TEST_F(RecentTabHelperTest, DownloadRequestLaterInLoad) {
const GURL kTestUrl("http://mystery.site/foo.html");
NavigateAndCommit(kTestUrl);
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
ASSERT_EQ(0U, GetAllPages().size());

Expand Down Expand Up @@ -884,7 +884,7 @@ TEST_F(RecentTabHelperTest, SimultaneousCapturesFromLastNAndDownloads) {
// signals are poor signals for those).
TEST_F(RecentTabHelperTest, DuplicateTabHiddenEventsShouldTriggerNewSnapshots) {
NavigateAndCommit(GURL("http://mystery.site/foo.html"));
recent_tab_helper()->DocumentAvailableInMainFrame(main_rfh());
recent_tab_helper()->PrimaryMainDocumentElementAvailable();
FastForwardSnapshotController();
recent_tab_helper()->OnVisibilityChanged(content::Visibility::HIDDEN);
RunUntilIdle();
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/views/reader_mode/reader_mode_icon_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ void ReaderModeIconView::ReadyToCommitNavigation(
GetPageType(web_contents));
}

void ReaderModeIconView::DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) {
void ReaderModeIconView::PrimaryMainDocumentElementAvailable() {
content::WebContents* web_contents = GetWebContents();
if (!web_contents)
return;
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/views/reader_mode/reader_mode_icon_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ class ReaderModeIconView : public PageActionIconView,
content::NavigationHandle* navigation_handle) override;
void ReadyToCommitNavigation(
content::NavigationHandle* navigation_handle) override;
void DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;

// PageActionIconView overrides:
void UpdateImpl() override;
Expand Down
3 changes: 1 addition & 2 deletions components/autofill_assistant/browser/controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1174,8 +1174,7 @@ void Controller::DidFinishNavigation(
}
}

void Controller::DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) {
void Controller::PrimaryMainDocumentElementAvailable() {
OnUrlChange();
}

Expand Down
3 changes: 1 addition & 2 deletions components/autofill_assistant/browser/controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,7 @@ class Controller : public ScriptExecutorDelegate,
content::NavigationHandle* navigation_handle) override;
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;
void DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;
void PrimaryMainFrameRenderProcessGone(
base::TerminationStatus status) override;
void OnWebContentsFocused(
Expand Down
2 changes: 1 addition & 1 deletion components/offline_pages/core/snapshot_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void SnapshotController::PendingSnapshotCompleted() {
state_ = State::READY;
}

void SnapshotController::DocumentAvailableInMainFrame() {
void SnapshotController::PrimaryMainDocumentElementAvailable() {
DCHECK_EQ(PageQuality::POOR, current_page_quality_);
// Post a delayed task to snapshot.
task_runner_->PostDelayedTask(
Expand Down
4 changes: 2 additions & 2 deletions components/offline_pages/core/snapshot_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ class SnapshotController {
// now completed (so the next one can be started).
void PendingSnapshotCompleted();

// Invoked from WebContentObserver::DocumentAvailableInMainFrame
void DocumentAvailableInMainFrame();
// Invoked from WebContentObserver::PrimaryMainDocumentElementAvailable
void PrimaryMainDocumentElementAvailable();

// Invoked from WebContentObserver::DocumentOnLoadCompletedInPrimaryMainFrame
void DocumentOnLoadCompletedInPrimaryMainFrame();
Expand Down
14 changes: 7 additions & 7 deletions components/offline_pages/core/snapshot_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ TEST_F(SnapshotControllerTest, OnLoad) {
TEST_F(SnapshotControllerTest, OnDocumentAvailable) {
EXPECT_GT(controller()->GetDelayAfterDocumentAvailableForTest(), 0LL);
// OnDOM should make snapshot after a delay.
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();
PumpLoop();
EXPECT_EQ(0, snapshot_count());
FastForwardBy(base::Milliseconds(
Expand All @@ -100,7 +100,7 @@ TEST_F(SnapshotControllerTest, OnLoadSnapshotIsTheLastOne) {
EXPECT_GT(controller()->GetDelayAfterDocumentAvailableForTest(),
controller()->GetDelayAfterDocumentOnLoadCompletedForTest());
// OnDOM should make snapshot after a delay.
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();
PumpLoop();
EXPECT_EQ(0, snapshot_count());
controller()->DocumentOnLoadCompletedInPrimaryMainFrame();
Expand All @@ -119,7 +119,7 @@ TEST_F(SnapshotControllerTest, OnLoadSnapshotIsTheLastOne) {

TEST_F(SnapshotControllerTest, OnLoadSnapshotAfterLongDelay) {
// OnDOM should make snapshot after a delay.
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();
PumpLoop();
EXPECT_EQ(0, snapshot_count());
FastForwardBy(base::Milliseconds(
Expand All @@ -136,7 +136,7 @@ TEST_F(SnapshotControllerTest, OnLoadSnapshotAfterLongDelay) {

TEST_F(SnapshotControllerTest, Stop) {
// OnDOM should make snapshot after a delay.
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();
PumpLoop();
EXPECT_EQ(0, snapshot_count());
controller()->Stop();
Expand All @@ -150,7 +150,7 @@ TEST_F(SnapshotControllerTest, Stop) {
}

TEST_F(SnapshotControllerTest, ClientReset) {
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();

controller()->Reset();
FastForwardBy(base::Milliseconds(
Expand All @@ -163,7 +163,7 @@ TEST_F(SnapshotControllerTest, ClientReset) {
EXPECT_EQ(1, snapshot_count());

controller()->Reset();
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();
FastForwardBy(base::Milliseconds(
controller()->GetDelayAfterDocumentAvailableForTest()));
// No snapshot since session was reset.
Expand All @@ -181,7 +181,7 @@ TEST_F(SnapshotControllerTest, ClientResetWhileSnapshotting) {
controller()->Reset();
controller()->PendingSnapshotCompleted();
// Next snapshot should be initiated when new document is loaded.
controller()->DocumentAvailableInMainFrame();
controller()->PrimaryMainDocumentElementAvailable();
FastForwardBy(base::Milliseconds(
controller()->GetDelayAfterDocumentAvailableForTest()));
// No snapshot since session was reset.
Expand Down
5 changes: 3 additions & 2 deletions components/thin_webview/internal/thin_webview.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ void ThinWebView::Destroy(JNIEnv* env, const JavaParamRef<jobject>& object) {
delete this;
}

void ThinWebView::DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) {
// TODO(crbug.com/1291556): Use WebContentsObserver::PrimaryPageChanged to
// disable thin webview.
void ThinWebView::PrimaryMainDocumentElementAvailable() {
// Disable browser controls when used for thin webview.
web_contents_->UpdateBrowserControlsState(cc::BrowserControlsState::kHidden,
cc::BrowserControlsState::kHidden,
Expand Down
3 changes: 1 addition & 2 deletions components/thin_webview/internal/thin_webview.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ class ThinWebView : public content::WebContentsObserver {

private:
// content::WebContentsObserver overrides:
void DocumentAvailableInMainFrame(
content::RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;

void SetWebContents(
content::WebContents* web_contents,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ void GinJavaBridgeDispatcherHost::RemoveNamedObject(
// As the object isn't going to be removed from the JavaScript side until the
// next page reload, calls to it must still work, thus we should continue to
// hold it. All the transient objects and removed named objects will be purged
// during the cleansing caused by DocumentAvailableInMainFrame event.
// during the cleansing caused by PrimaryMainDocumentElementAvailable event.

// We should include pending RenderFrameHosts, otherwise they will miss the
// chance when calling add or remove methods when they are created but not
Expand All @@ -298,8 +298,7 @@ void GinJavaBridgeDispatcherHost::SetAllowObjectContentsInspection(bool allow) {
allow_object_contents_inspection_ = allow;
}

void GinJavaBridgeDispatcherHost::DocumentAvailableInMainFrame(
RenderFrameHost* render_frame_host) {
void GinJavaBridgeDispatcherHost::PrimaryMainDocumentElementAvailable() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
// Called when the window object has been cleared in the main frame.
// That means, all sub-frames have also been cleared, so only named
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ class GinJavaBridgeDispatcherHost

// WebContentsObserver
void RenderFrameCreated(RenderFrameHost* render_frame_host) override;
void DocumentAvailableInMainFrame(
RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;
void WebContentsDestroyed() override;
void RenderViewHostChanged(RenderViewHost* old_host,
RenderViewHost* new_host) override;
Expand Down
5 changes: 3 additions & 2 deletions content/browser/android/web_contents_observer_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,10 @@ void WebContentsObserverProxy::DidChangeVisibleSecurityState() {
AttachCurrentThread(), java_observer_);
}

void WebContentsObserverProxy::DocumentAvailableInMainFrame(
RenderFrameHost* render_frame_host) {
void WebContentsObserverProxy::PrimaryMainDocumentElementAvailable() {
JNIEnv* env = AttachCurrentThread();
// TODO(crbug.com/1288029): Rename DocumentAvailableInMainFrame to
// PrimaryMainDocumentElementAvailable in java code.
Java_WebContentsObserverProxy_documentAvailableInMainFrame(env,
java_observer_);
}
Expand Down
3 changes: 1 addition & 2 deletions content/browser/android/web_contents_observer_proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ class WebContentsObserverProxy : public WebContentsObserver {
const GURL& validated_url,
int error_code) override;
void DidChangeVisibleSecurityState() override;
void DocumentAvailableInMainFrame(
RenderFrameHost* render_frame_host) override;
void PrimaryMainDocumentElementAvailable() override;
void DidFirstVisuallyNonEmptyPaint() override;
void OnVisibilityChanged(content::Visibility visibility) override;
void TitleWasSet(NavigationEntry* entry) override;
Expand Down
Loading

0 comments on commit 4978330

Please sign in to comment.