Plumb selection bounds as a single unit

Plumb selection bounds as a single unit

Wire the composited selection bounds as a single structure, including
information about editability. This lays the groundwork for plumbing
metadata about the selection triggering cause, as well as any other
required metadata that can be used to inform selection response in a
non-racy way relative to the selection bound location.

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

[mfomitchev, 2015-02-17 22:25:51]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2015-02-17 22:25:51]

https://codereview.chromium.org/929213004/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/929213004/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:117: static WebSelectionBounds
toWebSelectionBounds(const CompositedSelectionBounds& bound)
This doesn't need to be changed, right?

https://codereview.chromium.org/929213004/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:748:
m_webView->updateCompositedSelectionBounds(toWebSelectionBounds(anchor));
anchor -> bounds

[jdduke (slow), 2015-02-17 22:28:50]

https://codereview.chromium.org/929213004/diff/1/Source/web/ChromeClientImpl.cpp
File Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/929213004/diff/1/Source/web/ChromeClientImpl....
Source/web/ChromeClientImpl.cpp:117: static WebSelectionBounds
toWebSelectionBounds(const CompositedSelectionBounds& bound)
On 2015/02/17 22:25:51, mfomitchev wrote:
> This doesn't need to be changed, right?

Oops, yeah this was a typo, I never actually compiled it =/

[jdduke (slow), 2015-03-12 18:18:37]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org, rbyers@chromium.org

[jdduke (slow), 2015-03-12 18:18:38]

+aelias for Source/web
+rbyers for Source/core and public/platform

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1672: bounds.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
If there's a better way of determining this, please share :)

[Rick Byers, 2015-03-12 19:25:52]

rbyers@chromium.org changed reviewers:
+ yosin@chromium.org

[Rick Byers, 2015-03-12 19:27:24]

The mechanics of grouping this into CompositedSelectionBounds all look fine to
me.  But we should add an editing expert (+yosin@) to review the few lines about
editability in FrameView.cpp

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1669: bounds.isEditable =
selection.isContentEditable();
does isContentEditable really return true for the common editable cases like
<input>?  I expected (from the name and quick glance at the code) that this
would be true only when the element or an ancestor has the contentEditable=true
attribute.

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1672: bounds.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
On 2015/03/12 18:18:38, jdduke wrote:
> If there's a better way of determining this, please share :)

not sure off hand.  yosin@?

