Ignore visibility when checking whether Position is editable or not at DeletetSelectionCommand

Ignore visibility when checking whether Position is editable or not at DeletetSelectionCommand 

This patch introduces |lastEditablePositionBeforePositionInRoot()| to get
to editable |Position| instead of |VisiblePosition| for making
DeletetSelectionCommand to work editable position regardless its visibility.

The root cause of issue 339187 is |DeleteSelectionCommand| shrinks end point of 
selection range, start position is start of "bar", which is hidden editable, 
and end position is "foo", which isn't editable, to editable position but 
|DeleteSelectionCommand| checks both editability and visibility, then it gets 
null position since end point is hidden. 


BUG=339187
TEST=LayoutTests/editing/execCommand/delete-hidden-crash.html

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

[yosin_UTC9, 2014-06-18 07:48:06]

Could you review this patch?
Thanks in advance.

[Yuta Kitamura, 2014-06-18 08:08:11]

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/delete-hidden-crash.html (right):

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:8: onload = function(){
Space required before the brace.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:9:
document.execCommand('SelectAll')
Bad indentation.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:25: <tbody>
Indentation is not necessary here.

If you need the indentation here, please use
4-space indent.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:38: </tbody>
This test case is very hard to understand...

It would be awesome if you can come up with a simpler
test case.

At least the second <tr> does not seem relevant to
this test.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:39: </body>
No close tag of <table>?

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
Source/core/editing/htmlediting.cpp:308: return p;
Let's stop using abbreviated names.

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
File Source/core/editing/htmlediting.h (right):

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
Source/core/editing/htmlediting.h:177: Position
lastEditablePositionBeforePositionInRoot(const Position&, Node*);
It's extremely weird to change this function only.

Why don't you write another CL that changes the return
value types of these functions first?

[yosin_UTC9, 2014-06-19 07:36:57]

PTAL

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
File LayoutTests/editing/execCommand/delete-hidden-crash.html (right):

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:8: onload = function(){
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> Space required before the brace.

Done.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:9:
document.execCommand('SelectAll')
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> Bad indentation.

Done.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:25: <tbody>
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> Indentation is not necessary here.
> 
> If you need the indentation here, please use
> 4-space indent.

Done.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:38: </tbody>
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> This test case is very hard to understand...
> 
> It would be awesome if you can come up with a simpler
> test case.
> 
> At least the second <tr> does not seem relevant to
> this test.

Done.

https://codereview.chromium.org/340713003/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/delete-hidden-crash.html:39: </body>
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> No close tag of <table>?

Done.

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
File Source/core/editing/htmlediting.cpp (right):

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
Source/core/editing/htmlediting.cpp:308: return p;
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> Let's stop using abbreviated names.

Done by http://crrev.com/342553008/

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
File Source/core/editing/htmlediting.h (right):

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
Source/core/editing/htmlediting.h:177: Position
lastEditablePositionBeforePositionInRoot(const Position&, Node*);
On 2014/06/18 08:08:11, Yuta Kitamura wrote:
> It's extremely weird to change this function only.
> 
> Why don't you write another CL that changes the return
> value types of these functions first?

Make sense. I made another patch http://crrev.com/340713003 for renaming these
functions.

[Yuta Kitamura, 2014-06-19 07:43:36]

LGTM

This is much better, thanks!

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
File Source/core/editing/htmlediting.h (right):

https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
Source/core/editing/htmlediting.h:177: Position
lastEditablePositionBeforePositionInRoot(const Position&, Node*);
On 2014/06/19 07:36:57, yosi wrote:
> Make sense. I made another patch http://crrev.com/340713003 for renaming these
> functions.

Well, self recursion detected in your link ;)

[yosin_UTC9, 2014-06-19 08:05:47]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2014-06-19 08:06:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yosin@chromium.org/340713003/40001

[yosin_UTC9, 2014-06-19 08:07:26]

On 2014/06/19 07:43:36, Yuta Kitamura wrote:
> LGTM
> 
> This is much better, thanks!
> 
>
https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
> File Source/core/editing/htmlediting.h (right):
> 
>
https://codereview.chromium.org/340713003/diff/1/Source/core/editing/htmledit...
> Source/core/editing/htmlediting.h:177: Position
> lastEditablePositionBeforePositionInRoot(const Position&, Node*);
> On 2014/06/19 07:36:57, yosi wrote:
> > Make sense. I made another patch http://crrev.com/340713003 for renaming
these
> > functions.
> 
> Well, self recursion detected in your link ;)

Thanks for reviewing!

Here is *actual* patches for renaming and cleanup:
- http://crrev.com/342553008/ Avoid to use abbreviated names in
{first,last}EditableVisiblePosition{After,Before}PositionInRoot
- http://crrev.com/340013002
Rename {first,last}EditablePosition{After,Before}Position... to denote them
returned VisiblePosition

[commit-bot: I haz the power, 2014-06-19 08:43:03]

Change committed as 176497