Extend WebSurroundingText to accept a WebRange.

Extend WebSurroundingText to accept a WebRange.

This will make it possible to get the text surrounding the
current selection by calling WebFrame::selectionRange() and
passing that range to WebSurroundingText.

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

[mlamouri (slow - plz ping), 2014-05-21 13:37:21]

jochen@, let me know if you want someone else to review the core/editing/
changes.

[jochen (gone - plz use gerrit), 2014-05-22 08:38:58]

On 2014/05/21 13:37:21, Mounir Lamouri wrote:
> jochen@, let me know if you want someone else to review the core/editing/
> changes.

I'd recommend tkent@

[mlamouri (slow - plz ping), 2014-05-22 08:46:34]

tkent@, could you have a look at this CL please? :)

[tkent, 2014-05-22 08:51:14]

+yosin and yutak because this is editing-related.

[yosin_UTC9, 2014-05-22 09:03:34]

Could you tell us how will you use this new API for better understanding of this
patch?

Can we use two |Position| for specifying range rather than using
|VisibleSelection|?
To support shadow DOM tree, we would like to use |Position|.
Thanks in advance.
-yosi

[mlamouri (slow - plz ping), 2014-05-22 09:34:21]

On 2014/05/22 09:03:34, yosi wrote:
> Could you tell us how will you use this new API for better understanding of
this
> patch?

See https://codereview.chromium.org/292113008/ such as the linked bug.

> Can we use two |Position| for specifying range rather than using
> |VisibleSelection|?
> To support shadow DOM tree, we would like to use |Position|.

Hmm, I'm not sure how using |Position| is related to this change. I'm splitting
VisibleSelection into two VisiblePositions, to follow the current flow of the
code. We could have a ctor taking two |Position| in which case, VisibleSelection
would be split into two |Position| instead of two |VisiblePosition|. My change
sounds orthogonal to that and not incompatible, doesn't it?

[yosin_UTC9, 2014-05-22 09:57:35]

On 2014/05/22 09:34:21, Mounir Lamouri wrote:
> On 2014/05/22 09:03:34, yosi wrote:
> > Could you tell us how will you use this new API for better understanding of
> this
> > patch?
> 
> See https://codereview.chromium.org/292113008/ such as the linked bug.
> 
> > Can we use two |Position| for specifying range rather than using
> > |VisibleSelection|?
> > To support shadow DOM tree, we would like to use |Position|.
> 
> Hmm, I'm not sure how using |Position| is related to this change. I'm
splitting
> VisibleSelection into two VisiblePositions, to follow the current flow of the
> code. We could have a ctor taking two |Position| in which case,
VisibleSelection
> would be split into two |Position| instead of two |VisiblePosition|. My change
> sounds orthogonal to that and not incompatible, doesn't it?

We would like to reduce usage of |VisibleSelection|, because VisibleSelection
ctor does some canonicalizataion. Also, in SurroundingText, you use
visibleStart() and visibleEnd(). These functions can cause layout update, so we
would like to avoid to use unless we really need.

It seems two |Positions| for SurroundingText is enough, and call site in
WebLocalFrame can be simpler as:
SurroundingText surroundingText(frame()->selection().start(),
frame()->selection().end())

[mlamouri (slow - plz ping), 2014-05-22 09:59:12]

On 2014/05/22 09:57:35, yosi wrote:
> On 2014/05/22 09:34:21, Mounir Lamouri wrote:
> > On 2014/05/22 09:03:34, yosi wrote:
> > > Could you tell us how will you use this new API for better understanding
of
> > this
> > > patch?
> > 
> > See https://codereview.chromium.org/292113008/ such as the linked bug.
> > 
> > > Can we use two |Position| for specifying range rather than using
> > > |VisibleSelection|?
> > > To support shadow DOM tree, we would like to use |Position|.
> > 
> > Hmm, I'm not sure how using |Position| is related to this change. I'm
> splitting
> > VisibleSelection into two VisiblePositions, to follow the current flow of
the
> > code. We could have a ctor taking two |Position| in which case,
> VisibleSelection
> > would be split into two |Position| instead of two |VisiblePosition|. My
change
> > sounds orthogonal to that and not incompatible, doesn't it?
> 
> We would like to reduce usage of |VisibleSelection|, because VisibleSelection
> ctor does some canonicalizataion. Also, in SurroundingText, you use
> visibleStart() and visibleEnd(). These functions can cause layout update, so
we
> would like to avoid to use unless we really need.
> 
> It seems two |Positions| for SurroundingText is enough, and call site in
> WebLocalFrame can be simpler as:
> SurroundingText surroundingText(frame()->selection().start(),
> frame()->selection().end())

SurroundingText is currently using VisiblePosition. I guess we would want it to
use Position intsead in that case. It seems that converting everything from
VisiblePosition to Position is another work, isn't?