Does this handle the 'empty' contentEditable=true case properly?

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
File Source/core/layout/compositing/CompositedSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
Source/core/layout/compositing/CompositedSelectionBounds.h:40: struct
CompositedSelectionBounds {
can you please add a short comment describing the purpose of this class?  Maybe
say to see WebSelectionBounds for details?

[aelias_OOO_until_Jul13, 2015-03-12 19:29:16]

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
File public/platform/WebSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
public/platform/WebSelectionBounds.h:13: struct WebSelectionBounds {
Just WebSelection might be a better name?

[Rick Byers, 2015-03-12 19:29:22]

https://codereview.chromium.org/929213004/diff/40001/Source/web/tests/data/co...
File Source/web/tests/data/composited_selection_bounds_editable.html (right):

https://codereview.chromium.org/929213004/diff/40001/Source/web/tests/data/co...
Source/web/tests/data/composited_selection_bounds_editable.html:10: <textarea
id="text">
Are <textarea> and <input type=text> similar enough that it's OK to test only
one?

I think we probably need a test case for contentEditable (see
https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_Editable).

[yosin_UTC9, 2015-03-13 02:06:37]

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1672: bounds.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
On 2015/03/12 19:27:23, Rick Byers wrote:
> On 2015/03/12 18:18:38, jdduke wrote:
> > If there's a better way of determining this, please share :)
> 
> not sure off hand.  yosin@?
> 
> Does this handle the 'empty' contentEditable=true case properly?

Short answer is "yes". Long answer is "case by case" since word "properly" is
ambiguous.
Could you tell me your usage of result of |isContentEditable()|?
Current implementation returns true if container node of start position has
user-modify: read-write regardless editability of between start(exclusive) to
end. In this case, replacing selection content operation replaces text at start
position and rest of text are left.

P.S. It is editing team due to handl text form control editablity by
|isContentEditable()|. It is not good idea to check text form control here.

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
File Source/core/layout/compositing/CompositedSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
Source/core/layout/compositing/CompositedSelectionBounds.h:51: bool
isEditableRegionEmpty;
Q: Could you tell me what "Region" is?

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
File public/platform/WebSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
public/platform/WebSelectionBounds.h:33: // If the selection region is editable,
whether it is empty.
Q: Do you mean "region" as "range"?
It is confusing that |WebSelectionBound| have type |Caret| and
|WebSelectionBounds| doesn't have concept about collapsed selection.

[jdduke (slow), 2015-03-16 18:15:41]

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1669: bounds.isEditable =
selection.isContentEditable();
On 2015/03/12 19:27:24, Rick Byers wrote:
> does isContentEditable really return true for the common editable cases like
> <input>?  I expected (from the name and quick glance at the code) that this
> would be true only when the element or an ancestor has the
contentEditable=true
> attribute.

Hmm, it appears to, yes. One odd thing that I noticed is that we don't display
the OSK when we enter a "contentEditable" div, which seems like a bug?

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1672: bounds.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
On 2015/03/13 02:06:37, Yosi_UTC9 wrote:
> On 2015/03/12 19:27:23, Rick Byers wrote:
> > On 2015/03/12 18:18:38, jdduke wrote:
> > > If there's a better way of determining this, please share :)
> > 
> > not sure off hand.  yosin@?
> > 
> > Does this handle the 'empty' contentEditable=true case properly?
> 
> Short answer is "yes". Long answer is "case by case" since word "properly" is
> ambiguous.
> Could you tell me your usage of result of |isContentEditable()|?

Sure. We need to know whether to display a text insertion handle if the
selection area is editable. In short, this should only be true for areas where
we can insert text, so it's possible there's a different bit we need here?

> Current implementation returns true if container node of start position has
> user-modify: read-write regardless editability of between start(exclusive) to
> end. In this case, replacing selection content operation replaces text at
start
> position and rest of text are left.
> 
> P.S. It is editing team due to handl text form control editablity by
> |isContentEditable()|. It is not good idea to check text form control here.
> 
> 

So, what property should I check?

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
File Source/core/layout/compositing/CompositedSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
Source/core/layout/compositing/CompositedSelectionBounds.h:40: struct
CompositedSelectionBounds {
On 2015/03/12 19:27:24, Rick Byers wrote:
> can you please add a short comment describing the purpose of this class? 
Maybe
> say to see WebSelectionBounds for details?

Done.

https://codereview.chromium.org/929213004/diff/40001/Source/core/layout/compo...
Source/core/layout/compositing/CompositedSelectionBounds.h:51: bool
isEditableRegionEmpty;
On 2015/03/13 02:06:37, Yosi_UTC9 wrote:
> Q: Could you tell me what "Region" is?

Sure.

In short, we want to know whether we can 1) enter text into the region (edit
it), and 2) if we can enter text, whether the area we will enter text into is
empty. We use this metadata to conditionally show the insertion handles in the
browser, aligning with native platform defaults. If you can think of a better
name, by all means do so!

https://codereview.chromium.org/929213004/diff/40001/Source/web/tests/data/co...
File Source/web/tests/data/composited_selection_bounds_editable.html (right):

https://codereview.chromium.org/929213004/diff/40001/Source/web/tests/data/co...
Source/web/tests/data/composited_selection_bounds_editable.html:10: <textarea
id="text">
On 2015/03/12 19:29:22, Rick Byers wrote:
> Are <textarea> and <input type=text> similar enough that it's OK to test only
> one?
> 
> I think we probably need a test case for contentEditable (see
> https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_Editable).

Good call, I'll add one.

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
File public/platform/WebSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
public/platform/WebSelectionBounds.h:13: struct WebSelectionBounds {
On 2015/03/12 19:29:16, aelias wrote:
> Just WebSelection might be a better name?

Sure. I hesitated to name it that to prevent confusion about it being a
reflection of the selection region, rather than selection itself, but I can
change it.

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
public/platform/WebSelectionBounds.h:33: // If the selection region is editable,
whether it is empty.
On 2015/03/13 02:06:37, Yosi_UTC9 wrote:
> Q: Do you mean "region" as "range"?
> It is confusing that |WebSelectionBound| have type |Caret| and
> |WebSelectionBounds| doesn't have concept about collapsed selection.

Right, this isn't ideal. We should have sent the bounds as a unit before (as I'm
doing here) so that we could use a higher-level indicator for the selection
type, i.e., caret vs range, as with VisibleSelection. I'd look to move closer
toward that model, for now it's implicit that we have a caret if the bounds are
identical and they each have the type "Caret".

As a follow-up, I plan on removing the type metadata from the start/end region
up into this data structure, again, patterning it more closely to existing Blink
selection data structures.

[jdduke (slow), 2015-03-16 18:19:21]

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
File public/platform/WebSelectionBounds.h (right):

https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
public/platform/WebSelectionBounds.h:13: struct WebSelectionBounds {
On 2015/03/16 18:15:41, jdduke wrote:
> On 2015/03/12 19:29:16, aelias wrote:
> > Just WebSelection might be a better name?
> 
> Sure. I hesitated to name it that to prevent confusion about it being a
> reflection of the selection region, rather than selection itself, but I can
> change it.

Maybe WebCompositedSelection?

[aelias_OOO_until_Jul13, 2015-03-16 18:42:03]

On 2015/03/16 at 18:19:21, jdduke wrote:
>
https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
> File public/platform/WebSelectionBounds.h (right):
> 
>
https://codereview.chromium.org/929213004/diff/40001/public/platform/WebSelec...
> public/platform/WebSelectionBounds.h:13: struct WebSelectionBounds {
> On 2015/03/16 18:15:41, jdduke wrote:
> > On 2015/03/12 19:29:16, aelias wrote:
> > > Just WebSelection might be a better name?
> > 
> > Sure. I hesitated to name it that to prevent confusion about it being a
> > reflection of the selection region, rather than selection itself, but I can
> > change it.
> 
> Maybe WebCompositedSelection?

I guess I have no strong preference between that and WebSelectionBounds, do as
you prefer.  Source/web lgtm regardless.

[yosin_UTC9, 2015-03-18 05:15:41]

Sorry for late response. m(_ _)m

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1672: bounds.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
On 2015/03/16 18:15:41, jdduke wrote:
> On 2015/03/13 02:06:37, Yosi_UTC9 wrote:
> > On 2015/03/12 19:27:23, Rick Byers wrote:
> > > On 2015/03/12 18:18:38, jdduke wrote:
> > > > If there's a better way of determining this, please share :)
> > > 
> > > not sure off hand.  yosin@?
> > > 
> > > Does this handle the 'empty' contentEditable=true case properly?
> > 
> > Short answer is "yes". Long answer is "case by case" since word "properly"
is
> > ambiguous.
> > Could you tell me your usage of result of |isContentEditable()|?
> 
> Sure. We need to know whether to display a text insertion handle if the
> selection area is editable. In short, this should only be true for areas where
> we can insert text, so it's possible there's a different bit we need here?
> 
> > Current implementation returns true if container node of start position has
> > user-modify: read-write regardless editability of between start(exclusive)
to
> > end. In this case, replacing selection content operation replaces text at
> start
> > position and rest of text are left.
> > 
> > P.S. It is editing team due to handl text form control editablity by
> > |isContentEditable()|. It is not good idea to check text form control here.
> > 
> > 
> 
> So, what property should I check?
> 

How about calling |isContentEdtiable()| if |selection.isCaret()|? Because of
determining editability of selection range is unclear, e.g. range contains
uneditable node.

I think you don't need to check form controls here, it should be handled by
|isContentEditable()| function, if not, please file a bug, we'll fix it.

[yosin_UTC9, 2015-03-18 05:16:57]

yosin@chromium.org changed reviewers:
+ yoichio@chromium.org

[yosin_UTC9, 2015-03-18 05:16:58]

+yoichio@ he is an expert about selection and API between Blink and content.

[jdduke (slow), 2015-03-18 20:20:45]

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1672: bounds.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
On 2015/03/18 05:15:41, Yosi_UTC9 wrote:
> On 2015/03/16 18:15:41, jdduke wrote:
> > On 2015/03/13 02:06:37, Yosi_UTC9 wrote:
> > > On 2015/03/12 19:27:23, Rick Byers wrote:
> > > > On 2015/03/12 18:18:38, jdduke wrote:
> > > > > If there's a better way of determining this, please share :)
> > > > 
> > > > not sure off hand.  yosin@?
> > > > 
> > > > Does this handle the 'empty' contentEditable=true case properly?
> > > 
> > > Short answer is "yes". Long answer is "case by case" since word "properly"
> is
> > > ambiguous.
> > > Could you tell me your usage of result of |isContentEditable()|?
> > 
> > Sure. We need to know whether to display a text insertion handle if the
> > selection area is editable. In short, this should only be true for areas
where
> > we can insert text, so it's possible there's a different bit we need here?
> > 
> > > Current implementation returns true if container node of start position
has
> > > user-modify: read-write regardless editability of between start(exclusive)
> to
> > > end. In this case, replacing selection content operation replaces text at
> > start
> > > position and rest of text are left.
> > > 
> > > P.S. It is editing team due to handl text form control editablity by
> > > |isContentEditable()|. It is not good idea to check text form control
here.
> > > 
> > > 
> > 
> > So, what property should I check?
> > 
> 
> How about calling |isContentEdtiable()| if |selection.isCaret()|? Because of
> determining editability of selection range is unclear, e.g. range contains
> uneditable node.
> 

That's one option. I think what we want is for this to align with the IME
concept of editability, i.e., if the textInputType is not "none".

> I think you don't need to check form controls here, it should be handled by
> |isContentEditable()| function, if not, please file a bug, we'll fix it.
> 

The reason we want to know if the text region is empty is the following:

If the user taps an empty field, we don't want to display the insertion handle.
Otherwise, we do want to display the insertion handle. It's not a big deal, but
it's necessary to align with native Android widget behavior.

[jdduke (slow), 2015-03-19 00:32:42]

I went ahead and made WebSelection a proper class that exposes data/types in a
fashion similar to VisibleSelection. Once a few downstream dependencies have
been removed we can further clean up some of the typing here.

So now we just need to agree on text editability semantics.

[yoichio, 2015-03-20 05:17:51]

https://codereview.chromium.org/929213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1680: selection.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
How about <div isContentEditable=true></div>? That is not
HTMLTextFormControlElement.

[yoichio, 2015-03-20 05:27:56]

Could you tell me what is "the insertion handle" in Android you want to follow?
It might be some Android SDK help pages, I guess.

[jdduke (slow), 2015-03-20 15:02:18]

On 2015/03/20 05:27:56, yoichio wrote:
> Could you tell me what is "the insertion handle" in Android you want to
follow?
> It might be some Android SDK help pages, I guess.

Here's an image of the insertion handle:
https://drive.google.com/open?id=0B_JJh4qRM6VaWEM3OXItR0t4MUk4RWJJUUxJcUlXOUF...

In short, for selection editability we're ONLY concerned if it is editable text,
i.e., if it would trigger the OSK or otherwise allow keyboard entry. The
insertion handle is used to move the cursor position via touch input.

https://codereview.chromium.org/929213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/929213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1680: selection.isEditableRegionEmpty =
enclosingTextFormControlElement->value().isEmpty();
On 2015/03/20 05:17:51, yoichio wrote:
> How about <div isContentEditable=true></div>? That is not
> HTMLTextFormControlElement.

You're right.

I think it's OK if we make this "best effort", but perhaps we should rename the
field to be something like "isInEmptyTextControl"? That should handle the most
common case.

Note that we actually don't care at all about the Caret if the region is not
editable, i.e., if we'll never display the OSK for that caret location, then we
really don't care about the selection at all, and I'd almost prefer we report 
"NoSelection", but if we do that it should probably be a separate patch.

[jdduke (slow), 2015-03-20 15:27:45]

On 2015/03/20 15:02:18, jdduke wrote:
> On 2015/03/20 05:27:56, yoichio wrote:
> > Could you tell me what is "the insertion handle" in Android you want to
> follow?
> > It might be some Android SDK help pages, I guess.
> 
> Here's an image of the insertion handle:
>
https://drive.google.com/open?id=0B_JJh4qRM6VaWEM3OXItR0t4MUk4RWJJUUxJcUlXOUF...

A more friendly link:
https://drive.google.com/a/chromium.org/file/d/0ByhHNaAkRQvCUExVME9KcWUxRUE/v...

[jdduke (slow), 2015-03-20 20:00:15]

https://codereview.chromium.org/929213004/diff/100001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_editable.html (right):

https://codereview.chromium.org/929213004/diff/100001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_editable.html:21:
window.expectedResult = [document, 1, 1, 1, 15, document, 1, 1, 1, 15,
expectEditable, expectEmptyTextFormControl];
Any guidance on how to ensure consistent sizing across platforms? I'm getting
some off-by-one coordinates between linux and mac/win.

[jdduke (slow), 2015-03-25 17:10:03]

On 2015/03/20 20:00:15, jdduke wrote:
>
https://codereview.chromium.org/929213004/diff/100001/Source/web/tests/data/c...
> File Source/web/tests/data/composited_selection_bounds_editable.html (right):
> 
>
https://codereview.chromium.org/929213004/diff/100001/Source/web/tests/data/c...
> Source/web/tests/data/composited_selection_bounds_editable.html:21:
> window.expectedResult = [document, 1, 1, 1, 15, document, 1, 1, 1, 15,
> expectEditable, expectEmptyTextFormControl];
> Any guidance on how to ensure consistent sizing across platforms? I'm getting
> some off-by-one coordinates between linux and mac/win.

Friendly ping? yoichio?

[yoichio, 2015-03-26 03:39:15]

isEmptyTextFormControl sounds good.

https://codereview.chromium.org/929213004/diff/100001/Source/web/tests/data/c...
File Source/web/tests/data/composited_selection_bounds_editable.html (right):

https://codereview.chromium.org/929213004/diff/100001/Source/web/tests/data/c...
Source/web/tests/data/composited_selection_bounds_editable.html:21:
window.expectedResult = [document, 1, 1, 1, 15, document, 1, 1, 1, 15,
expectEditable, expectEmptyTextFormControl];
On 2015/03/20 20:00:15, jdduke wrote:
> Any guidance on how to ensure consistent sizing across platforms? I'm getting
> some off-by-one coordinates between linux and mac/win.

It can render in different layout on each platform.
This patch is a kind of extension so you could find similar tests for
WebSelectionBound.

[jdduke (slow), 2015-03-27 20:18:21]

yoichio@: Could you take another look? I went back to using start/end for
plumbing, as it appears they can differ from base/extent because of selection
granularity, which is not (immediately) useful to the browser consumer of the
selection data.

[yoichio, 2015-03-30 05:07:39]

webkit_unit_tests looks failed.

https://codereview.chromium.org/929213004/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/929213004/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:2034: // FIXME: Remove this overload when downstream
consumers have been updated, crbug.com/466672.
Use "TODO(jdduke)" as this comment style was changed recently:)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

[yoichio, 2015-03-30 05:08:52]

Please ping us if your change gets settled.

[jdduke (slow), 2015-03-30 15:24:44]

On 2015/03/30 05:07:39, yoichio wrote:
> webkit_unit_tests looks failed.

Really? They all passed with patchset #10, which was tested Friday.

https://codereview.chromium.org/929213004/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/929213004/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:2034: // FIXME: Remove this overload when downstream
consumers have been updated, crbug.com/466672.
On 2015/03/30 05:07:38, yoichio wrote:
> Use "TODO(jdduke)" as this comment style was changed recently:)
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

Yay! Done.

[yoichio, 2015-03-31 05:05:06]

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4285: const int yEpsilon = 2;
Existing tests don't need this margin. What's the difference?

[jdduke (slow), 2015-03-31 15:09:24]

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4285: const int yEpsilon = 2;
On 2015/03/31 05:05:06, yoichio wrote:
> Existing tests don't need this margin. What's the difference?

That's what I asked last week, and nobody had a good answer... the
selectionBounds() tests don't actually check the composited position, they just
check logical positions. Only the editable nodes here seem to introduce variance
in the y-coordinate, and this is the first time we're actually testing <input>
elements and the like for composited positions.

[yoichio, 2015-04-02 05:20:44]

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4285: const int yEpsilon = 2;
On 2015/03/31 15:09:23, jdduke wrote:
> On 2015/03/31 05:05:06, yoichio wrote:
> > Existing tests don't need this margin. What's the difference?
> 
> That's what I asked last week, and nobody had a good answer... the
> selectionBounds() tests don't actually check the composited position, they
just
> check logical positions. Only the editable nodes here seem to introduce
variance
> in the y-coordinate, and this is the first time we're actually testing <input>
> elements and the like for composited positions.
The variance is only on editable element, could you introduce it in test like
window.expectedResult = [document, 1, 1, 1, 14, document, 1, 1, 1, 14,
expectEditable, expectEmptyTextFormControl, yEpsilon] ?

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4291:
EXPECT_NEAR(expectedResult.Get(2)->Int32Value(), selectStart->edgeTopInLayer.y,
yEpsilon);
Could you confirm this variance is same for each y position? :
int variance = selectStart->edgeTopInLayer.y -
expectedResult.Get(2)->Int32Value();
EXPECT_EQ(expectedResult.Get(4)->Int32Value() + variance,
selectStart->edgeBottomInLayer.y);
...

[jdduke (slow), 2015-04-06 22:41:07]

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4285: const int yEpsilon = 2;
On 2015/04/02 05:20:43, yoichio wrote:
> On 2015/03/31 15:09:23, jdduke wrote:
> > On 2015/03/31 05:05:06, yoichio wrote:
> > > Existing tests don't need this margin. What's the difference?
> > 
> > That's what I asked last week, and nobody had a good answer... the
> > selectionBounds() tests don't actually check the composited position, they
> just
> > check logical positions. Only the editable nodes here seem to introduce
> variance
> > in the y-coordinate, and this is the first time we're actually testing
<input>
> > elements and the like for composited positions.
> The variance is only on editable element, could you introduce it in test like
> window.expectedResult = [document, 1, 1, 1, 14, document, 1, 1, 1, 14,
> expectEditable, expectEmptyTextFormControl, yEpsilon] ?
> 

Done.

https://codereview.chromium.org/929213004/diff/200001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:4291:
EXPECT_NEAR(expectedResult.Get(2)->Int32Value(), selectStart->edgeTopInLayer.y,
yEpsilon);
On 2015/04/02 05:20:43, yoichio wrote:
> Could you confirm this variance is same for each y position? :
> int variance = selectStart->edgeTopInLayer.y -
> expectedResult.Get(2)->Int32Value();
> EXPECT_EQ(expectedResult.Get(4)->Int32Value() + variance,
> selectStart->edgeBottomInLayer.y);
> ...
> 

Done.

[yoichio, 2015-04-07 01:59:39]

lgtm

[Rick Byers, 2015-04-07 21:15:45]

https://codereview.chromium.org/929213004/diff/220001/public/web/WebSelection.h
File public/web/WebSelection.h (right):

https://codereview.chromium.org/929213004/diff/220001/public/web/WebSelection...
public/web/WebSelection.h:9: #include "../platform/WebSelectionBound.h"
Any reason why this file is in public/web instead of public/platform along with
WebSelectionBound?  I expected they'd be next to each other and it looks like
you don't need the different DEPS rules of public/web here (but I must admit I
don't fully understand the distinction between public/web and public/platform).

[jdduke (slow), 2015-04-07 21:31:15]

https://codereview.chromium.org/929213004/diff/220001/public/web/WebSelection.h
File public/web/WebSelection.h (right):

https://codereview.chromium.org/929213004/diff/220001/public/web/WebSelection...
public/web/WebSelection.h:9: #include "../platform/WebSelectionBound.h"
On 2015/04/07 21:15:45, Rick Byers wrote:
> Any reason why this file is in public/web instead of public/platform along
with
> WebSelectionBound?  I expected they'd be next to each other and it looks like
> you don't need the different DEPS rules of public/web here (but I must admit I
> don't fully understand the distinction between public/web and
public/platform).

