Commit 233cf5cc authored by Chris Harrelson's avatar Chris Harrelson Committed by Commit Bot

Don't invoke text fragment anchors until the document has parsed or loaded.

Text fragment anchors are very expensive to compute; otherwise
page load performance may suffer significantly. Before this CL,
text fragments could be invoked an arbitrary number of times up to
the time when the document was loaded - whenever a lifecycle-based
or forced update happened. This slowed down page load, as well as
all layout-inducing DOM APIs.

A previous CL (*) regressed performance on top of this problem, because
it invoked the text fragment regardless of whether layout was dirty.
Before that CL, the expensive part of text fragment invocation
only happened if layout was dirty. Layout could be dirty many times
before the document is loaded, but many fewer times than the number
of lifecycles or forced layouts.

The main consequence of this CL is that text fragment invocation will
not happen at all until the document is parsed, and then once again
once it is loaded (which includes waiting for certain subresources to
load, in particular images).

It appears that the spec allows (*) user agents to define the
timing of fragment scrolling and highlighting.

The test TextFragmentAnchorTest.TargetStaysInView exercises this
new code code quite well, and still passes with this CL. I couldn't
make it pass until every part of the new CL was in place.

(*) https://chromium-review.googlesource.com/c/chromium/src/+/1981806
(**) https://html.spec.whatwg.org/multipage/browsing-the-web.html#try-to-scroll-to-the-fragment

Bug: 1094452

Change-Id: Iee19989061128ff158b72ed99cabd0dec8f449e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2267137Reviewed-by: default avatarDavid Bokan <bokan@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782814}
parent 92c44c35
...@@ -172,13 +172,34 @@ bool TextFragmentAnchor::Invoke() { ...@@ -172,13 +172,34 @@ bool TextFragmentAnchor::Invoke() {
else else
page_has_been_visible_ = true; page_has_been_visible_ = true;
// We need to keep this TextFragmentAnchor alive if we're proxying an
// element fragment anchor.
if (element_fragment_anchor_) { if (element_fragment_anchor_) {
DCHECK(search_finished_); DCHECK(search_finished_);
// We need to keep this TextFragmentAnchor alive if we're proxying an
// element fragment anchor.
return true; return true;
} }
// Only invoke once, and then a second time once the document is loaded.
// Otherwise page load performance could be significantly
// degraded, since TextFragmentFinder has O(n) performance. The reason
// for invoking twice is to give client-side rendered sites more opportunity
// to add text that can participate in text fragment invocation.
if (!frame_->GetDocument()->IsLoadCompleted()) {
// When parsing is complete the following sequence happens:
// 1. Invoke with beforematch_state_ == kNoMatchFound. This runs a match and
// causes beforematch_state_ to be set to kEventQueued, and queues
// a task to set beforematch_state_ to be set to kFiredEvent.
// 2. (maybe) Invoke with beforematch_state_ == kEventQueued.
// 3. Invoke with beforematch_state_ == kFiredEvent. This runs a match and
// causes text_searched_after_parsing_finished_ to become true.
// 4. Any future calls to Invoke before loading are ignored.
//
// TODO(chrishtr): if layout is not dirtied, we don't need to re-run
// the text finding again and again for each of the above steps.
if (has_performed_first_text_search_ && beforematch_state_ != kEventQueued)
return true;
}
// If we're done searching, return true if this hasn't been dismissed yet so // If we're done searching, return true if this hasn't been dismissed yet so
// that this is kept alive. // that this is kept alive.
if (search_finished_) if (search_finished_)
...@@ -210,6 +231,9 @@ bool TextFragmentAnchor::Invoke() { ...@@ -210,6 +231,9 @@ bool TextFragmentAnchor::Invoke() {
finder.FindMatch(*frame_->GetDocument()); finder.FindMatch(*frame_->GetDocument());
} }
if (beforematch_state_ != kEventQueued)
has_performed_first_text_search_ = true;
// Stop searching for matching text once the load event has fired. This may // Stop searching for matching text once the load event has fired. This may
// cause ScrollToTextFragment to not work on pages which dynamically load // cause ScrollToTextFragment to not work on pages which dynamically load
// content: http://crbug.com/963045 // content: http://crbug.com/963045
......
...@@ -109,6 +109,9 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor, ...@@ -109,6 +109,9 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
// to determine whether the user subsequently scrolls back to the top. // to determine whether the user subsequently scrolls back to the top.
bool did_non_zero_scroll_ = false; bool did_non_zero_scroll_ = false;
// Whether a text fragment finder was run.
bool has_performed_first_text_search_ = false;
enum BeforematchState { enum BeforematchState {
kNoMatchFound, // DidFindMatch has not been called. kNoMatchFound, // DidFindMatch has not been called.
kEventQueued, // Beforematch event has been queued, but not fired yet. kEventQueued, // Beforematch event has been queued, but not fired yet.
......
...@@ -1197,11 +1197,12 @@ TEST_F(TextFragmentAnchorTest, TargetStaysInView) { ...@@ -1197,11 +1197,12 @@ TEST_F(TextFragmentAnchorTest, TargetStaysInView) {
<p id="text">test</p> <p id="text">test</p>
)HTML"); )HTML");
RunAsyncMatchingTasks(); RunAsyncMatchingTasks();
// Render two frames to handle the async step added by the beforematch event.
Compositor().BeginFrame(); Compositor().BeginFrame();
Compositor().BeginFrame(); Compositor().BeginFrame();
EXPECT_FALSE(GetDocument().IsLoadCompleted());
EXPECT_TRUE(GetDocument().HasFinishedParsing());
ScrollOffset first_scroll_offset = LayoutViewport()->GetScrollOffset(); ScrollOffset first_scroll_offset = LayoutViewport()->GetScrollOffset();
ASSERT_NE(ScrollOffset(), first_scroll_offset); ASSERT_NE(ScrollOffset(), first_scroll_offset);
...@@ -1215,8 +1216,13 @@ TEST_F(TextFragmentAnchorTest, TargetStaysInView) { ...@@ -1215,8 +1216,13 @@ TEST_F(TextFragmentAnchorTest, TargetStaysInView) {
<rect fill="green" width="200" height="2000"/> <rect fill="green" width="200" height="2000"/>
</svg> </svg>
)SVG"); )SVG");
RunPendingTasks();
EXPECT_TRUE(GetDocument().IsLoadCompleted());
EXPECT_TRUE(GetDocument().HasFinishedParsing());
RunAsyncMatchingTasks(); RunAsyncMatchingTasks();
Compositor().BeginFrame(); Compositor().BeginFrame();
Compositor().BeginFrame();
// Ensure the target text is still in view and stayed centered // Ensure the target text is still in view and stayed centered
ASSERT_NE(first_scroll_offset, LayoutViewport()->GetScrollOffset()); ASSERT_NE(first_scroll_offset, LayoutViewport()->GetScrollOffset());
...@@ -1850,6 +1856,8 @@ TEST_F(TextFragmentAnchorTest, NonTextDirectives) { ...@@ -1850,6 +1856,8 @@ TEST_F(TextFragmentAnchorTest, NonTextDirectives) {
<p id="first">This is a test page</p> <p id="first">This is a test page</p>
<p id="second">This is some more text</p> <p id="second">This is some more text</p>
)HTML"); )HTML");
RunPendingTasks();
// Render two frames to handle the async step added by the beforematch event. // Render two frames to handle the async step added by the beforematch event.
Compositor().BeginFrame(); Compositor().BeginFrame();
Compositor().BeginFrame(); Compositor().BeginFrame();
......
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