Skip to content

Commit

Permalink
Rename to_different_document to should_show_loading_ui in LoadingStat…
Browse files Browse the repository at this point in the history
…eChanged() callbacks

Currently, the purpose of this bit is to indicate to embedders whether
or not a navigation should show loading UI indicators. Currently, same
document navigations never show loading UI because they're always
processed synchronously, so there's no point in flipping to showing
loading UI only to immediately turn it off again. However, in a future
patch, we will introduce a new asynchronous kind of same-document
navigation (as part of the new appHistory API). In preparation for
that, rename to_different_document to should_show_loading_ui to better
indicate what the bit means and how it should be used.

Bug: 1241202
Change-Id: I8c890da3571b80b361644fdf8fbb366336a09f1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3268574
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Owners-Override: Charlie Reis <creis@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#940997}
  • Loading branch information
natechapin authored and Chromium LUCI CQ committed Nov 12, 2021
1 parent 9872337 commit 9aabf5f
Show file tree
Hide file tree
Showing 59 changed files with 142 additions and 137 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public OverlayPanelContent(@NonNull OverlayContentDelegate contentDelegate,
private boolean mIsFullscreen;

@Override
public void loadingStateChanged(boolean toDifferentDocument) {
public void loadingStateChanged(boolean shouldShowLoadingUI) {
boolean isLoading = mWebContents != null && mWebContents.isLoading();
if (isLoading) {
mProgressObserver.onProgressBarStarted();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public boolean shouldCreateWebContents(GURL targetUrl) {
}

@Override
public void loadingStateChanged(boolean toDifferentDocument) {
public void loadingStateChanged(boolean shouldShowLoadingUI) {
boolean isLoading = mWebContents != null && mWebContents.isLoading();
if (isLoading) {
if (mSheetContent == null) return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,7 @@ public void initialize(Tab parent, @Nullable @TabCreationState Integer creationS

initWebContents(webContents);

if (!creatingWebContents && webContents.isLoadingToDifferentDocument()) {
if (!creatingWebContents && webContents.shouldShowLoadingUI()) {
didStartPageLoad(webContents.getVisibleUrl());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,14 @@ public boolean addMessageToConsole(int level, String message, int lineNumber, St
}

@Override
public void loadingStateChanged(boolean toDifferentDocument) {
public void loadingStateChanged(boolean shouldShowLoadingUI) {
boolean isLoading = mTab.getWebContents() != null && mTab.getWebContents().isLoading();
if (isLoading) {
mTab.onLoadStarted(toDifferentDocument);
mTab.onLoadStarted(shouldShowLoadingUI);
} else {
mTab.onLoadStopped();
}
mDelegate.loadingStateChanged(toDifferentDocument);
mDelegate.loadingStateChanged(shouldShowLoadingUI);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ash/login/ui/simple_web_view_dialog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,9 @@ void SimpleWebViewDialog::NavigationStateChanged(
}

void SimpleWebViewDialog::LoadingStateChanged(WebContents* source,
bool to_different_document) {
bool should_show_loading_ui) {
bool is_loading = source->IsLoading();
UpdateReload(is_loading && to_different_document, false);
UpdateReload(is_loading && should_show_loading_ui, false);
command_updater_->UpdateCommandEnabled(IDC_STOP, is_loading);
}

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ash/login/ui/simple_web_view_dialog.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class SimpleWebViewDialog : public views::View,
void NavigationStateChanged(content::WebContents* source,
content::InvalidateTypes changed_flags) override;
void LoadingStateChanged(content::WebContents* source,
bool to_different_document) override;
bool should_show_loading_ui) override;

// Implements LocationBarView::Delegate:
content::WebContents* GetWebContents() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public boolean shouldCreateWebContents(GURL targetUrl) {
}

@Override
public void loadingStateChanged(boolean toDifferentDocument) {
public void loadingStateChanged(boolean shouldShowLoadingUI) {
boolean isLoading = mWebContents != null && mWebContents.isLoading();
if (isLoading) {
if (mToolbarModel == null) return;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/favicon/favicon_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ gfx::Image TabFaviconFromWebContents(content::WebContents* contents) {
gfx::Image favicon = favicon_driver->GetFavicon();

// Desaturate the favicon if the navigation entry contains a network error.
if (!contents->IsLoadingToDifferentDocument()) {
if (!contents->ShouldShowLoadingUI()) {
content::NavigationController& controller = contents->GetController();

content::NavigationEntry* entry = controller.GetLastCommittedEntry();
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/resource_coordinator/tab_load_tracker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,8 @@ void TabLoadTracker::StopTracking(content::WebContents* web_contents) {
void TabLoadTracker::PrimaryPageChanged(content::WebContents* web_contents) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// Only observe top-level navigation to a different document.
if (!web_contents->IsLoadingToDifferentDocument())
// Only observe top-level navigation that triggers navigation UI.
if (!web_contents->ShouldShowLoadingUI())
return;

auto it = tabs_.find(web_contents);
Expand Down Expand Up @@ -225,7 +225,7 @@ TabLoadTracker::LoadingState TabLoadTracker::DetermineLoadingState(
// Determine if the WebContents is actively loading, using our definition of
// loading. Start from the assumption that it is UNLOADED.
LoadingState loading_state = UNLOADED;
if (web_contents->IsLoadingToDifferentDocument() &&
if (web_contents->ShouldShowLoadingUI() &&
!web_contents->IsWaitingForResponse()) {
loading_state = LOADING;
} else {
Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1653,9 +1653,9 @@ void Browser::ActivateContents(WebContents* contents) {
}

void Browser::LoadingStateChanged(WebContents* source,
bool to_different_document) {
bool should_show_loading_ui) {
ScheduleUIUpdate(source, content::INVALIDATE_TYPE_LOAD);
UpdateWindowForLoadingStateChanged(source, to_different_document);
UpdateWindowForLoadingStateChanged(source, should_show_loading_ui);
}

void Browser::CloseContents(WebContents* source) {
Expand Down Expand Up @@ -2762,13 +2762,13 @@ void Browser::TabDetachedAtImpl(content::WebContents* contents,
}

void Browser::UpdateWindowForLoadingStateChanged(content::WebContents* source,
bool to_different_document) {
bool should_show_loading_ui) {
window_->UpdateLoadingAnimations(tab_strip_model_->TabsAreLoading());
window_->UpdateTitleBar();

WebContents* selected_contents = tab_strip_model_->GetActiveWebContents();
if (source == selected_contents) {
bool is_loading = source->IsLoading() && to_different_document;
bool is_loading = source->IsLoading() && should_show_loading_ui;
command_controller_->LoadingStateChanged(is_loading, false);
if (GetStatusBubble()) {
GetStatusBubble()->SetStatus(CoreTabHelper::FromWebContents(
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ui/browser.h
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ class Browser : public TabStripModelObserver,
bool* was_blocked) override;
void ActivateContents(content::WebContents* contents) override;
void LoadingStateChanged(content::WebContents* source,
bool to_different_document) override;
bool should_show_loading_ui) override;
void CloseContents(content::WebContents* source) override;
void SetContentsBounds(content::WebContents* source,
const gfx::Rect& bounds) override;
Expand Down Expand Up @@ -1036,7 +1036,7 @@ class Browser : public TabStripModelObserver,

// Updates the loading state for the window and tabstrip.
void UpdateWindowForLoadingStateChanged(content::WebContents* source,
bool to_different_document);
bool should_show_loading_ui);

// Shared code between Reload() and ReloadBypassingCache().
void ReloadInternal(WindowOpenDisposition disposition, bool bypass_cache);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/tabs/tab_network_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
TabNetworkState TabNetworkStateForWebContents(content::WebContents* contents) {
DCHECK(contents);

if (!contents->IsLoadingToDifferentDocument()) {
if (!contents->ShouldShowLoadingUI()) {
content::NavigationEntry* entry =
contents->GetController().GetLastCommittedEntry();
if (entry && (entry->GetPageType() == content::PAGE_TYPE_ERROR))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ public void onPageLoadFinished(Tab tab, GURL url) {
private static boolean loadComplete(Tab tab, String url) {
return !tab.isLoading()
&& (url == null || TextUtils.equals(getUrlStringOnUiThread(tab), url))
&& !tab.getWebContents().isLoadingToDifferentDocument();
&& !tab.getWebContents().shouldShowLoadingUI();
}

public static String getTitleOnUiThread(Tab tab) {
Expand Down Expand Up @@ -243,7 +243,7 @@ public static void waitForTabPageLoaded(final Tab tab, @Nullable final String ur
+ "loading: %b, web contents init: %b, web contents loading: %b",
url, getUrlStringOnUiThread(tab), Math.round(100 * tab.getProgress()),
tab.isLoading(), webContents != null,
webContents == null ? false : webContents.isLoadingToDifferentDocument()));
webContents == null ? false : webContents.shouldShowLoadingUI()));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,11 @@ void WebContentsDelegateAndroid::ActivateContents(WebContents* contents) {

void WebContentsDelegateAndroid::LoadingStateChanged(
WebContents* source,
bool to_different_document) {
bool should_show_loading_ui) {
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = GetJavaDelegate(env);
Java_WebContentsDelegateAndroid_loadingStateChanged(env, obj,
to_different_document);
should_show_loading_ui);
}

void WebContentsDelegateAndroid::RendererUnresponsive(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class WebContentsDelegateAndroid : public content::WebContentsDelegate {
void VisibleSecurityStateChanged(content::WebContents* source) override;
void ActivateContents(content::WebContents* contents) override;
void LoadingStateChanged(content::WebContents* source,
bool to_different_document) override;
bool should_show_loading_ui) override;
void RendererUnresponsive(
content::WebContents* source,
content::RenderWidgetHost* render_widget_host,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public void activateContents() {}
public void closeContents() {}

@CalledByNative
public void loadingStateChanged(boolean toDifferentDocument) {}
public void loadingStateChanged(boolean shouldShowLoadingUI) {}

@CalledByNative
public void navigationStateChanged(int flags) {}
Expand Down
4 changes: 2 additions & 2 deletions components/guest_view/browser/guest_view_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -631,12 +631,12 @@ bool GuestViewBase::HandleKeyboardEvent(
}

void GuestViewBase::LoadingStateChanged(WebContents* source,
bool to_different_document) {
bool should_show_loading_ui) {
if (!attached() || !embedder_web_contents()->GetDelegate())
return;

embedder_web_contents()->GetDelegate()->LoadingStateChanged(
embedder_web_contents(), to_different_document);
embedder_web_contents(), should_show_loading_ui);
}

void GuestViewBase::ResizeDueToAutoResize(WebContents* web_contents,
Expand Down
2 changes: 1 addition & 1 deletion components/guest_view/browser/guest_view_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ class GuestViewBase : public content::BrowserPluginGuestDelegate,
bool exited) final;
void ContentsZoomChange(bool zoom_in) final;
void LoadingStateChanged(content::WebContents* source,
bool to_different_document) final;
bool should_show_loading_ui) final;
void ResizeDueToAutoResize(content::WebContents* web_contents,
const gfx::Size& new_size) final;
void RunFileChooser(content::RenderFrameHost* render_frame_host,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class PageLoadTrackerDecoratorHelper::WebContentsObserver

// |web_contents| must not be loading when it starts being tracked by this
// observer. Otherwise, loading state wouldn't be tracked correctly.
DCHECK(!web_contents->IsLoadingToDifferentDocument());
DCHECK(!web_contents->ShouldShowLoadingUI());
}

WebContentsObserver(const WebContentsObserver&) = delete;
Expand All @@ -76,7 +76,7 @@ class PageLoadTrackerDecoratorHelper::WebContentsObserver
DCHECK_EQ(loading_state_, LoadingState::kNotLoading);

// Only observe top-level navigation to a different document.
if (!web_contents()->IsLoadingToDifferentDocument())
if (!web_contents()->ShouldShowLoadingUI())
return;

loading_state_ = LoadingState::kWaitingForNavigation;
Expand All @@ -88,8 +88,8 @@ class PageLoadTrackerDecoratorHelper::WebContentsObserver
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// Only observe top-level navigation to a different document.
if (!web_contents()->IsLoadingToDifferentDocument())
// Only observe top-level navigation that will show navigation UI.
if (!web_contents()->ShouldShowLoadingUI())
return;

DCHECK(web_contents()->IsLoading());
Expand Down
2 changes: 1 addition & 1 deletion components/webapps/browser/banners/app_banner_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ void AppBannerManager::DidActivatePortal(
// If this page was loaded in a portal, AppBannerManager may have been
// instantiated after DidFinishLoad. Trigger the banner pipeline now (on
// portal activation) if we missed the load event.
if (!load_finished_ && !web_contents()->IsLoadingToDifferentDocument()) {
if (!load_finished_ && !web_contents()->ShouldShowLoadingUI()) {
DidFinishLoad(web_contents()->GetMainFrame(),
web_contents()->GetLastCommittedURL());
}
Expand Down
2 changes: 1 addition & 1 deletion content/browser/fenced_frame/fenced_frame.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class CONTENT_EXPORT FencedFrame : public blink::mojom::FencedFrameOwnerHost,

// FrameTree::Delegate.
void DidStartLoading(FrameTreeNode* frame_tree_node,
bool to_different_document) override {}
bool should_show_loading_ui) override {}
void DidStopLoading() override;
void DidChangeLoadProgress() override {}
bool IsHidden() override;
Expand Down
2 changes: 1 addition & 1 deletion content/browser/portal/portal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ void Portal::WebContentsDestroyed() {
}

void Portal::LoadingStateChanged(WebContents* source,
bool to_different_document) {
bool should_show_loading_ui) {
DCHECK_EQ(source, portal_contents_.get());
if (!source->IsLoading())
client_->DispatchLoadEvent();
Expand Down
2 changes: 1 addition & 1 deletion content/browser/portal/portal.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class CONTENT_EXPORT Portal : public blink::mojom::Portal,

// WebContentsDelegate overrides.
void LoadingStateChanged(WebContents* source,
bool to_different_document) override;
bool should_show_loading_ui) override;
void PortalWebContentsCreated(WebContents* portal_web_contents) override;
void CloseContents(WebContents*) override;
WebContents* GetResponsibleWebContents(WebContents* web_contents) override;
Expand Down
2 changes: 1 addition & 1 deletion content/browser/prerender/prerender_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class PrerenderHost::PageHolder : public FrameTree::Delegate,
// distinguish between the different FrameTrees.

void DidStartLoading(FrameTreeNode* frame_tree_node,
bool to_different_document) override {}
bool should_show_loading_ui) override {}

void DidStopLoading() override {
if (on_wait_loading_finished_) {
Expand Down
4 changes: 2 additions & 2 deletions content/browser/renderer_host/frame_tree.cc
Original file line number Diff line number Diff line change
Expand Up @@ -806,13 +806,13 @@ void FrameTree::DidAccessInitialMainDocument() {
}

void FrameTree::DidStartLoadingNode(FrameTreeNode& node,
bool to_different_document,
bool should_show_loading_ui,
bool was_previously_loading) {
if (was_previously_loading)
return;

root()->render_manager()->SetIsLoading(IsLoading());
delegate_->DidStartLoading(&node, to_different_document);
delegate_->DidStartLoading(&node, should_show_loading_ui);
}

void FrameTree::DidStopLoadingNode(FrameTreeNode& node) {
Expand Down
8 changes: 4 additions & 4 deletions content/browser/renderer_host/frame_tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,11 @@ class CONTENT_EXPORT FrameTree {
// A RenderFrameHost in the specified |frame_tree_node| started loading a
// new document. This corresponds to browser UI starting to show a spinner
// or other visual indicator for loading. This method is only invoked if the
// FrameTree hadn't been previously loading. |to_different_document| will be
// true unless the load is a fragment navigation, or triggered by
// FrameTree hadn't been previously loading. |should_show_loading_ui| will
// be true unless the load is a fragment navigation, or triggered by
// history.pushState/replaceState.
virtual void DidStartLoading(FrameTreeNode* frame_tree_node,
bool to_different_document) = 0;
bool should_show_loading_ui) = 0;

// This is called when all nodes in the FrameTree stopped loading. This
// corresponds to the browser UI stop showing a spinner or other visual
Expand Down Expand Up @@ -383,7 +383,7 @@ class CONTENT_EXPORT FrameTree {
void FrameRemoved(FrameTreeNode* frame);

void DidStartLoadingNode(FrameTreeNode& node,
bool to_different_document,
bool should_show_loading_ui,
bool was_previously_loading);
void DidStopLoadingNode(FrameTreeNode& node);
void DidCancelLoading();
Expand Down
8 changes: 4 additions & 4 deletions content/browser/renderer_host/frame_tree_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -555,13 +555,13 @@ void FrameTreeNode::ResetNavigationRequest(bool keep_state) {
render_manager_.CleanUpNavigation();
}

void FrameTreeNode::DidStartLoading(bool to_different_document,
void FrameTreeNode::DidStartLoading(bool should_show_loading_ui,
bool was_previously_loading) {
TRACE_EVENT2("navigation", "FrameTreeNode::DidStartLoading",
"frame_tree_node", frame_tree_node_id(), "to different document",
to_different_document);
"frame_tree_node", frame_tree_node_id(),
"should_show_loading_ui ", should_show_loading_ui);

frame_tree_->DidStartLoadingNode(*this, to_different_document,
frame_tree_->DidStartLoadingNode(*this, should_show_loading_ui,
was_previously_loading);

// Set initial load progress and update overall progress. This will notify
Expand Down
8 changes: 5 additions & 3 deletions content/browser/renderer_host/frame_tree_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -327,13 +327,15 @@ class CONTENT_EXPORT FrameTreeNode {
void ResetNavigationRequest(bool keep_state);

// A RenderFrameHost in this node started loading.
// |to_different_document| will be true unless the load is a fragment
// navigation, or triggered by history.pushState/replaceState.
// |should_show_loading_ui| indicates whether this navigation should be
// visible in the UI. True for cross-document navigations and navigations
// intercepted by appHistory's transitionWhile().
// |was_previously_loading| is false if the FrameTree was not loading before.
// The caller is required to provide this boolean as the delegate should only
// be notified if the FrameTree went from non-loading to loading state.
// However, when it is called, the FrameTree should be in a loading state.
void DidStartLoading(bool to_different_document, bool was_previously_loading);
void DidStartLoading(bool should_show_loading_ui,
bool was_previously_loading);

// A RenderFrameHost in this node stopped loading.
void DidStopLoading();
Expand Down
3 changes: 2 additions & 1 deletion content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8223,7 +8223,8 @@ void RenderFrameHostImpl::HandleRendererDebugURL(const GURL& url) {
if (!url.SchemeIs(url::kJavaScriptScheme)) {
bool was_loading = frame_tree()->IsLoading();
is_loading_ = true;
frame_tree_node()->DidStartLoading(true, was_loading);
frame_tree_node()->DidStartLoading(true /* should_show_loading_ui */,
was_loading);
}

GetAssociatedLocalFrame()->HandleRendererDebugURL(url);
Expand Down
Loading

0 comments on commit 9aabf5f

Please sign in to comment.