[yosin_UTC9, 2014-05-23 01:04:02]

> SurroundingText is currently using VisiblePosition. I guess we would want it
to
> use Position intsead in that case. It seems that converting everything from
> VisiblePosition to Position is another work, isn't?
Agreed. Editing team will change SurroundingText and BackwardCharacterIterator
to use Position.

However, I would like to hear from yutak@, he is working on
BackwardsCharacterIterator now. He might have another point of view or better
solution for both end.

[mlamouri (slow - plz ping), 2014-05-30 14:11:55]

On 2014/05/23 01:04:02, yosi wrote:
> > SurroundingText is currently using VisiblePosition. I guess we would want it
> to
> > use Position intsead in that case. It seems that converting everything from
> > VisiblePosition to Position is another work, isn't?
> Agreed. Editing team will change SurroundingText and BackwardCharacterIterator
> to use Position.
> 
> However, I would like to hear from yutak@, he is working on
> BackwardsCharacterIterator now. He might have another point of view or better
> solution for both end.

I had a look with regards to use Position instead of VisiblePosition but I
blocked when I had to change endDocument() and startOfDocument(). Those methods
are currently related to VisibleUnits/VisiblePosition and using Position instead
doesn't seem trivial to me.

Furthermore, in the Chromium side of the review, I was asked if SurroundingText
could use unsigned instead of int. What do you think?

PTAL

[mlamouri (slow - plz ping), 2014-06-03 12:39:47]

I have rebased this CL on top of https://codereview.chromium.org/307353002/
which makes SurroundingText depending on Position instead of VisiblePosition.

Yosi and Yuta, could you PTAL?

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

I have rebased the CL on top of https://codereview.chromium.org/307353002/ and
changed the exposed WebFrame API to use |int| instead of |unsigned| because of
review comments on the content/ side of things.

[Yuta Kitamura, 2014-06-06 02:20:08]

https://codereview.chromium.org/294073005/diff/80001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/294073005/diff/80001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:484: virtual WebString textSurroundingSelection(int
maxLength, int* startOffset, int* endOffset) const = 0;
1. We usually use non-const references for output arguments,
   instead of non-const pointers.

2. I'm not sure why we need to add this method to WebFrame.
   Is there any reason you cannot extend WebSurroundingText
   interface?

[mlamouri (slow - plz ping), 2014-06-09 10:39:27]

On 2014/06/06 02:20:08, Yuta Kitamura wrote:
> https://codereview.chromium.org/294073005/diff/80001/public/web/WebFrame.h
> File public/web/WebFrame.h (right):
> 
>
https://codereview.chromium.org/294073005/diff/80001/public/web/WebFrame.h#ne...
> public/web/WebFrame.h:484: virtual WebString textSurroundingSelection(int
> maxLength, int* startOffset, int* endOffset) const = 0;
> 1. We usually use non-const references for output arguments,
>    instead of non-const pointers.

Interesting. Isn't that the opposite in Chromium?

> 2. I'm not sure why we need to add this method to WebFrame.
>    Is there any reason you cannot extend WebSurroundingText
>    interface?

I could use WebSurroundingText but I would have to initialize it with a WebFrame
because I want the text surrounding the selection. Would that work for you?

[abarth-chromium, 2014-06-09 17:29:26]

On 2014/06/09 10:39:27, Mounir Lamouri wrote:
>
https://codereview.chromium.org/294073005/diff/80001/public/web/WebFrame.h#ne...
> > public/web/WebFrame.h:484: virtual WebString textSurroundingSelection(int
> > maxLength, int* startOffset, int* endOffset) const = 0;
> > 1. We usually use non-const references for output arguments,
> >    instead of non-const pointers.
> 
> Interesting. Isn't that the opposite in Chromium?

Yes.  The Blink API uses Blink style, which isn't the same as Chromium style
(yet!).

[Yuta Kitamura, 2014-06-10 04:43:10]

On 2014/06/09 10:39:27, Mounir Lamouri wrote:
> On 2014/06/06 02:20:08, Yuta Kitamura wrote:
> > 2. I'm not sure why we need to add this method to WebFrame.
> >    Is there any reason you cannot extend WebSurroundingText
> >    interface?
> 
> I could use WebSurroundingText but I would have to initialize it with a
WebFrame
> because I want the text surrounding the selection. Would that work for you?

I think it's possible to implement it in WebSurroundingText
without adding the dependency to WebFrame, since the embedders
can obtain a WebRange object from WebFrame::selectionRange()
and pass it to the WebSurroundText initializer.

[mlamouri (slow - plz ping), 2014-06-10 13:17:15]

I have splitted this CL to have the editing/ changes (the core) separated from
the interface changes. I kept this CL for the interface (WebSurroundingText)
changes only.

