Commit fbbdff52 authored by Liquan(Max) Gu's avatar Liquan(Max) Gu Committed by Commit Bot

[LCP] Text: wrap largest text paint in a class

Currently, we are using flags (is_reporting_lcp_) to stop the
largest-text-paint pipeline in TextPaintTimingDetector. However,
the flag is hard to maintain and is error-prone. To fix this issue,
we introduce a wrapper for the largest-text-paint part of code.
Once we stop largest-text-paint, we set the wrapper instance to null.
This way, any data structure and functions of this pipeline would no
longer be accessible.

In this patch, we also remove the timer. The removal is included in
this patch because it's part of the largest-text-paint pipeline.
Although the removal is a relatively separate part, we suggest
including it here because making it two steps would complicate the
middle state.

The removal of timer has several benefits:
1) can avoid the complicated control logic of timer.
2) can make the metric result more precise, because we move the result
update from the 1s-timer callback to the swap-time assignment callback.

Bug:977181,977926,977952

Change-Id: I01821517ee9a01c70ad65dd901b0f3d687bbb1de
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1672113
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: default avatarLiquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: default avatarNicolás Peña Moreno <npm@chromium.org>
Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671941}
parent 0aa7df50
......@@ -76,9 +76,6 @@ class LargestContentfulPaintCalculatorTest : public RenderingTest {
.GetTextPaintTimingDetector();
text_detector->ReportSwapTime(WebWidgetClient::SwapResult::kDidSwap,
simulated_clock_.NowTicks());
// TexPaintTimingDetector does not update the candidate on the swap
// callback, so manually call it.
text_detector->UpdateCandidate();
}
private:
......
......@@ -54,7 +54,7 @@ bool IsBackgroundImageContentful(const LayoutObject& object,
PaintTimingDetector::PaintTimingDetector(LocalFrameView* frame_view)
: frame_view_(frame_view),
text_paint_timing_detector_(
MakeGarbageCollected<TextPaintTimingDetector>(frame_view)),
MakeGarbageCollected<TextPaintTimingDetector>(frame_view, this)),
image_paint_timing_detector_(
MakeGarbageCollected<ImagePaintTimingDetector>(frame_view)) {}
......
......@@ -42,7 +42,7 @@ FloatRect TextElementTiming::ComputeIntersectionRect(
Node* node,
const IntRect& aggregated_visual_rect,
const PropertyTreeState& property_tree_state,
LocalFrameView* frame_view) {
const LocalFrameView* frame_view) {
if (!NeededForElementTiming(node))
return FloatRect();
......
......@@ -44,7 +44,7 @@ class CORE_EXPORT TextElementTiming final
Node*,
const IntRect& aggregated_visual_rect,
const PropertyTreeState&,
LocalFrameView*);
const LocalFrameView*);
// Called when the swap promise queued by TextPaintTimingDetector has been
// resolved. Dispatches PerformanceElementTiming entries to WindowPerformance.
......
......@@ -27,8 +27,6 @@ namespace blink {
namespace {
// Calculate metrics candidate every 1 second after the first text pre-paint.
constexpr base::TimeDelta kTimerDelay = base::TimeDelta::FromSeconds(1);
constexpr size_t kTextNodeNumberLimit = 5000;
bool LargeTextFirst(const base::WeakPtr<TextRecord>& a,
......@@ -44,14 +42,13 @@ bool LargeTextFirst(const base::WeakPtr<TextRecord>& a,
} // namespace
TextPaintTimingDetector::TextPaintTimingDetector(LocalFrameView* frame_view)
: records_manager_(),
timer_(frame_view->GetFrame().GetTaskRunner(TaskType::kInternalDefault),
this,
&TextPaintTimingDetector::TimerFired),
TextPaintTimingDetector::TextPaintTimingDetector(
LocalFrameView* frame_view,
PaintTimingDetector* paint_timing_detector)
: records_manager_(frame_view, paint_timing_detector),
frame_view_(frame_view) {}
void TextPaintTimingDetector::PopulateTraceValue(
void LargestTextPaintManager::PopulateTraceValue(
TracedValue& value,
const TextRecord& first_text_paint) {
// TODO(crbug.com/976893): Remove DOMNodeId.
......@@ -63,7 +60,7 @@ void TextPaintTimingDetector::PopulateTraceValue(
!frame_view_->GetFrame().LocalFrameRoot().IsMainFrame());
}
void TextPaintTimingDetector::ReportCandidateToTrace(
void LargestTextPaintManager::ReportCandidateToTrace(
const TextRecord& largest_text_record) {
auto value = std::make_unique<TracedValue>();
PopulateTraceValue(*value, largest_text_record);
......@@ -75,7 +72,7 @@ void TextPaintTimingDetector::ReportCandidateToTrace(
ToTraceValue(&frame_view_->GetFrame()));
}
void TextPaintTimingDetector::ReportNoCandidateToTrace() {
void LargestTextPaintManager::ReportNoCandidateToTrace() {
auto value = std::make_unique<TracedValue>();
value->SetInteger("candidateIndex", ++count_candidates_);
value->SetBoolean("isMainFrame", frame_view_->GetFrame().IsMainFrame());
......@@ -86,37 +83,26 @@ void TextPaintTimingDetector::ReportNoCandidateToTrace() {
ToTraceValue(&frame_view_->GetFrame()));
}
// The timer has guaranteed that |this| exists when this function is invoked.
void TextPaintTimingDetector::TimerFired(TimerBase* time) {
// Wrap |UpdateCandidate| method in TimerFired so that we can drop |time| for
// |UpdateCandidate| in testing.
UpdateCandidate();
}
void TextPaintTimingDetector::UpdateCandidate() {
if (!is_recording_)
return;
if (!is_recording_ltp_)
return;
DCHECK(RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled());
base::WeakPtr<TextRecord> largest_text_record =
records_manager_.FindLargestPaintCandidate();
void LargestTextPaintManager::UpdateCandidate() {
base::WeakPtr<TextRecord> largest_text_record = FindLargestPaintCandidate();
const base::TimeTicks time =
largest_text_record ? largest_text_record->paint_time : base::TimeTicks();
const uint64_t size =
largest_text_record ? largest_text_record->first_size : 0;
PaintTimingDetector& detector = frame_view_->GetPaintTimingDetector();
bool changed = detector.NotifyIfChangedLargestTextPaint(time, size);
DCHECK(paint_timing_detector_);
bool changed =
paint_timing_detector_->NotifyIfChangedLargestTextPaint(time, size);
if (!changed)
return;
if (largest_text_record && !largest_text_record->paint_time.is_null()) {
if (auto* lcp_calculator = detector.GetLargestContentfulPaintCalculator())
if (auto* lcp_calculator =
paint_timing_detector_->GetLargestContentfulPaintCalculator())
lcp_calculator->OnLargestTextUpdated(largest_text_record);
ReportCandidateToTrace(*largest_text_record);
} else {
if (auto* lcp_calculator = detector.GetLargestContentfulPaintCalculator())
if (auto* lcp_calculator =
paint_timing_detector_->GetLargestContentfulPaintCalculator())
lcp_calculator->OnLargestTextUpdated(nullptr);
ReportNoCandidateToTrace();
}
......@@ -125,12 +111,10 @@ void TextPaintTimingDetector::UpdateCandidate() {
void TextPaintTimingDetector::OnPaintFinished() {
if (need_update_timing_at_frame_end_) {
need_update_timing_at_frame_end_ = false;
UpdateCandidate();
if (records_manager_.GetLargestTextPaintManager())
records_manager_.GetLargestTextPaintManager()->UpdateCandidate();
}
if (records_manager_.NeedMeausuringPaintTime()) {
// Start repeating timer only once at the first text paint.
if (!timer_.IsActive() && is_recording_ltp_)
timer_.StartRepeating(kTimerDelay, FROM_HERE);
if (!awaiting_swap_promise_) {
// |WrapCrossThreadWeakPersistent| guarantees that when |this| is killed,
// the callback function will not be invoked.
......@@ -180,6 +164,8 @@ void TextPaintTimingDetector::ReportSwapTime(WebWidgetClient::SwapResult result,
}
}
records_manager_.AssignPaintTimeToQueuedNodes(timestamp);
if (records_manager_.GetLargestTextPaintManager())
records_manager_.GetLargestTextPaintManager()->UpdateCandidate();
awaiting_swap_promise_ = false;
}
......@@ -195,7 +181,8 @@ bool TextPaintTimingDetector::ShouldWalkObject(
// If we have finished recording Largest Text Paint and the element is a
// shadow element or has no elementtiming attribute, then we should not record
// its text.
if (!is_recording_ltp_ && !TextElementTiming::NeededForElementTiming(node))
if (!records_manager_.IsRecordingLargestTextPaint() &&
!TextElementTiming::NeededForElementTiming(node))
return false;
DOMNodeId node_id = DOMNodeIds::ExistingIdForNode(node);
......@@ -246,20 +233,13 @@ void TextPaintTimingDetector::RecordAggregatedText(
}
}
base::WeakPtr<TextRecord> TextPaintTimingDetector::FindLargestPaintCandidate() {
return records_manager_.FindLargestPaintCandidate();
}
void TextPaintTimingDetector::StopRecordEntries() {
timer_.Stop();
is_recording_ = false;
records_manager_.CleanUp();
}
void TextPaintTimingDetector::StopRecordingLargestTextPaint() {
timer_.Stop();
is_recording_ltp_ = false;
records_manager_.CleanUpLargestContentfulPaint();
records_manager_.CleanUpLargestTextPaint();
}
void TextPaintTimingDetector::Trace(blink::Visitor* visitor) {
......@@ -267,26 +247,36 @@ void TextPaintTimingDetector::Trace(blink::Visitor* visitor) {
visitor->Trace(frame_view_);
}
TextRecordsManager::TextRecordsManager() : size_ordered_set_(&LargeTextFirst) {}
LargestTextPaintManager::LargestTextPaintManager(
LocalFrameView* frame_view,
PaintTimingDetector* paint_timing_detector)
: size_ordered_set_(&LargeTextFirst),
frame_view_(frame_view),
paint_timing_detector_(paint_timing_detector) {}
void LargestTextPaintManager::Trace(blink::Visitor* visitor) {
visitor->Trace(frame_view_);
visitor->Trace(paint_timing_detector_);
}
void TextRecordsManager::RemoveVisibleRecord(const LayoutObject& object) {
DCHECK(visible_node_map_.Contains(&object));
size_ordered_set_.erase(visible_node_map_.at(&object)->AsWeakPtr());
if (ltp_manager_) {
ltp_manager_->RemoveVisibleRecord(
visible_node_map_.at(&object)->AsWeakPtr());
}
visible_node_map_.erase(&object);
// We don't need to remove elements in |texts_queued_for_paint_time_| and
// |cached_largest_paint_candidate_| as they are weak ptr.
is_result_invalidated_ = true;
}
void TextRecordsManager::CleanUpLargestContentfulPaint() {
size_ordered_set_.clear();
is_result_invalidated_ = true;
void TextRecordsManager::CleanUpLargestTextPaint() {
ltp_manager_.reset();
}
void TextRecordsManager::RemoveInvisibleRecord(const LayoutObject& object) {
DCHECK(invisible_node_ids_.Contains(&object));
invisible_node_ids_.erase(&object);
is_result_invalidated_ = true;
}
void TextRecordsManager::AssignPaintTimeToQueuedNodes(
......@@ -310,7 +300,8 @@ void TextRecordsManager::AssignPaintTimeToQueuedNodes(
if (text_element_timing_)
text_element_timing_->OnTextNodesPainted(texts_queued_for_paint_time_);
texts_queued_for_paint_time_.clear();
is_result_invalidated_ = true;
if (ltp_manager_)
ltp_manager_->SetCachedResultInvalidated(true);
}
void TextRecordsManager::RecordVisibleObject(
......@@ -323,11 +314,12 @@ void TextRecordsManager::RecordVisibleObject(
std::unique_ptr<TextRecord> record =
std::make_unique<TextRecord>(node_id, visual_size, element_timing_rect);
if (is_recording_ltp_)
size_ordered_set_.emplace(record->AsWeakPtr());
QueueToMeasurePaintTime(record->AsWeakPtr());
base::WeakPtr<TextRecord> record_weak_ptr = record->AsWeakPtr();
if (ltp_manager_)
ltp_manager_->InsertRecord(record_weak_ptr);
QueueToMeasurePaintTime(record_weak_ptr);
visible_node_map_.insert(&object, std::move(record));
is_result_invalidated_ = true;
}
bool TextRecordsManager::HasTooManyNodes() const {
......@@ -335,8 +327,7 @@ bool TextRecordsManager::HasTooManyNodes() const {
kTextNodeNumberLimit;
}
base::WeakPtr<TextRecord> TextRecordsManager::FindLargestPaintCandidate() {
DCHECK_EQ(visible_node_map_.size(), size_ordered_set_.size());
base::WeakPtr<TextRecord> LargestTextPaintManager::FindLargestPaintCandidate() {
if (!is_result_invalidated_ && cached_largest_paint_candidate_)
return cached_largest_paint_candidate_;
base::WeakPtr<TextRecord> new_largest_paint_candidate = nullptr;
......@@ -352,15 +343,23 @@ base::WeakPtr<TextRecord> TextRecordsManager::FindLargestPaintCandidate() {
return new_largest_paint_candidate;
}
TextRecordsManager::TextRecordsManager(
LocalFrameView* frame_view,
PaintTimingDetector* paint_timing_detector) {
if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled())
ltp_manager_.emplace(frame_view, paint_timing_detector);
}
void TextRecordsManager::CleanUp() {
visible_node_map_.clear();
invisible_node_ids_.clear();
texts_queued_for_paint_time_.clear();
CleanUpLargestContentfulPaint();
CleanUpLargestTextPaint();
}
void TextRecordsManager::Trace(blink::Visitor* visitor) {
visitor->Trace(text_element_timing_);
visitor->Trace(ltp_manager_);
}
} // namespace blink
......@@ -13,7 +13,6 @@
#include "third_party/blink/renderer/core/dom/dom_node_ids.h"
#include "third_party/blink/renderer/core/paint/text_element_timing.h"
#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
#include "third_party/blink/renderer/platform/timer.h"
#include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
#include "third_party/blink/renderer/platform/wtf/hash_set.h"
#include "third_party/blink/renderer/platform/wtf/time.h"
......@@ -43,18 +42,65 @@ class TextRecord : public base::SupportsWeakPtr<TextRecord> {
DISALLOW_COPY_AND_ASSIGN(TextRecord);
};
class TextRecordsManager {
class CORE_EXPORT LargestTextPaintManager {
DISALLOW_NEW();
using TextRecordSetComparator = bool (*)(const base::WeakPtr<TextRecord>&,
const base::WeakPtr<TextRecord>&);
using TextRecordSet =
std::set<base::WeakPtr<TextRecord>, TextRecordSetComparator>;
friend class TextPaintTimingDetectorTest;
public:
TextRecordsManager();
LargestTextPaintManager(LocalFrameView*, PaintTimingDetector*);
inline void RemoveVisibleRecord(base::WeakPtr<TextRecord> record) {
size_ordered_set_.erase(record);
if (cached_largest_paint_candidate_.get() == record.get())
cached_largest_paint_candidate_ = nullptr;
is_result_invalidated_ = true;
}
base::WeakPtr<TextRecord> FindLargestPaintCandidate();
void ReportCandidateToTrace(const TextRecord&);
void ReportNoCandidateToTrace();
void UpdateCandidate();
void PopulateTraceValue(TracedValue&, const TextRecord& first_text_paint);
inline void SetCachedResultInvalidated(bool value) {
is_result_invalidated_ = value;
}
inline void InsertRecord(base::WeakPtr<TextRecord> record) {
size_ordered_set_.emplace(record);
SetCachedResultInvalidated(true);
}
void Trace(blink::Visitor*);
private:
friend class LargestContentfulPaintCalculatorTest;
friend class TextPaintTimingDetectorTest;
TextRecordSet size_ordered_set_;
base::WeakPtr<TextRecord> cached_largest_paint_candidate_;
// This is used to cache the largest text paint result for better
// efficiency.
// The result will be invalidated whenever any change is done to the
// variables used in |FindLargestPaintCandidate|.
bool is_result_invalidated_ = false;
unsigned count_candidates_ = 0;
Member<const LocalFrameView> frame_view_;
Member<PaintTimingDetector> paint_timing_detector_;
DISALLOW_COPY_AND_ASSIGN(LargestTextPaintManager);
};
class CORE_EXPORT TextRecordsManager {
DISALLOW_NEW();
friend class TextPaintTimingDetectorTest;
public:
TextRecordsManager(LocalFrameView*, PaintTimingDetector*);
void RemoveVisibleRecord(const LayoutObject&);
void RemoveInvisibleRecord(const LayoutObject&);
inline void RecordInvisibleObject(const LayoutObject& object) {
......@@ -88,40 +134,39 @@ class TextRecordsManager {
}
void CleanUp();
void CleanUpLargestContentfulPaint();
void CleanUpLargestTextPaint();
void StopRecordingLargestTextPaint();
bool IsRecordingLargestTextPaint() const { return is_recording_ltp_; }
bool HasTextElementTiming() const { return !!text_element_timing_; }
bool HasTextElementTiming() const { return text_element_timing_; }
void SetTextElementTiming(TextElementTiming* text_element_timing) {
text_element_timing_ = text_element_timing;
}
inline base::Optional<LargestTextPaintManager>& GetLargestTextPaintManager() {
return ltp_manager_;
}
inline bool IsRecordingLargestTextPaint() const {
return ltp_manager_.has_value();
}
void Trace(blink::Visitor*);
private:
inline void QueueToMeasurePaintTime(base::WeakPtr<TextRecord> record) {
friend class LargestContentfulPaintCalculatorTest;
friend class TextPaintTimingDetectorTest;
inline void QueueToMeasurePaintTime(base::WeakPtr<TextRecord>& record) {
texts_queued_for_paint_time_.push_back(std::move(record));
}
// This is used to cache the largest text paint result for better efficiency.
// The result will be invalidated whenever any change is done to the variables
// used in |FindLargestPaintCandidate|.
bool is_result_invalidated_ = false;
// This is used to know whether |size_ordered_set_| should be populated or
// not, since this is used by Largest Text Paint but not by Text Element
// Timing.
bool is_recording_ltp_ =
RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled();
// Once LayoutObject* is destroyed, |visible_node_map_| and
// |invisible_node_ids_| must immediately clear the corresponding record from
// themselves.
HashMap<const LayoutObject*, std::unique_ptr<TextRecord>> visible_node_map_;
HashSet<const LayoutObject*> invisible_node_ids_;
TextRecordSet size_ordered_set_;
Deque<base::WeakPtr<TextRecord>> texts_queued_for_paint_time_;
base::WeakPtr<TextRecord> cached_largest_paint_candidate_;
base::Optional<LargestTextPaintManager> ltp_manager_;
Member<TextElementTiming> text_element_timing_;
DISALLOW_COPY_AND_ASSIGN(TextRecordsManager);
......@@ -151,49 +196,35 @@ class CORE_EXPORT TextPaintTimingDetector final
friend class TextPaintTimingDetectorTest;
public:
TextPaintTimingDetector(LocalFrameView* frame_view);
explicit TextPaintTimingDetector(LocalFrameView*, PaintTimingDetector*);
bool ShouldWalkObject(const LayoutBoxModelObject&) const;
void RecordAggregatedText(const LayoutBoxModelObject& aggregator,
const IntRect& aggregated_visual_rect,
const PropertyTreeState&);
void OnPaintFinished();
void LayoutObjectWillBeDestroyed(const LayoutObject&);
base::WeakPtr<TextRecord> FindLargestPaintCandidate();
void StopRecordEntries();
void StopRecordingLargestTextPaint();
bool IsRecording() const { return is_recording_; }
bool FinishedReportingText() const {
return !is_recording_ && !need_update_timing_at_frame_end_;
}
inline bool FinishedReportingText() const { return !is_recording_; }
void Trace(blink::Visitor*);
private:
friend class LargestContentfulPaintCalculatorTest;
void PopulateTraceValue(TracedValue&, const TextRecord& first_text_paint);
void TimerFired(TimerBase*);
void UpdateCandidate();
void ReportSwapTime(WebWidgetClient::SwapResult result,
base::TimeTicks timestamp);
void RegisterNotifySwapTime(ReportTimeCallback callback);
void ReportCandidateToTrace(const TextRecord&);
void ReportNoCandidateToTrace();
TextRecordsManager records_manager_;
// Make sure that at most one swap promise is ongoing.
bool awaiting_swap_promise_ = false;
unsigned count_candidates_ = 0;
bool is_recording_ = true;
bool is_recording_ltp_ =
RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled();
bool has_records_changed_ = true;
bool need_update_timing_at_frame_end_ = false;
TaskRunnerTimer<TextPaintTimingDetector> timer_;
Member<LocalFrameView> frame_view_;
Member<const LocalFrameView> frame_view_;
DISALLOW_COPY_AND_ASSIGN(TextPaintTimingDetector);
};
......
......@@ -55,7 +55,8 @@ class TextPaintTimingDetectorTest
wtf_size_t CountRankingSetSize() {
return GetPaintTimingDetector()
.GetTextPaintTimingDetector()
->records_manager_.size_ordered_set_.size();
->records_manager_.GetLargestTextPaintManager()
->size_ordered_set_.size();
}
void InvokeCallback() {
......@@ -95,10 +96,6 @@ class TextPaintTimingDetectorTest
test_task_runner_->NowTicks());
}
void UpdateCandidate() {
GetPaintTimingDetector().GetTextPaintTimingDetector()->UpdateCandidate();
}
Element* AppendFontBlockToBody(String content) {
Element* font = GetDocument().CreateRawElement(html_names::kFontTag);
font->setAttribute(html_names::kSizeAttr, AtomicString("5"));
......@@ -123,6 +120,7 @@ class TextPaintTimingDetectorTest
return GetFrameView()
.GetPaintTimingDetector()
.GetTextPaintTimingDetector()
->records_manager_.GetLargestTextPaintManager()
->FindLargestPaintCandidate();
}
......@@ -130,6 +128,7 @@ class TextPaintTimingDetectorTest
return GetChildFrameView()
.GetPaintTimingDetector()
.GetTextPaintTimingDetector()
->records_manager_.GetLargestTextPaintManager()
->FindLargestPaintCandidate();
}
......@@ -236,7 +235,6 @@ TEST_F(TextPaintTimingDetectorTest, UpdateResultWhenCandidateChanged) {
<div>small text</div>
)HTML");
UpdateAllLifecyclePhasesAndSimulateSwapTime();
UpdateCandidate();
base::TimeTicks time2 = NowTicks();
base::TimeTicks first_largest = LargestPaintStoredResult();
EXPECT_GE(first_largest, time1);
......@@ -244,7 +242,6 @@ TEST_F(TextPaintTimingDetectorTest, UpdateResultWhenCandidateChanged) {
AppendDivElementToBody("a long-long-long text");
UpdateAllLifecyclePhasesAndSimulateSwapTime();
UpdateCandidate();
base::TimeTicks time3 = NowTicks();
base::TimeTicks second_largest = LargestPaintStoredResult();
EXPECT_GE(second_largest, time2);
......@@ -335,14 +332,12 @@ TEST_F(TextPaintTimingDetectorTest, LargestTextPaint_ReportLastNullCandidate) {
)HTML");
Element* text = AppendDivElementToBody("text to remove");
UpdateAllLifecyclePhasesAndSimulateSwapTime();
UpdateCandidate();
EXPECT_EQ(TextRecordOfLargestTextPaint()->node_id,
DOMNodeIds::ExistingIdForNode(text));
EXPECT_NE(LargestPaintStoredResult(), base::TimeTicks());
RemoveElement(text);
UpdateAllLifecyclePhasesAndSimulateSwapTime();
UpdateCandidate();
EXPECT_FALSE(TextRecordOfLargestTextPaint());
EXPECT_EQ(LargestPaintStoredResult(), 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