Make VisiblePosition::FirstPositionInNode() to take const Node& instead of Node* (issue 2955823002 by yosin@chromium.org)

[yosin@chromium.org via codereview.chromium.org]

Reviewers: tkent
CL: https://codereview.chromium.org/2955823002/

Message:
PTAL

Including core/editing, core/html and core/svg

Description:
Make VisiblePosition::FirstPositionInNode() to take const Node& instead of Node*

This patch makes |VisiblePosition::FirstPositionInNode()| to take |const Node&|
as |Position::FirstPositionInNode()| for improving code health.

This is mechanical change:
s/VisiblePositon::FirstPositionInNode(/VisiblePositon::FirstPositionInNode(*/


BUG=n/a
TEST=n/a; no behavior changes


Affected files (+20, -20 lines):
M third_party/WebKit/Source/core/editing/VisiblePosition.h
M third_party/WebKit/Source/core/editing/VisiblePosition.cpp
M third_party/WebKit/Source/core/editing/VisibleUnits.cpp
M third_party/WebKit/Source/core/editing/VisibleUnitsLine.cpp
M third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
M third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
M third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp
M third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
M third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
M third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
M third_party/WebKit/Source/core/html/TextControlElement.cpp
M third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp


Index: third_party/WebKit/Source/core/editing/VisiblePosition.cpp
diff --git a/third_party/WebKit/Source/core/editing/VisiblePosition.cpp b/third_party/WebKit/Source/core/editing/VisiblePosition.cpp
index e119c149d1e30e2fc66e8a0718af86e73eeedf83..261b75c507668c6f36b4e94543196c4941ccc4d3 100644
--- a/third_party/WebKit/Source/core/editing/VisiblePosition.cpp
+++ b/third_party/WebKit/Source/core/editing/VisiblePosition.cpp
@@ -116,9 +116,9 @@ VisiblePositionTemplate<Strategy> VisiblePositionTemplate<Strategy>::BeforeNode(

template <typename Strategy>
VisiblePositionTemplate<Strategy>
-VisiblePositionTemplate<Strategy>::FirstPositionInNode(Node* node) {
+VisiblePositionTemplate<Strategy>::FirstPositionInNode(const Node& node) {
return Create(PositionWithAffinityTemplate<Strategy>(
- PositionTemplate<Strategy>::FirstPositionInNode(*node)));
+ PositionTemplate<Strategy>::FirstPositionInNode(node)));
}

template <typename Strategy>
Index: third_party/WebKit/Source/core/editing/VisiblePosition.h
diff --git a/third_party/WebKit/Source/core/editing/VisiblePosition.h b/third_party/WebKit/Source/core/editing/VisiblePosition.h
index a98122b7a82a0e22aac3224c8a1928b4025f46f0..0e0f39b298ecc8a1919361a465fb9f8cbd0ef83b 100644
--- a/third_party/WebKit/Source/core/editing/VisiblePosition.h
+++ b/third_party/WebKit/Source/core/editing/VisiblePosition.h
@@ -106,7 +106,7 @@ class CORE_TEMPLATE_CLASS_EXPORT VisiblePositionTemplate final {

static VisiblePositionTemplate<Strategy> AfterNode(const Node&);
static VisiblePositionTemplate<Strategy> BeforeNode(const Node&);
- static VisiblePositionTemplate<Strategy> FirstPositionInNode(Node*);
+ static VisiblePositionTemplate<Strategy> FirstPositionInNode(const Node&);
static VisiblePositionTemplate<Strategy> InParentAfterNode(const Node&);
static VisiblePositionTemplate<Strategy> InParentBeforeNode(const Node&);
static VisiblePositionTemplate<Strategy> LastPositionInNode(Node*);
Index: third_party/WebKit/Source/core/editing/VisibleUnits.cpp
diff --git a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
index 868deaadf37992cf1a6fd3ded0c31678678f3ad0..13798f33ae528b8e660a9b4f4aee1e3cdea0adf7 100644
--- a/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
+++ b/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
@@ -549,7 +549,7 @@ VisiblePosition StartOfBlock(const VisiblePosition& visible_position,
position.ComputeContainerNode()
? EnclosingBlock(position.ComputeContainerNode(), rule)
: 0;
- return start_block ? VisiblePosition::FirstPositionInNode(start_block)
+ return start_block ? VisiblePosition::FirstPositionInNode(*start_block)
: VisiblePosition();
}

@@ -643,7 +643,7 @@ VisiblePosition StartOfEditableContent(
if (!highest_root)
return VisiblePosition();

- return VisiblePosition::FirstPositionInNode(highest_root);
+ return VisiblePosition::FirstPositionInNode(*highest_root);
}

VisiblePosition EndOfEditableContent(const VisiblePosition& visible_position) {
Index: third_party/WebKit/Source/core/editing/VisibleUnitsLine.cpp
diff --git a/third_party/WebKit/Source/core/editing/VisibleUnitsLine.cpp b/third_party/WebKit/Source/core/editing/VisibleUnitsLine.cpp
index 37ed8d25ee5d1f397cc8135c01bb9596a872960d..659e3da52c00c3321ea51bf3dd4432ad2c61b523 100644
--- a/third_party/WebKit/Source/core/editing/VisibleUnitsLine.cpp
+++ b/third_party/WebKit/Source/core/editing/VisibleUnitsLine.cpp
@@ -686,7 +686,7 @@ VisiblePosition PreviousLinePosition(const VisiblePosition& visible_position,
: node->GetDocument().documentElement();
if (!root_element)
return VisiblePosition();
- return VisiblePosition::FirstPositionInNode(root_element);
+ return VisiblePosition::FirstPositionInNode(*root_element);
}

VisiblePosition NextLinePosition(const VisiblePosition& visible_position,
Index: third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp b/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
index 56ff069d4dc3269c984b5517e04accd9665028c2..3c41955a657bdebf39d92e473099df1f6290cec1 100644
--- a/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
@@ -1067,7 +1067,7 @@ HTMLElement* CompositeEditCommand::MoveParagraphContentsToNewBlockIfNecessary(
visible_paragraph_start = StartOfParagraph(visible_pos);
visible_paragraph_end = EndOfParagraph(visible_pos);
MoveParagraphs(visible_paragraph_start, visible_paragraph_end,
- VisiblePosition::FirstPositionInNode(new_block),
+ VisiblePosition::FirstPositionInNode(*new_block),
editing_state);
if (editing_state->IsAborted())
return nullptr;
@@ -1798,7 +1798,7 @@ Position CompositeEditCommand::PositionAvoidingSpecialElementBoundary(
// wrong paragraph.
if (enclosing_anchor && !IsEnclosingBlock(enclosing_anchor)) {
VisiblePosition first_in_anchor =
- VisiblePosition::FirstPositionInNode(enclosing_anchor);
+ VisiblePosition::FirstPositionInNode(*enclosing_anchor);
VisiblePosition last_in_anchor =
VisiblePosition::LastPositionInNode(enclosing_anchor);
// If visually just after the anchor, insert *inside* the anchor unless it's
@@ -1882,7 +1882,7 @@ Node* CompositeEditCommand::SplitTreeToNode(Node* start,

// Do not split a node when doing so introduces an empty node.
VisiblePosition position_in_parent =
- VisiblePosition::FirstPositionInNode(parent_element);
+ VisiblePosition::FirstPositionInNode(*parent_element);
VisiblePosition position_in_node =
CreateVisiblePosition(FirstPositionInOrBeforeNode(node));
if (position_in_parent.DeepEquivalent() !=
Index: third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp b/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
index c3d742efee6cddec3b66f3168d3f3b984da88eff..3e00ab7063cacb72f1fb9bcfb46497d774d170b8 100644
--- a/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
@@ -48,7 +48,7 @@ using namespace HTMLNames;

static bool IsTableCellEmpty(Node* cell) {
DCHECK(IsTableCell(cell)) << cell;
- return VisiblePosition::FirstPositionInNode(cell).DeepEquivalent() ==
+ return VisiblePosition::FirstPositionInNode(*cell).DeepEquivalent() ==
VisiblePosition::LastPositionInNode(cell).DeepEquivalent();
}

@@ -513,7 +513,7 @@ void DeleteSelectionCommand::RemoveNode(
GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
if (node == start_block_) {
VisiblePosition previous = PreviousPositionOf(
- VisiblePosition::FirstPositionInNode(start_block_.Get()));
+ VisiblePosition::FirstPositionInNode(*start_block_.Get()));
if (previous.IsNotNull() && !IsEndOfBlock(previous))
need_placeholder_ = true;
}
Index: third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp b/third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp
index 7bd2a581cf3f34c3488fb3a069bfae1ebf363356..dda03865f00eabbec295fdab1a6c738f6752f45a 100644
--- a/third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/IndentOutdentCommand.cpp
@@ -227,7 +227,7 @@ void IndentOutdentCommand::OutdentParagraph(EditingState* editing_state) {

// The selection is inside a blockquote i.e. enclosingNode is a blockquote
VisiblePosition position_in_enclosing_block =
- VisiblePosition::FirstPositionInNode(enclosing_element);
+ VisiblePosition::FirstPositionInNode(*enclosing_element);
// If the blockquote is inline, the start of the enclosing block coincides
// with positionInEnclosingBlock.
VisiblePosition start_of_enclosing_block =
Index: third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp b/third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
index b3a5d0f7ef2ee9c0c6719d253fd41fe719e675f6..5d6f8c884035865639dd5617b70bf4788feb49c6 100644
--- a/third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/InsertListCommand.cpp
@@ -384,7 +384,7 @@ bool InsertListCommand::DoApplyForSingleParagraph(

GetDocument().UpdateStyleAndLayoutIgnorePendingStylesheets();
Node* first_child_in_list =
- EnclosingListChild(VisiblePosition::FirstPositionInNode(list_element)
+ EnclosingListChild(VisiblePosition::FirstPositionInNode(*list_element)
.DeepEquivalent()
.AnchorNode(),
list_element);
@@ -394,7 +394,7 @@ bool InsertListCommand::DoApplyForSingleParagraph(
: list_element;

MoveParagraphWithClones(
- VisiblePosition::FirstPositionInNode(list_element),
+ VisiblePosition::FirstPositionInNode(*list_element),
VisiblePosition::LastPositionInNode(list_element), new_list,
outer_block, editing_state);
if (editing_state->IsAborted())
@@ -460,7 +460,7 @@ void InsertListCommand::UnlistifyParagraph(
VisiblePosition end;
DCHECK(list_child_node);
if (isHTMLLIElement(*list_child_node)) {
- start = VisiblePosition::FirstPositionInNode(list_child_node);
+ start = VisiblePosition::FirstPositionInNode(*list_child_node);
end = VisiblePosition::LastPositionInNode(list_child_node);
next_list_child = list_child_node->nextSibling();
previous_list_child = list_child_node->previousSibling();
Index: third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp b/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
index 7a6123b936026a9bdde19929b783e84ba58ccdda..a28357e8d29937dd7e01b2d601628aee23a6b678 100644
--- a/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/ReplaceSelectionCommand.cpp
@@ -630,9 +630,9 @@ void ReplaceSelectionCommand::RemoveRedundantStylesAndKeepStyleSpanInline(
// FIXME: Tolerate differences in id, class, and style attributes.
if (element->parentNode() && IsNonTableCellHTMLBlockElement(element) &&
AreIdenticalElements(*element, *element->parentNode()) &&
- VisiblePosition::FirstPositionInNode(element->parentNode())
+ VisiblePosition::FirstPositionInNode(*element->parentNode())
.DeepEquivalent() ==
- VisiblePosition::FirstPositionInNode(element).DeepEquivalent() &&
+ VisiblePosition::FirstPositionInNode(*element).DeepEquivalent() &&
VisiblePosition::LastPositionInNode(element->parentNode())
.DeepEquivalent() ==
VisiblePosition::LastPositionInNode(element).DeepEquivalent()) {
Index: third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
diff --git a/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp b/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
index 3dbbedfd4cf410ef14c032281470846c25446276..46e23a955f615725a7310d28ccdff9c6ab0d74cf 100644
--- a/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
+++ b/third_party/WebKit/Source/core/editing/commands/TypingCommand.cpp
@@ -769,7 +769,7 @@ void TypingCommand::DeleteKeyPressed(TextGranularity granularity,
if (enclosing_table_cell &&
visible_start.DeepEquivalent() ==
VisiblePosition::FirstPositionInNode(
- const_cast<Node*>(enclosing_table_cell))
+ *const_cast<Node*>(enclosing_table_cell))
.DeepEquivalent())
return;

Index: third_party/WebKit/Source/core/html/TextControlElement.cpp
diff --git a/third_party/WebKit/Source/core/html/TextControlElement.cpp b/third_party/WebKit/Source/core/html/TextControlElement.cpp
index d05dac2d0005878ceda446a2f871fca6c263b76d..284c4111e1a86db4b5cb763b6ed663795295f32e 100644
--- a/third_party/WebKit/Source/core/html/TextControlElement.cpp
+++ b/third_party/WebKit/Source/core/html/TextControlElement.cpp
@@ -459,7 +459,7 @@ bool TextControlElement::CacheSelection(unsigned start,

VisiblePosition TextControlElement::VisiblePositionForIndex(int index) const {
if (index <= 0)
- return VisiblePosition::FirstPositionInNode(InnerEditorElement());
+ return VisiblePosition::FirstPositionInNode(*InnerEditorElement());
Position start, end;
bool selected = Range::selectNodeContents(InnerEditorElement(), start, end);
if (!selected)
Index: third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp b/third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp
index 9a1ea04b7e228cf9c1bd39cc086587d4c9aab8b0..8e9533b61cfb54646819e1aec0cc149fb94f28a9 100644
--- a/third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGTextContentElement.cpp
@@ -215,7 +215,7 @@ void SVGTextContentElement::selectSubString(unsigned charnum,

// Find selection start
VisiblePosition start = VisiblePosition::FirstPositionInNode(
- const_cast<SVGTextContentElement*>(this));
+ *const_cast<SVGTextContentElement*>(this));
for (unsigned i = 0; i < charnum; ++i)
start = NextPositionOf(start);

[tk...@chromium.org]

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
File
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp#newcode1070
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:1070:
VisiblePosition::FirstPositionInNode(*new_block),
Is new_block always non-null?

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
File
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
(right):

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp#newcode49
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:49:
static bool IsTableCellEmpty(Node* cell) {
Please make the |cell| argument |Node&|, or add |DCHECK(cell)|.
It's hard to confirm |cell| is never null at a glance.

https://codereview.chromium.org/2955823002/

[yosin@chromium.org via codereview.chromium.org]

PTAL

Add DCHECKs

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
File
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp
(right):

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp#newcode1070
third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:1070:
VisiblePosition::FirstPositionInNode(*new_block),

On 2017/06/26 at 08:08:22, tkent wrote:
> Is new_block always non-null?

Yes, |new_block| holds return value of
InsertNewDefaultParagraphElementAt() which is non-null
except for abort case.

I'll add DCHECK(new_block) for sane.

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
File
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp
(right):

https://codereview.chromium.org/2955823002/diff/1/third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp#newcode49
third_party/WebKit/Source/core/editing/commands/DeleteSelectionCommand.cpp:49:
static bool IsTableCellEmpty(Node* cell) {

On 2017/06/26 at 08:08:22, tkent wrote:
> Please make the |cell| argument |Node&|, or add |DCHECK(cell)|.
> It's hard to confirm |cell| is never null at a glance.

Done.

https://codereview.chromium.org/2955823002/

[tk...@chromium.org]

lgtm



https://codereview.chromium.org/2955823002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2955823002/20001

https://codereview.chromium.org/2955823002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/cbf7050ee81b7ca0390e634cfd08f764f33f1307

https://codereview.chromium.org/2955823002/