Skip to content

Commit

Permalink
Integrate text fragments with Document lifecycle
Browse files Browse the repository at this point in the history 
This CL is a refactoring to improve how text fragments are integrated
with the document lifecycle. There are several high-level changes here:

- Annotations (the thing that performs text search and applies the
  highlights) no longer allow attaching at arbitrary points. Instead,
  clients can set an annotation as "needing attachment" and schedule a
  BeginFrame. In the next BeginFrame, all annotations needing attachment
  perform their search after layout is updated.
- Text fragments have a special case for when a page has never been
  visible; the text search is deferred until the page becomes visible.
  The above change required moving this behavior into
  AnnotationAgentContainerImpl.
- TextFragmentAnchor is no longer responsible for controlling when
  annotation attachment occurs. Instead, it's invoked right after
  annotations are attached and checks the status of its annotations to
  update its internal state and perform scroll into view and related
  actions.
- Simplify TextFragmentAnchor's state machine by removing the
  ScriptableActions state. Instead, the script-running actions are
  scheduled for an animation frame and then move the anchor into the
  kDone state.

See also the related followup which this CL enables:
https://crrev.com/c/4586061

Change-Id: I1f6fd7c28b86274beae2cdc934b529c9f7bfb472
Bug: 1327734,1303887,963045
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4577583
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1158968}
  • Loading branch information
@bokand
bokand authored and Chromium LUCI CQ committed Jun 16, 2023
1 parent 2e908b8 commit a2cf450
Show file tree
Hide file tree
Showing 24 changed files with 455 additions and 413 deletions.
156 changes: 82 additions & 74 deletions third_party/blink/renderer/core/annotation/annotation_agent_container_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#include "third_party/blink/renderer/core/fragment_directive/text_fragment_handler.h"
#include "third_party/blink/renderer/core/fragment_directive/text_fragment_selector.h"
#include "third_party/blink/renderer/core/frame/local_frame.h"
#include "third_party/blink/renderer/core/page/chrome_client.h"
#include "third_party/blink/renderer/core/page/page.h"