See this CL for the editing/ changes: https://codereview.chromium.org/323983006

Yuta-san, can you PTAL? :)

[mlamouri (slow - plz ping), 2014-06-12 14:51:28]

Now that https://codereview.chromium.org/323983006 has landed, this should be
ready to land.

Tamura-san and Yuta-san, could you have a look?

[tkent, 2014-06-13 01:15:37]

https://codereview.chromium.org/294073005/diff/100001/Source/web/WebSurroundi...
File Source/web/WebSurroundingText.cpp (right):

https://codereview.chromium.org/294073005/diff/100001/Source/web/WebSurroundi...
Source/web/WebSurroundingText.cpp:53: const Range* range =
webRange.constUnwrap<Range>();
nit: You can simplify the code like:

if (RefPtrWillBeRawPtr<Range> range = static_cast<PassRefPtrWillBeRawPtr<Range>
>(webRange))
    m_private.reset(new SurroundingText(*range, maxLength));

https://codereview.chromium.org/294073005/diff/100001/public/web/WebRange.h
File public/web/WebRange.h (right):

https://codereview.chromium.org/294073005/diff/100001/public/web/WebRange.h#n...
public/web/WebRange.h:85: template<typename T> T* unwrap()
You don't need it.
We already have |operator PassRefPtrWillBeRawPtr<>()|.  You can get a Rage
object with:
    RefPtrWillBeRawPtr<Range> range = static_cast<PassRefPtrWillBeRawPtr<Range>
>(webRange);

WebNode and WebDOMEvent need it because storage type and wanted type are
mismatched. e.g. The storage of WebDocument is WebPrivatePtr<Node> inherited
from WebNode, but WebDocument needs Document* instead of Node*.

https://codereview.chromium.org/294073005/diff/100001/public/web/WebSurroundi...
File public/web/WebSurroundingText.h (right):

https://codereview.chromium.org/294073005/diff/100001/public/web/WebSurroundi...
public/web/WebSurroundingText.h:44: // WebSurroundingText is a Blink API that
gives access to the SurroundingText API.
Nit: I recommend to wrap code comments in 80 columns.

[Yuta Kitamura, 2014-06-13 07:12:30]

Since this patch is mostly about web/ interface, I'd like to
leave the actual review to Kent-san who is a web/ OWNER.

As far as I can see, this patch LGTM as long as Kent-san's
comments are addressed.

[mlamouri (slow - plz ping), 2014-06-13 10:34:04]

Comments applied. PTAL.

https://codereview.chromium.org/294073005/diff/100001/Source/web/WebSurroundi...
File Source/web/WebSurroundingText.cpp (right):

https://codereview.chromium.org/294073005/diff/100001/Source/web/WebSurroundi...
Source/web/WebSurroundingText.cpp:53: const Range* range =
webRange.constUnwrap<Range>();
On 2014/06/13 01:15:37, tkent wrote:
> nit: You can simplify the code like:
> 
> if (RefPtrWillBeRawPtr<Range> range =
static_cast<PassRefPtrWillBeRawPtr<Range>
> >(webRange))
>     m_private.reset(new SurroundingText(*range, maxLength));

Done.

https://codereview.chromium.org/294073005/diff/100001/public/web/WebRange.h
File public/web/WebRange.h (right):

https://codereview.chromium.org/294073005/diff/100001/public/web/WebRange.h#n...
public/web/WebRange.h:85: template<typename T> T* unwrap()
On 2014/06/13 01:15:37, tkent wrote:
> You don't need it.
> We already have |operator PassRefPtrWillBeRawPtr<>()|.  You can get a Rage
> object with:
>     RefPtrWillBeRawPtr<Range> range =
static_cast<PassRefPtrWillBeRawPtr<Range>
> >(webRange);
> 
> WebNode and WebDOMEvent need it because storage type and wanted type are
> mismatched. e.g. The storage of WebDocument is WebPrivatePtr<Node> inherited
> from WebNode, but WebDocument needs Document* instead of Node*.
> 

Done.

https://codereview.chromium.org/294073005/diff/100001/public/web/WebSurroundi...
File public/web/WebSurroundingText.h (right):

https://codereview.chromium.org/294073005/diff/100001/public/web/WebSurroundi...
public/web/WebSurroundingText.h:44: // WebSurroundingText is a Blink API that
gives access to the SurroundingText API.
On 2014/06/13 01:15:37, tkent wrote:
> Nit: I recommend to wrap code comments in 80 columns.

Done. I've also changed the rest of the comments in this file to match.

[tkent, 2014-06-15 23:42:29]

lgtm

[commit-bot: I haz the power, 2014-06-15 23:42:50]

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

[commit-bot: I haz the power, 2014-06-16 00:30:21]

Change committed as 176168