Use Position instead of VisiblePosition for SurroundingText.

Use Position instead of VisiblePosition for SurroundingText.

Using VisiblePosition has some not required performance implications.

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175788

[mlamouri (slow - plz ping), 2014-06-03 11:30:08]

As requested here https://codereview.chromium.org/294073005/, I have updated
SurroundingText API to take a Position instead of a VisiblePosition.

Yosi and Yuta, could you take a look please? :)

[yosin_UTC9, 2014-06-04 01:04:05]

ACK
Thanks for reducing editing team job! (^_^)b

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
File Source/core/editing/SurroundingText.cpp (right):

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingText.cpp:50: ASSERT(document);
nit: We don't need to have this |ASSERT(document)|, because we check
|Position.isNull()| on L44.

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
File Source/core/editing/SurroundingTextTest.cpp (right):

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingTextTest.cpp:22: virtual void SetUp() OVERRIDE;
nit: We can make |SetUp()| as private.

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingTextTest.cpp:61: EXPECT_EQ(unsigned(0),
surroundingText.positionOffsetInContent());
nit: 0u
I couldn't find any usage of |unsinged(x)| in Blink and Chromium, except for
|unsinged(non-literal)|.

[yosin_UTC9, 2014-06-04 01:13:10]

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
File Source/core/editing/SurroundingText.cpp (right):

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingText.cpp:50: ASSERT(document);
On 2014/06/04 01:04:05, yosi wrote:
> nit: We don't need to have this |ASSERT(document)|, because we check
> |Position.isNull()| on L44.

How about unify |Position::isNull| and |Position::document|?

Document* document = position.document();
if (!document)
   return;

[mlamouri (slow - plz ping), 2014-06-04 09:52:12]

Thank you yosi, I've applied all your review comments.

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
File Source/core/editing/SurroundingText.cpp (right):

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingText.cpp:50: ASSERT(document);
On 2014/06/04 01:13:10, yosi wrote:
> On 2014/06/04 01:04:05, yosi wrote:
> > nit: We don't need to have this |ASSERT(document)|, because we check
> > |Position.isNull()| on L44.
> 
> How about unify |Position::isNull| and |Position::document|?
> 
> Document* document = position.document();
> if (!document)
>    return;

Done.

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
File Source/core/editing/SurroundingTextTest.cpp (right):

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingTextTest.cpp:22: virtual void SetUp() OVERRIDE;
On 2014/06/04 01:04:05, yosi wrote:
> nit: We can make |SetUp()| as private.

Done.

https://codereview.chromium.org/307353002/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingTextTest.cpp:61: EXPECT_EQ(unsigned(0),
surroundingText.positionOffsetInContent());
On 2014/06/04 01:04:05, yosi wrote:
> nit: 0u
> I couldn't find any usage of |unsinged(x)| in Blink and Chromium, except for
> |unsinged(non-literal)|.

Done.

[mlamouri (slow - plz ping), 2014-06-04 13:48:16]

Yosi, I have converted the layout tests to unit tests. I think it is better to
have unit tests for that feature. What do you think?

PTAL. Thanks :)

[mlamouri (slow - plz ping), 2014-06-04 14:56:38]

https://codereview.chromium.org/307353002/diff/40001/Source/web/WebSurroundin...
File Source/web/WebSurroundingText.cpp (right):

https://codereview.chromium.org/307353002/diff/40001/Source/web/WebSurroundin...
Source/web/WebSurroundingText.cpp:48: m_private.reset(new
SurroundingText(node->renderer()->positionForPoint(static_cast<IntPoint>(nodePoint)).position(),
maxLength));
If I manually test this on Android, it is crashing. Do you know what could be
wrong?

[mlamouri (slow - plz ping), 2014-06-05 15:05:34]

Yosi, I found out my problem: the range I was creating wasn't right so I
introduced a makeRange(Position,Position) helper, like the
makeRange(VisiblePosition,VisiblePosition). I reverted my change removing the
layout tests to make this CL smaller. I will do that in another CL.

This is blocking a couple of other CL so it would be great if you could have a
look today. Thanks! :)

[Yuta Kitamura, 2014-06-06 08:57:30]

Mostly looking good other than the addition of makeRange().

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

https://codereview.chromium.org/307353002/diff/60001/Source/core/dom/Position...
Source/core/dom/Position.cpp:1295: Position e = end.parentAnchoredEquivalent();
What's the point of getting parent-anchored version of
|start| and |end|?

https://codereview.chromium.org/307353002/diff/60001/Source/core/dom/Position.h
File Source/core/dom/Position.h (right):

https://codereview.chromium.org/307353002/diff/60001/Source/core/dom/Position...
Source/core/dom/Position.h:321: PassRefPtrWillBeRawPtr<Range> makeRange(const
Position&, const Position&);
I'm not sure what's the difference between this function and
Range::create(Document&, const Position&, const Position&).

[mlamouri (slow - plz ping), 2014-06-06 09:23:16]

Thank you for the quick review! :)

I applied your comments. PTAL

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

https://codereview.chromium.org/307353002/diff/60001/Source/core/dom/Position...
Source/core/dom/Position.cpp:1295: Position e = end.parentAnchoredEquivalent();
On 2014/06/06 08:57:30, Yuta Kitamura wrote:
> What's the point of getting parent-anchored version of
> |start| and |end|?

Removed. I'm calling directly Range::create() instead.

https://codereview.chromium.org/307353002/diff/60001/Source/core/dom/Position.h
File Source/core/dom/Position.h (right):

https://codereview.chromium.org/307353002/diff/60001/Source/core/dom/Position...
Source/core/dom/Position.h:321: PassRefPtrWillBeRawPtr<Range> makeRange(const
Position&, const Position&);
On 2014/06/06 08:57:30, Yuta Kitamura wrote:
> I'm not sure what's the difference between this function and
> Range::create(Document&, const Position&, const Position&).

Removed.

[Yuta Kitamura, 2014-06-09 01:52:28]

LGTM, but you still need to get an approval from someone in
Source/web's OWNERS.

[tkent, 2014-06-09 02:02:39]

rslgtm

[mlamouri (slow - plz ping), 2014-06-09 09:43:25]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-09 09:43:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/307353002/80001

[commit-bot: I haz the power, 2014-06-09 10:47:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10812)

[commit-bot: I haz the power, 2014-06-09 11:20:04]

Change committed as 175788