Untemplatize PositionAlgorithm<Strategy>::rendersInDifferentPosition() (issue 1270023002 by yosin@chromium.org)

[yo...@chromium.org]

Reviewers: tkent, yoichio, hajimehoshi,

Message:
PTAL

Description:
Untemplatize PositionAlgorithm<Strategy>::rendersInDifferentPosition()

Because of |rendersInDifferentPosition()| is used only for DOM tree, this
patch
makes |PositionAlgorithm<Strategy>::rendersInDifferentPosition()| as
function rather than member function of |PositionAlgorithm<Strategy>|
template
class to avoid unused template function for improving code health.

Following patch move |rendersInDifferentPosition()| into "htmlediting.cpp"
since

it is used only in editing.

This patch is a preparation of templatizing of |VisiblePosition| for
granularity
support in selection on composed tree.

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


Please review this at https://codereview.chromium.org/1270023002/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+53, -43 lines):
M Source/core/dom/Position.h
M Source/core/dom/Position.cpp
M Source/core/editing/CompositeEditCommand.cpp
M Source/core/editing/htmlediting.cpp


Index: Source/core/dom/Position.cpp
diff --git a/Source/core/dom/Position.cpp b/Source/core/dom/Position.cpp
index
68fc3ac705d545b561cbff4d8e78c1867d51bd6c..fe9bd20247586014cff2d992c2f3e67378a38fff
100644
--- a/Source/core/dom/Position.cpp
+++ b/Source/core/dom/Position.cpp
@@ -580,24 +580,26 @@ bool PositionAlgorithm<Strategy>::atEndOfTree() const
return !Strategy::parent(*anchorNode()) && m_offset >=
lastOffsetForEditing(anchorNode());
}