I think I have some logic in the WebSelection.cpp file that made it impossible
to put this in platform. However, I can move that logic just about anywhere, so
I'll go ahead and do that if you prefer (also moving this in to platform).

[Rick Byers, 2015-04-08 16:11:59]

On 2015/04/07 21:31:15, jdduke wrote:
>
https://codereview.chromium.org/929213004/diff/220001/public/web/WebSelection.h
> File public/web/WebSelection.h (right):
> 
>
https://codereview.chromium.org/929213004/diff/220001/public/web/WebSelection...
> public/web/WebSelection.h:9: #include "../platform/WebSelectionBound.h"
> On 2015/04/07 21:15:45, Rick Byers wrote:
> > Any reason why this file is in public/web instead of public/platform along
> with
> > WebSelectionBound?  I expected they'd be next to each other and it looks
like
> > you don't need the different DEPS rules of public/web here (but I must admit
I
> > don't fully understand the distinction between public/web and
> public/platform).
> 
> I think I have some logic in the WebSelection.cpp file that made it impossible
> to put this in platform. However, I can move that logic just about anywhere,
so
> I'll go ahead and do that if you prefer (also moving this in to platform).

I think it would be least surprising if WebSelection and WebSelectionBounds were
in the same directory (unless there's a good reason otherwise).  But either way
(move both to platform or web) is fine with me - whatever feels cleanest to you.

[jdduke (slow), 2015-04-09 20:02:34]

On 2015/04/08 16:11:59, Rick Byers wrote:
> I think it would be least surprising if WebSelection and WebSelectionBounds
were
> in the same directory (unless there's a good reason otherwise).  But either
way
> (move both to platform or web) is fine with me - whatever feels cleanest to
you.

Agreed. I was going to move WebSelectionBound to web/, but downstream code
currently depends on it being in platform/. That direct dependency will be
removed soon, at which point I'll move it to web/. I'll add a TODO.

[Rick Byers, 2015-04-09 21:29:58]

On 2015/04/09 20:02:34, jdduke wrote:
> On 2015/04/08 16:11:59, Rick Byers wrote:
> > I think it would be least surprising if WebSelection and WebSelectionBounds
> were
> > in the same directory (unless there's a good reason otherwise).  But either
> way
> > (move both to platform or web) is fine with me - whatever feels cleanest to
> you.
> 
> Agreed. I was going to move WebSelectionBound to web/, but downstream code
> currently depends on it being in platform/. That direct dependency will be
> removed soon, at which point I'll move it to web/. I'll add a TODO.

Thanks for digging into this, sorry for the distraction!  LGTM

[jdduke (slow), 2015-04-09 22:25:18]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-04-09 22:25:18]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
yoichio@chromium.org
Link to the patchset: https://codereview.chromium.org/929213004/#ps240001
(title: "TODO for moving WebSelectionBound")

[commit-bot: I haz the power, 2015-04-09 22:25:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/929213004/240001

[commit-bot: I haz the power, 2015-04-10 00:21:43]

Committed patchset #13 (id:240001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193494