namespace blink {

Expand All @@ -39,13 +41,13 @@ const char AnnotationAgentContainerImpl::kSupplementName[] =
"AnnotationAgentContainerImpl";

// static
AnnotationAgentContainerImpl* AnnotationAgentContainerImpl::From(
AnnotationAgentContainerImpl* AnnotationAgentContainerImpl::CreateIfNeeded(
Document& document) {
if (!document.IsActive())
if (!document.IsActive()) {
return nullptr;
}

AnnotationAgentContainerImpl* container =
Supplement<Document>::From<AnnotationAgentContainerImpl>(document);
AnnotationAgentContainerImpl* container = FromIfExists(document);
if (!container) {
container =
MakeGarbageCollected<AnnotationAgentContainerImpl>(document, PassKey());
Expand All @@ -55,6 +57,12 @@ AnnotationAgentContainerImpl* AnnotationAgentContainerImpl::From(
return container;
}

// static
AnnotationAgentContainerImpl* AnnotationAgentContainerImpl::FromIfExists(
Document& document) {
return Supplement<Document>::From<AnnotationAgentContainerImpl>(document);
}

// static
void AnnotationAgentContainerImpl::BindReceiver(
LocalFrame* frame,
Expand All @@ -63,7 +71,7 @@ void AnnotationAgentContainerImpl::BindReceiver(
DCHECK(frame->GetDocument());
Document& document = *frame->GetDocument();

auto* container = AnnotationAgentContainerImpl::From(document);
auto* container = AnnotationAgentContainerImpl::CreateIfNeeded(document);
if (!container)
return;

Expand All @@ -83,8 +91,8 @@ AnnotationAgentContainerImpl::AnnotationAgentContainerImpl(Document& document,

void AnnotationAgentContainerImpl::Bind(
mojo::PendingReceiver<mojom::blink::AnnotationAgentContainer> receiver) {
receivers_.Add(std::move(receiver), GetSupplementable()->GetTaskRunner(
TaskType::kInternalDefault));
receivers_.Add(std::move(receiver),
GetDocument().GetTaskRunner(TaskType::kInternalDefault));
}

void AnnotationAgentContainerImpl::Trace(Visitor* visitor) const {
Expand All @@ -94,23 +102,28 @@ void AnnotationAgentContainerImpl::Trace(Visitor* visitor) const {
Supplement<Document>::Trace(visitor);
}

void AnnotationAgentContainerImpl::FinishedParsing() {
TRACE_EVENT("blink", "AnnotationAgentContainerImpl::FinishedParsing",
void AnnotationAgentContainerImpl::PerformInitialAttachments() {
TRACE_EVENT("blink",
"AnnotationAgentContainerImpl::PerformInitialAttachments",
"num_agents", agents_.size());
CHECK(IsLifecycleCleanForAttachment());

if (GetFrame().GetPage()->IsPageVisible()) {
page_has_been_visible_ = true;
}

for (auto& agent : agents_) {
// TODO(crbug.com/1379741): Don't try attaching shared highlights like
// this. Their lifetime is currently owned by TextFragmentAnchor which is
// driven by the document lifecycle. Attach() itself may perform lifecycle
// updates and has safeguards to prevent reentrancy so it's important to
// not call Attach outside of that process. See also: comment in
// Document::ApplyScrollRestorationLogic. Eventually we'd like to move the
// lifecycle management of shared highlight annotations out of
// TextFragmentAnchor. When that happens we can remove this exception.
if (agent->GetType() == mojom::blink::AnnotationType::kSharedHighlight)
continue;

if (!agent->DidTryAttach())
agent->Attach();
if (agent->NeedsAttachment()) {
// SharedHighlights must wait until the page has been made visible at
// least once before searching. See:
// https://wicg.github.io/scroll-to-text-fragment/#search-timing:~:text=If%20a%20UA,in%20background%20documents.
if (agent->GetType() == mojom::blink::AnnotationType::kSharedHighlight &&
!page_has_been_visible_) {
continue;
}

agent->Attach(PassKey());
}
}
}

Expand All @@ -119,27 +132,20 @@ AnnotationAgentImpl* AnnotationAgentContainerImpl::CreateUnboundAgent(
AnnotationSelector& selector) {
auto* agent_impl = MakeGarbageCollected<AnnotationAgentImpl>(
*this, type, selector, PassKey());
agents_.insert(agent_impl);

// TODO(bokan): This is a stepping stone in refactoring the
// TextFragmentHandler. When we replace it with a browser-side manager it may
// make for a better API to have components register a handler for an
// annotation type with AnnotationAgentContainer.
// https://crbug.com/1303887.
if (type == mojom::blink::AnnotationType::kSharedHighlight) {
TextFragmentHandler::DidCreateTextFragment(*agent_impl,
*GetSupplementable());
}
agents_.push_back(agent_impl);

// Attachment will happen as part of the document lifecycle in a new frame.
ScheduleBeginMainFrame();

return agent_impl;
}

void AnnotationAgentContainerImpl::RemoveAgent(AnnotationAgentImpl& agent,
AnnotationAgentImpl::PassKey) {
DCHECK(!agent.IsAttached());
auto itr = agents_.find(&agent);
DCHECK_NE(itr, agents_.end());
agents_.erase(itr);
wtf_size_t index = agents_.Find(&agent);
DCHECK_NE(index, kNotFound);
agents_.EraseAt(index);
}

HeapHashSet<Member<AnnotationAgentImpl>>
Expand Down Expand Up @@ -175,26 +181,15 @@ void AnnotationAgentContainerImpl::CreateAgent(
return;
}

auto* agent_impl = MakeGarbageCollected<AnnotationAgentImpl>(
*this, type, *selector, PassKey());
agents_.insert(agent_impl);
auto* agent_impl = CreateUnboundAgent(type, *selector);
agent_impl->Bind(std::move(host_remote), std::move(agent_receiver));

Document& document = *GetSupplementable();

// We may have received this message before the document finishes parsing.
// Postpone attachment for now; these agents will try attaching when the
// document finishes parsing.
if (document.HasFinishedParsing()) {
agent_impl->Attach();
} else {
TRACE_EVENT_INSTANT("blink", "Waiting on parse to attach");
}
}

void AnnotationAgentContainerImpl::CreateAgentFromSelection(
mojom::blink::AnnotationType type,
CreateAgentFromSelectionCallback callback) {
TRACE_EVENT("blink", "AnnotationAgentContainerImpl::CreateAgentFromSelection",
"type", ToString(type));
DCHECK(annotation_agent_generator_);
annotation_agent_generator_->GetForCurrentSelection(
type,
Expand All @@ -211,24 +206,22 @@ void AnnotationAgentContainerImpl::DidFinishSelectorGeneration(
const String& selected_text,
const TextFragmentSelector& selector,
shared_highlighting::LinkGenerationError error) {
TRACE_EVENT("blink",
"AnnotationAgentContainerImpl::DidFinishSelectorGeneration",
"type", ToString(type));

if (error != shared_highlighting::LinkGenerationError::kNone) {
std::move(callback).Run(/*SelectorCreationResult=*/nullptr, error,
ready_status);
return;
}

// TODO(bokan): Should we clear the frame selection?
{
// If the document were detached selector generation will return above with
// an error.
Document* document = GetSupplementable();
DCHECK(document);
// If the document was detached then selector generation must have returned
// an error.
CHECK(GetSupplementable());

LocalFrame* frame = document->GetFrame();
DCHECK(frame);

frame->Selection().Clear();
}
// TODO(bokan): Why doesn't this clear selection?
GetFrame().Selection().Clear();

mojo::PendingRemote<mojom::blink::AnnotationAgentHost> pending_host_remote;
mojo::PendingReceiver<mojom::blink::AnnotationAgent> pending_agent_receiver;
Expand Down Expand Up @@ -261,8 +254,6 @@ void AnnotationAgentContainerImpl::DidFinishSelectorGeneration(
CreateUnboundAgent(type, *annotation_selector);
agent_impl->Bind(std::move(pending_host_remote),
std::move(pending_agent_receiver));

agent_impl->Attach();
}

void AnnotationAgentContainerImpl::OpenedContextMenuOverSelection() {
Expand All @@ -273,29 +264,46 @@ void AnnotationAgentContainerImpl::OpenedContextMenuOverSelection() {
annotation_agent_generator_->PreemptivelyGenerateForCurrentSelection();
}

bool AnnotationAgentContainerImpl::ShouldPreemptivelyGenerate() {
Document* document = GetSupplementable();
DCHECK(document);

LocalFrame* frame = document->GetFrame();
DCHECK(frame);
bool AnnotationAgentContainerImpl::IsLifecycleCleanForAttachment() const {
return GetDocument().HasFinishedParsing() &&
!GetDocument().NeedsLayoutTreeUpdate() &&
!GetFrame().View()->NeedsLayout();
}

if (!shared_highlighting::ShouldOfferLinkToText(
GURL(frame->GetDocument()->Url()))) {
bool AnnotationAgentContainerImpl::ShouldPreemptivelyGenerate() {
if (!shared_highlighting::ShouldOfferLinkToText(GURL(GetDocument().Url()))) {
return false;
}

if (frame->Selection().SelectedText().empty())
if (GetFrame().Selection().SelectedText().empty()) {
return false;
}

if (frame->IsOutermostMainFrame())
if (GetFrame().IsOutermostMainFrame()) {
return true;
}

// Only generate for iframe urls if they are supported
return base::FeatureList::IsEnabled(
shared_highlighting::kSharedHighlightingAmp) &&
shared_highlighting::SupportsLinkGenerationInIframe(
GURL(frame->GetDocument()->Url()));
GURL(GetDocument().Url()));
}

void AnnotationAgentContainerImpl::ScheduleBeginMainFrame() {
GetFrame().GetPage()->GetChromeClient().ScheduleAnimation(GetFrame().View());
}

Document& AnnotationAgentContainerImpl::GetDocument() const {
Document* document = GetSupplementable();
CHECK(document);
return *document;
}

LocalFrame& AnnotationAgentContainerImpl::GetFrame() const {
LocalFrame* frame = GetDocument().GetFrame();
CHECK(frame);
return *frame;
}

} // namespace blink
25 changes: 21 additions & 4 deletions third_party/blink/renderer/core/annotation/annotation_agent_container_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "third_party/blink/renderer/core/dom/document.h"
#include "third_party/blink/renderer/core/fragment_directive/text_fragment_selector_generator.h"
#include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_set.h"
#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"
#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
#include "third_party/blink/renderer/platform/heap/member.h"
#include "third_party/blink/renderer/platform/mojo/heap_mojo_receiver_set.h"
Expand Down Expand Up @@ -46,7 +47,11 @@ class CORE_EXPORT AnnotationAgentContainerImpl final
// Static getter for the container for the given document. Will instantiate a
// container if the document doesn't yet have one. This can return nullptr if
// requested from an inactive or detached document.
static AnnotationAgentContainerImpl* From(Document&);
static AnnotationAgentContainerImpl* CreateIfNeeded(Document&);

// Same as above but won't create an instance if one isn't already present on
// the Document.
static AnnotationAgentContainerImpl* FromIfExists(Document&);

static void BindReceiver(
LocalFrame* frame,
Expand All @@ -68,8 +73,9 @@ class CORE_EXPORT AnnotationAgentContainerImpl final

void Trace(Visitor* visitor) const override;

// Notifies the container when parsing in its document has finished.
void FinishedParsing();
// Calls Attach() on any agent that needs an attachment. Must be called in a
// clean lifecycle state.
void PerformInitialAttachments();

// Removes the given agent from this container. It is an error to try and
// remove an agent from a container that doesn't hold it. Once removed, the
Expand Down Expand Up @@ -98,6 +104,10 @@ class CORE_EXPORT AnnotationAgentContainerImpl final

void OpenedContextMenuOverSelection();

// Returns true if the document is in a clean state to run annotation
// attachment. i.e. Parsing has finished and layout and style are clean.
bool IsLifecycleCleanForAttachment() const;

private:
friend AnnotationAgentContainerImplTest;

Expand All @@ -111,13 +121,20 @@ class CORE_EXPORT AnnotationAgentContainerImpl final
const TextFragmentSelector& selector,
shared_highlighting::LinkGenerationError error);

void ScheduleBeginMainFrame();

Document& GetDocument() const;
LocalFrame& GetFrame() const;

Member<AnnotationAgentGenerator> annotation_agent_generator_;

HeapMojoReceiverSet<mojom::blink::AnnotationAgentContainer,
AnnotationAgentContainerImpl>
receivers_;

HeapHashSet<Member<AnnotationAgentImpl>> agents_;
HeapVector<Member<AnnotationAgentImpl>> agents_;

bool page_has_been_visible_ = false;
};

} // namespace blink
Expand Down

0 comments on commit a2cf450

Please sign in to comment.