Make SurroundingText work for a Range.

Make SurroundingText work for a Range.

It generalize the algorithm to find the surrounding text to be able to
work with a starting position and an ending position. The goal is to be
able to get the surrounding text arround the current selection.

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

[mlamouri (slow - plz ping), 2014-06-10 11:15:20]

I've split up the change from the Blink API exposure so we can discuss the
editing/ details separately if needed.

[yosin_UTC9, 2014-06-11 01:04:29]

ACK

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

https://codereview.chromium.org/323983006/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingText.cpp:96: m_startOffsetInContent =
Range::create(*document, backwardsRange->endPosition(),
startPosition)->text().length();
nit: Can we defer calculating start/end offset in content? It seems we don't
need to have |m_startOffsetInContent| and |m_endOffsetInContent|.
This is very optional.

[mlamouri (slow - plz ping), 2014-06-11 09:19:54]

Thank you for the review yosi@.

tkent@, does the ACK need that I need a stamp from an OWNER now? Or do I need
another review?

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

https://codereview.chromium.org/323983006/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingText.cpp:96: m_startOffsetInContent =
Range::create(*document, backwardsRange->endPosition(),
startPosition)->text().length();
On 2014/06/11 01:04:28, yosi wrote:
> nit: Can we defer calculating start/end offset in content? It seems we don't
> need to have |m_startOffsetInContent| and |m_endOffsetInContent|.
> This is very optional.

This is as optional as m_positionOffsetInContent and we currently compute it, so
I'm not sure why we should stop doing it now?

This said, regardless of what we currently do, I would not delay calculating
m_startOffsetInContent and m_endOffsetInContent because it would require
SurroundingText to keep a reference on |startPosition| and |endPosition| such as
|backwardsRange|. It is a lot of things to keep in memory just so we can compute
to integers. In addition, all callers use startOffsetInContent and/or
endOffsetInContent and I would expect this to stay as is given that
SurroundingText will return the content() be the caller might unlikely know what
is the initial Positions in that content().

[Yuta Kitamura, 2014-06-11 09:22:57]

LGTM with one obvious nitpick on function name

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

https://codereview.chromium.org/323983006/diff/1/Source/core/editing/Surround...
Source/core/editing/SurroundingText.cpp:56: void SurroundingText::process(const
Position& startPosition, const Position& endPosition, unsigned maxLength)
The function name "process" is pretty much clueless;
I'd name this as "initialize".

[mlamouri (slow - plz ping), 2014-06-11 10:10:57]

Thanks! :) I've applied all your comments.

Tamura-san, could you have a look at the changes in Source/web/ ?

[tkent, 2014-06-12 00:27:53]

lgtm.

yosi, what do you mean by "ACK"?

[yosin_UTC9, 2014-06-12 00:51:12]

On 2014/06/12 00:27:53, tkent wrote:
> lgtm.
> 
> yosi, what do you mean by "ACK"?

tkent@, thanks for asking. I've been word "ACK" wrongly. I mean "LGTM, please
wait for OWNER's review".
I reviewed detail rather than glancing.
http://dev.chromium.org/developers/committers-responsibility

[Yuta Kitamura, 2014-06-12 06:31:06]

(I actually don't know what exactly "ACK" means...
 Yoshi is the only person I know who uses it, and IMHO
 uncommon acronyms should be discouraged as they can
 make other people get confused...)

[mlamouri (slow - plz ping), 2014-06-12 08:55:28]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-12 08:56:01]

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

[commit-bot: I haz the power, 2014-06-12 10:07:14]

Change committed as 176015