-template <typename Strategy>
-int PositionAlgorithm<Strategy>::renderedOffset() const
+static int renderedOffsetOf(const Position& position)
{
- if (!anchorNode()->isTextNode())
- return m_offset;
+ // TODO(yosin) We should compute offset for |AfterAnchor| and
+ // |AfterChildren|.
+ int offset = position.deprecatedOffset();
+ if (!position.anchorNode()->isTextNode())
+ return offset;

- if (!anchorNode()->layoutObject())
- return m_offset;
+ if (!position.anchorNode()->layoutObject())
+ return offset;

int result = 0;
- LayoutText* textLayoutObject =
toLayoutText(anchorNode()->layoutObject());
+ LayoutText* textLayoutObject =
toLayoutText(position.anchorNode()->layoutObject());
for (InlineTextBox *box = textLayoutObject->firstTextBox(); box; box =
box->nextTextBox()) {
int start = box->start();
int end = box->start() + box->len();
- if (m_offset < start)
+ if (offset < start)
return result;
- if (m_offset <= end) {
- result += m_offset - start;
+ if (offset <= end) {
+ result += offset - start;
return result;
}
result += box->len();
@@ -1055,17 +1057,16 @@ bool
PositionAlgorithm<Strategy>::isRenderedCharacter() const
return false;
}

-template <typename Strategy>
-bool PositionAlgorithm<Strategy>::rendersInDifferentPosition(const
PositionAlgorithm<Strategy> &pos) const
+bool rendersInDifferentPosition(const Position& position1, const Position&
position2)
{
- if (isNull() || pos.isNull())
+ if (position1.isNull() || position2.isNull())
return false;

- LayoutObject* layoutObject = anchorNode()->layoutObject();
+ LayoutObject* layoutObject = position1.anchorNode()->layoutObject();
if (!layoutObject)
return false;

- LayoutObject* posLayoutObject = pos.anchorNode()->layoutObject();
+ LayoutObject* posLayoutObject = position2.anchorNode()->layoutObject();
if (!posLayoutObject)
return false;

@@ -1073,49 +1074,53 @@ bool
PositionAlgorithm<Strategy>::rendersInDifferentPosition(const PositionAlgor
|| posLayoutObject->style()->visibility() != VISIBLE)
return false;

- if (anchorNode() == pos.anchorNode()) {
- if (isHTMLBRElement(*anchorNode()))
+ if (position1.anchorNode() == position2.anchorNode()) {
+ if (isHTMLBRElement(*position1.anchorNode()))
return false;

- if (m_offset == pos.deprecatedEditingOffset())
+ // TODO(yosin) I'm not sure why do we compare
+ // |deprecatedOffset()| and |deprecatedEditingOffset()|.
+ if (position1.deprecatedOffset() ==
position2.deprecatedEditingOffset())
return false;

- if (!anchorNode()->isTextNode()
&& !pos.anchorNode()->isTextNode()) {
- if (m_offset != pos.deprecatedEditingOffset())
+ if (!position1.anchorNode()->isTextNode()
&& !position2.anchorNode()->isTextNode()) {
+ // TODO(yosin) I'm not sure why do we compare
+ // |deprecatedOffset()| and |deprecatedEditingOffset()|.
+ if (position1.deprecatedOffset() !=
position2.deprecatedEditingOffset())
return true;
}
}

- if (isHTMLBRElement(*anchorNode()) && pos.isCandidate())
+ if (isHTMLBRElement(*position1.anchorNode()) &&
position2.isCandidate())
return true;

- if (isHTMLBRElement(*pos.anchorNode()) && isCandidate())
+ if (isHTMLBRElement(*position2.anchorNode()) &&
position1.isCandidate())
return true;

- if (!inSameContainingBlockFlowElement(anchorNode(), pos.anchorNode()))
+ if (!inSameContainingBlockFlowElement(position1.anchorNode(),
position2.anchorNode()))
return true;

- if (anchorNode()->isTextNode() && !inRenderedText())
+ if (position1.anchorNode()->isTextNode()
&& !position1.inRenderedText())
return false;

- if (pos.anchorNode()->isTextNode() && !pos.inRenderedText())
+ if (position2.anchorNode()->isTextNode()
&& !position2.inRenderedText())
return false;

- int thisRenderedOffset = renderedOffset();
- int posRenderedOffset = pos.renderedOffset();
+ int renderedOffset1 = renderedOffsetOf(position1);
+ int renderedOffset2 = renderedOffsetOf(position2);

- if (layoutObject == posLayoutObject && thisRenderedOffset ==
posRenderedOffset)
+ if (layoutObject == posLayoutObject && renderedOffset1 ==
renderedOffset2)
return false;

- InlineBoxPosition boxPosition1 = computeInlineBoxPosition(DOWNSTREAM);
- InlineBoxPosition boxPosition2 =
pos.computeInlineBoxPosition(DOWNSTREAM);
+ InlineBoxPosition boxPosition1 =
position1.computeInlineBoxPosition(DOWNSTREAM);
+ InlineBoxPosition boxPosition2 =
position2.computeInlineBoxPosition(DOWNSTREAM);

WTF_LOG(Editing, "layoutObject: %p [%p]\n", layoutObject,
boxPosition1.inlineBox);
- WTF_LOG(Editing, "thisRenderedOffset: %d\n", thisRenderedOffset);
+ WTF_LOG(Editing, "renderedOffset1: %d\n", renderedOffset1);
WTF_LOG(Editing, "posLayoutObject: %p [%p]\n", posLayoutObject,
boxPosition2.inlineBox);
- WTF_LOG(Editing, "posRenderedOffset: %d\n", posRenderedOffset);
- WTF_LOG(Editing, "node min/max: %d:%d\n",
caretMinOffset(anchorNode()), caretMaxOffset(anchorNode()));
- WTF_LOG(Editing, "pos node min/max: %d:%d\n",
caretMinOffset(pos.anchorNode()), caretMaxOffset(pos.anchorNode()));
+ WTF_LOG(Editing, "renderedOffset2: %d\n", renderedOffset2);
+ WTF_LOG(Editing, "node min/max: %d:%d\n",
caretMinOffset(position1.anchorNode()),
caretMaxOffset(position1.anchorNode()));
+ WTF_LOG(Editing, "pos node min/max: %d:%d\n",
caretMinOffset(position2.anchorNode()),
caretMaxOffset(position2.anchorNode()));

WTF_LOG(Editing, "----------------------------------------------------------------------\n");

if (!boxPosition1.inlineBox || !boxPosition2.inlineBox) {
@@ -1126,13 +1131,13 @@ bool
PositionAlgorithm<Strategy>::rendersInDifferentPosition(const PositionAlgor
return true;
}

- if (nextRenderedEditable(anchorNode()) == pos.anchorNode()
- && thisRenderedOffset == caretMaxOffset(anchorNode())
&& !posRenderedOffset) {
+ if (nextRenderedEditable(position1.anchorNode()) ==
position2.anchorNode()
+ && renderedOffset1 == caretMaxOffset(position1.anchorNode())
&& !renderedOffset2) {
return false;
}

- if (previousRenderedEditable(anchorNode()) == pos.anchorNode()
- && !thisRenderedOffset && posRenderedOffset ==
caretMaxOffset(pos.anchorNode())) {
+ if (previousRenderedEditable(position1.anchorNode()) ==
position2.anchorNode()
+ && !renderedOffset1 && renderedOffset2 ==
caretMaxOffset(position2.anchorNode())) {
return false;
}

Index: Source/core/dom/Position.h
diff --git a/Source/core/dom/Position.h b/Source/core/dom/Position.h
index
5ae92d1a30215c9a5acd00406f5e44c8a748b474..562472c5e36bcb54fb1d5f57aea0ea36dc7ff5db
100644
--- a/Source/core/dom/Position.h
+++ b/Source/core/dom/Position.h
@@ -143,6 +143,12 @@ public:
// New code should not use this function.
int deprecatedEditingOffset() const;

+ // |deprecatedEditingOffset()| is used only for layout object related
+ // functions, |isRenderedCharacter()|, |inRenderedText()|, and
+ // |rendersInDifferentPosition()|.
+ // New code should not use this function.
+ int deprecatedOffset() const { return m_offset; }
+
// These are convenience methods which are smart about whether the
position is neighbor anchored or parent anchored
Node* computeNodeBeforePosition() const;
Node* computeNodeAfterPosition() const;
@@ -210,7 +216,6 @@ public:
bool isCandidate() const;
bool inRenderedText() const;
bool isRenderedCharacter() const;
- bool rendersInDifferentPosition(const PositionAlgorithm<Strategy>&)
const;

InlineBoxPosition computeInlineBoxPosition(EAffinity) const;
InlineBoxPosition computeInlineBoxPosition(EAffinity, TextDirection
primaryDirection) const;
@@ -253,8 +258,6 @@ private:
return isAfterAnchor() || isAfterChildren();
}

- int renderedOffset() const;
-
RefPtrWillBeMember<Node> m_anchorNode;
// m_offset can be the offset inside m_anchorNode, or if
editingIgnoresContent(m_anchorNode)
// returns true, then other places in editing will treat m_offset == 0
as "before the anchor"
@@ -270,6 +273,8 @@ extern template class CORE_EXTERN_TEMPLATE_EXPORT
PositionAlgorithm<EditingInCom
using Position = PositionAlgorithm<EditingStrategy>;
using PositionInComposedTree =
PositionAlgorithm<EditingInComposedTreeStrategy>;

+bool rendersInDifferentPosition(const Position&, const Position&);
+
inline Position createLegacyEditingPosition(PassRefPtrWillBeRawPtr<Node>
node, int offset)
{
return Position::createLegacyEditingPosition(node, offset);
Index: Source/core/editing/CompositeEditCommand.cpp
diff --git a/Source/core/editing/CompositeEditCommand.cpp
b/Source/core/editing/CompositeEditCommand.cpp
index
8f8bfc334d8a7097497969cf3e77eb37473fe353..dfadf0a0cd3a9854184b66217446a359d6488dec
100644
--- a/Source/core/editing/CompositeEditCommand.cpp
+++ b/Source/core/editing/CompositeEditCommand.cpp
@@ -1098,7 +1098,7 @@ void
CompositeEditCommand::cleanupAfterDeletion(VisiblePosition destination)
} else if (isBlock(node)) {
// If caret position after deletion and destination position
coincides,
// node should not be removed.
- if
(!position.rendersInDifferentPosition(destination.deepEquivalent())) {
+ if (!rendersInDifferentPosition(position,
destination.deepEquivalent())) {
prune(node, destinationNode);
return;
}
Index: Source/core/editing/htmlediting.cpp
diff --git a/Source/core/editing/htmlediting.cpp
b/Source/core/editing/htmlediting.cpp
index
4a49061042fdf7866281f5949a87780fc5247c16..6f343cd9db4d35a77ad4423f01c5c5188d28dc33
100644
--- a/Source/core/editing/htmlediting.cpp
+++ b/Source/core/editing/htmlediting.cpp
@@ -1121,7 +1121,7 @@ static Position previousCharacterPosition(const
Position& position, EAffinity af
if (atStartOfLine || !rendered) {
if (currentPos.isCandidate())
return currentPos;
- } else if (position.rendersInDifferentPosition(currentPos)) {
+ } else if (rendersInDifferentPosition(position, currentPos)) {
return currentPos;
}
}

[yoi...@chromium.org]

https://codereview.chromium.org/1270023002/diff/1/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/1270023002/diff/1/Source/core/dom/Position.cpp#newcode1081
Source/core/dom/Position.cpp:1081: // TODO(yosin) I'm not sure why do we
compare
This is not good comment.
You should be sure or this is a refactoring so the comment is not
needed.

https://codereview.chromium.org/1270023002/

[yo...@chromium.org]

PTAL

https://codereview.chromium.org/1270023002/diff/1/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/1270023002/diff/1/Source/core/dom/Position.cpp#newcode1081
Source/core/dom/Position.cpp:1081: // TODO(yosin) I'm not sure why do we
compare

On 2015/08/03 06:19:57, yoichio wrote:
> This is not good comment.
> You should be sure or this is a refactoring so the comment is not
needed.

Done.

https://codereview.chromium.org/1270023002/

[yoi...@chromium.org]

lgtm



https://codereview.chromium.org/1270023002/

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

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1270023002/20001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1270023002/20001


https://codereview.chromium.org/1270023002/

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

Try jobs failed on following builders:
blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/38957)

https://codereview.chromium.org/1270023002/

[tk...@chromium.org]

https://codereview.chromium.org/1270023002/diff/20001/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/1270023002/diff/20001/Source/core/dom/Position.cpp#newcode1081
Source/core/dom/Position.cpp:1081: if (position1.deprecatedOffset() ==
position2.deprecatedEditingOffset())
Comparing deprecatedOffset and deprecatedEditingOffset looks
inconsistent to me. Is it intentional?

https://codereview.chromium.org/1270023002/

[yo...@chromium.org]

https://codereview.chromium.org/1270023002/diff/20001/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/1270023002/diff/20001/Source/core/dom/Position.cpp#newcode1081
Source/core/dom/Position.cpp:1081: if (position1.deprecatedOffset() ==
position2.deprecatedEditingOffset())

On 2015/08/03 08:16:29, tkent wrote:
> Comparing deprecatedOffset and deprecatedEditingOffset looks
inconsistent to me.
> Is it intentional?

This patch simplify replaces |m_offset| to |deprecatedOffset()|.
We need farther study why original code uses |m_offset|.

https://codereview.chromium.org/1270023002/

[yo...@chromium.org]

On 2015/08/03 08:16:29, tkent wrote:

https://codereview.chromium.org/1270023002/diff/20001/Source/core/dom/Position.cpp
> File Source/core/dom/Position.cpp (right):


https://codereview.chromium.org/1270023002/diff/20001/Source/core/dom/Position.cpp#newcode1081
> Source/core/dom/Position.cpp:1081: if (position1.deprecatedOffset() ==
> position2.deprecatedEditingOffset())
> Comparing deprecatedOffset and deprecatedEditingOffset looks inconsistent
> to
me.
> Is it intentional?

Code change itself is identical. But, it is wrong.
I close this patch and submit another patch to use |
deprecatedEdigintOffset()|:
http://crrev.com/1272973002

https://codereview.chromium.org/1270023002/