Commit dd2af710 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot

Revert "Populate the distilled content slightly earlier"

This reverts commit 1dc90fac.

Reason for revert: suspect for causing flakes in
DomDistillerViewerSourceBrowserTest.PrefPersist

BUG=1004663

Original change's description:
> Populate the distilled content slightly earlier
> 
> Before this CL, the distilled content is injected on DidFinishLoad(),
> or when the distillation finishes if it's later than DidFinishLoad().
> When the DidFinishLoad() event propagates to TalkBack, the distilled
> content is usually not there yet, so TalkBack would announce the
> content of the place-holding page.
> 
> This CL populates the distilled content in DocumentLoadedInFrame()
> instead of DidFinishLoad(). DocumentLoadedInFrame() is late enough to
> execute JavaScript, and is early enough so that it's more likely that
> the title and content can be picked up by TalkBack instead of the
> placeholder.
> 
> If distillation is finished by DocumentLoadedInFrame(), onload() event
> would also be delayed, so that the accessibility focus is more likely
> to be on the web content. Otherwise, the focus is usually on the close
> button of the CustomTab (CCT), or nowhere.
> 
> Note that this CL doesn't guarantee the distilled title is announced
> by TalkBack. If distillation finishes later than
> DocumentLoadedInFrame(), or if for some reason the accessibility focus
> is on the close button of the CCT, the title would go unannounced like
> before.
> 
> Bug: 811417, 803474
> Change-Id: Iaf2ff988b427ca9af582622597b505e7a8aca8e1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1791048
> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> Reviewed-by: Matthew Jones <mdjones@chromium.org>
> Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#697074}

TBR=dmazzoni@chromium.org,mdjones@chromium.org,wychen@chromium.org

Change-Id: I6af4de1fa2a872402f9abe13321f5d1af8e09d75
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 811417, 803474
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807495Reviewed-by: default avatarMikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697128}
parent 788da280
...@@ -86,10 +86,8 @@ class DistilledPageObserver : public NavigationObserver { ...@@ -86,10 +86,8 @@ class DistilledPageObserver : public NavigationObserver {
void DidFinishLoad(content::RenderFrameHost* render_frame_host, void DidFinishLoad(content::RenderFrameHost* render_frame_host,
const GURL& validated_url) override { const GURL& validated_url) override {
if (!render_frame_host->GetParent() && if (!render_frame_host->GetParent() &&
validated_url.scheme() == kDomDistillerScheme) { validated_url.scheme() == kDomDistillerScheme)
loaded_distiller_page_ = true; loaded_distiller_page_ = true;
MaybeNotifyLoaded();
}
} }
void TitleWasSet(content::NavigationEntry* entry) override { void TitleWasSet(content::NavigationEntry* entry) override {
...@@ -97,19 +95,14 @@ class DistilledPageObserver : public NavigationObserver { ...@@ -97,19 +95,14 @@ class DistilledPageObserver : public NavigationObserver {
// and once when the distillation has finished. Watch for the second time // and once when the distillation has finished. Watch for the second time
// as a signal that the JavaScript that sets the content has run. // as a signal that the JavaScript that sets the content has run.
title_set_count_++; title_set_count_++;
MaybeNotifyLoaded(); if (title_set_count_ >= 2 && loaded_distiller_page_) {
new_url_loaded_runner_.QuitClosure().Run();
}
} }
private: private:
int title_set_count_; int title_set_count_;
bool loaded_distiller_page_; bool loaded_distiller_page_;
// DidFinishLoad() can come after the two title settings.
void MaybeNotifyLoaded() {
if (title_set_count_ >= 2 && loaded_distiller_page_) {
new_url_loaded_runner_.QuitClosure().Run();
}
}
}; };
class DomDistillerTabUtilsBrowserTest : public InProcessBrowserTest { class DomDistillerTabUtilsBrowserTest : public InProcessBrowserTest {
......
...@@ -65,8 +65,8 @@ class DomDistillerViewerSource::RequestViewerHandle ...@@ -65,8 +65,8 @@ class DomDistillerViewerSource::RequestViewerHandle
content::NavigationHandle* navigation_handle) override; content::NavigationHandle* navigation_handle) override;
void RenderProcessGone(base::TerminationStatus status) override; void RenderProcessGone(base::TerminationStatus status) override;
void WebContentsDestroyed() override; void WebContentsDestroyed() override;
void DocumentLoadedInFrame( void DidFinishLoad(content::RenderFrameHost* render_frame_host,
content::RenderFrameHost* render_frame_host) override; const GURL& validated_url) override;
void OnInterfaceRequestFromFrame( void OnInterfaceRequestFromFrame(
content::RenderFrameHost* render_frame_host, content::RenderFrameHost* render_frame_host,
const std::string& interface_name, const std::string& interface_name,
...@@ -179,24 +179,14 @@ void DomDistillerViewerSource::RequestViewerHandle::Cancel() { ...@@ -179,24 +179,14 @@ void DomDistillerViewerSource::RequestViewerHandle::Cancel() {
base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this); base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
} }
void DomDistillerViewerSource::RequestViewerHandle::DocumentLoadedInFrame( void DomDistillerViewerSource::RequestViewerHandle::DidFinishLoad(
content::RenderFrameHost* render_frame_host) { content::RenderFrameHost* render_frame_host,
// DocumentLoadedInFrame() is late enough to execute JavaScript, and is early const GURL& validated_url) {
// enough so that it's more likely that the title and content can be picked up
// by TalkBack instead of the placeholder. If distillation is finished by
// DocumentLoadedInFrame(), onload() event would also be delayed, so that the
// accessibility focus is more likely to be on the web content. Otherwise, the
// focus is usually on the close button of the CustomTab (CCT), or nowhere. If
// distillation finishes later than DocumentLoadedInFrame(), or if for some
// reason the accessibility focus is on the close button of the CCT, the title
// could go unannounced.
// See http://crbug.com/811417.
if (render_frame_host->GetParent()) { if (render_frame_host->GetParent()) {
return; return;
} }
int64_t start_time_ms = url_utils::GetTimeFromDistillerUrl( int64_t start_time_ms = url_utils::GetTimeFromDistillerUrl(validated_url);
render_frame_host->GetLastCommittedURL());
if (start_time_ms > 0) { if (start_time_ms > 0) {
base::TimeTicks start_time = base::TimeTicks start_time =
base::TimeDelta::FromMilliseconds(start_time_ms) + base::TimeTicks(); base::TimeDelta::FromMilliseconds(start_time_ms) + base::TimeTicks();
......
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