Let Selection.collapse remember a raw Position instead of a canonicalized one.

Let Selection.collapse remember a raw Position instead of a canonicalized one.

This patch fixes a long-standing editing bug that a Range object returned by
Selection.getRangeAt() may not be equivalent to the Range used to set selection
with Selection.collapse().

FrameSelection.setSelectedRange sets a Range without canonicalization:
https://src.chromium.org/viewvc/blink?revision=169100&view=revision
Hence this patch adopts it.

Following is the implementation detail.
DOMSelection.cpp:
- When selection.collapse(node, offset) is called, we create a caret Range corresponding to node and offset and pass it to FrameSelection.setSelectedRange.

FrameSelection.cpp:
- If new Selection is collapsed, use old isDirectional. This is following the current behavior.

LayoutTests:
editing/selection/no-range-canonicalization.html
- The main test added to current setRange test.

fast/css/counters/counter-before-content-not-incremented.html
- Some tests expect the collapse method to canonicalize Selection so we need to set a exact DOM position.

editing/editing.js
- Some tests expect the collapse method to canonicalize Selection at start so I canonicalize that as the old Blink collapse implementation.

editing/selection/DOMSelection-DocumentType.html
- The current HTML spec says that if Range is set DocumentType node, we should throw type exception. 

editing/selection/selection-invalid-offset.html
- We should throw exception when the method is called with a offset larger than length of a node.
- Remove a redundant case.


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

[yoichio, 2014-04-18 02:23:34]

Sorry, this CL is little fast to be reviewed.
I'll ping you later.

[yosin_UTC9, 2014-04-22 04:37:14]

https://codereview.chromium.org/225303002/diff/40001/Source/core/page/DOMSele...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/225303002/diff/40001/Source/core/page/DOMSele...
Source/core/page/DOMSelection.cpp:215: range->setEnd(node, offset,
exceptionState);
nit: |Range::setEnd()| should be succeeded, because |Range::setStart()| is
succeeded.
nit: Better to call |Range::collapse(true)|?

[Yuta Kitamura, 2014-04-22 07:10:09]

Is this patch ready?

[yoichio, 2014-04-22 07:35:01]

On 2014/04/22 07:10:09, Yuta Kitamura wrote:
> Is this patch ready?

Yes, but this brakes too many test because CL raise exception for a offset
longer than node length:
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/editing/deleting/delet...

However, this patch is ready except for those tests.

[yoichio, 2014-06-04 04:37:35]

Hi. Finally this CL is ready for ship.
Thanks in advance.

[yoichio, 2014-06-05 03:02:33]

ping?

[yosin_UTC9, 2014-06-05 03:43:36]

ACK

How about removing script elements from dump? If we remove scripts from dump, we
don't need to update some expected texts.

It seems we need to update expected texts due by script update for
Selection.exetend() method.

https://codereview.chromium.org/225303002/diff/190001/Source/core/page/DOMSel...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/225303002/diff/190001/Source/core/page/DOMSel...
Source/core/page/DOMSelection.cpp:210: if (!node)
nit: Can we merge this if-statement to L208?

https://codereview.chromium.org/225303002/diff/190001/Source/core/page/DOMSel...
Source/core/page/DOMSelection.cpp:212: RefPtr<Range> range =
Range::create(node->document());
nit: Please use RefPtrWillbeRawPtr<Range>

[Yuta Kitamura, 2014-06-05 09:09:28]

LGTM if Yoshi's comments and mine are addressed.

https://codereview.chromium.org/225303002/diff/190001/LayoutTests/editing/sel...
File LayoutTests/editing/selection/selection-invalid-offset.html (right):

https://codereview.chromium.org/225303002/diff/190001/LayoutTests/editing/sel...
LayoutTests/editing/selection/selection-invalid-offset.html:32: // these should
throw as well but don't at the moment. Just checking they don't crash.
The location of this comment is inappropriate.

https://codereview.chromium.org/225303002/diff/190001/LayoutTests/editing/sel...
LayoutTests/editing/selection/selection-invalid-offset.html:35: } catch (e)
{document.write(((e.code==DOMException.INDEX_SIZE_ERR) ? "OK" : "FAIL")+"<br>");
}
optional: I know it's not your fault, but the formatting
of this code is so ugly...

[yoichio, 2014-06-06 07:54:43]

On 2014/06/05 03:43:36, yosi wrote:
> ACK
> 
> How about removing script elements from dump? If we remove scripts from dump,
we
> don't need to update some expected texts.
> 
> It seems we need to update expected texts due by script update for
> Selection.exetend() method.
> 
Done by canonicalizing Selection at editing.js.

[yoichio, 2014-06-06 07:54:49]

https://codereview.chromium.org/225303002/diff/190001/LayoutTests/editing/sel...
File LayoutTests/editing/selection/selection-invalid-offset.html (right):

https://codereview.chromium.org/225303002/diff/190001/LayoutTests/editing/sel...
LayoutTests/editing/selection/selection-invalid-offset.html:32: // these should
throw as well but don't at the moment. Just checking they don't crash.
On 2014/06/05 09:09:28, Yuta Kitamura wrote:
> The location of this comment is inappropriate.

Done.

https://codereview.chromium.org/225303002/diff/190001/LayoutTests/editing/sel...
LayoutTests/editing/selection/selection-invalid-offset.html:35: } catch (e)
{document.write(((e.code==DOMException.INDEX_SIZE_ERR) ? "OK" : "FAIL")+"<br>");
}
On 2014/06/05 09:09:28, Yuta Kitamura wrote:
> optional: I know it's not your fault, but the formatting
> of this code is so ugly...

Done.

https://codereview.chromium.org/225303002/diff/190001/Source/core/page/DOMSel...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/225303002/diff/190001/Source/core/page/DOMSel...
Source/core/page/DOMSelection.cpp:210: if (!node)
On 2014/06/05 03:43:36, yosi wrote:
> nit: Can we merge this if-statement to L208?
Oh, |node| is asserted at L199 so this line is no longer needed.

https://codereview.chromium.org/225303002/diff/190001/Source/core/page/DOMSel...
Source/core/page/DOMSelection.cpp:212: RefPtr<Range> range =
Range::create(node->document());
On 2014/06/05 03:43:36, yosi wrote:
> nit: Please use RefPtrWillbeRawPtr<Range>

Done.

[yoichio, 2014-06-06 08:54:33]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2014-06-06 08:55:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/225303002/230001

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

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

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[yoichio, 2014-06-07 01:30:05]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2014-06-07 01:31:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/225303002/230001

[commit-bot: I haz the power, 2014-06-07 01:48:18]

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

[commit-bot: I haz the power, 2014-06-07 01:55:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-07 01:55:22]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10719)

[yoichio, 2014-06-08 11:38:48]

The CQ bit was checked by yoichio@chromium.org

[commit-bot: I haz the power, 2014-06-08 11:39:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/225303002/230001

[commit-bot: I haz the power, 2014-06-08 11:57:58]

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

[commit-bot: I haz the power, 2014-06-08 12:04:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-08 12:04:28]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[yoichio, 2014-06-09 06:10:57]

The CQ bit was checked by yoichio@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoichio@chromium.org/225303002/250001

[commit-bot: I haz the power, 2014-06-09 07:16:31]

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

[yoichio, 2014-06-09 07:18:39]

+tkent@ as an OWNER for Source/web/WebLocalFrameImpl.cpp.
Could you review?

[commit-bot: I haz the power, 2014-06-09 07:19:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-09 07:19:22]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7271)

[tkent, 2014-06-09 07:26:22]

lgtm

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.h:103: bool setSelectedRange(Range*,
EAffinity, bool isDirectional = false, SetSelectionOptions = CloseTyping |
ClearTypingStyle);
nit: We should not introduce new bool arguments.

[commit-bot: I haz the power, 2014-06-09 07:30:33]

Change committed as 175779

[yoichio, 2014-06-09 07:38:31]

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.h:103: bool setSelectedRange(Range*,
EAffinity, bool isDirectional = false, SetSelectionOptions = CloseTyping |
ClearTypingStyle);
On 2014/06/09 07:26:22, tkent wrote:
> nit: We should not introduce new bool arguments.

What do you mean?

[tkent, 2014-06-09 07:44:17]

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.h:103: bool setSelectedRange(Range*,
EAffinity, bool isDirectional = false, SetSelectionOptions = CloseTyping |
ClearTypingStyle);
On 2014/06/09 07:38:31, yoichio wrote:
> On 2014/06/09 07:26:22, tkent wrote:
> > nit: We should not introduce new bool arguments.
> 
> What do you mean?

Please refer to the 10th bullet of
http://dev.chromium.org/blink/coding-style#TOC-Names

[yoichio, 2014-06-09 07:46:22]

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
File Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/225303002/diff/250001/Source/core/editing/Fra...
Source/core/editing/FrameSelection.h:103: bool setSelectedRange(Range*,
EAffinity, bool isDirectional = false, SetSelectionOptions = CloseTyping |
ClearTypingStyle);
On 2014/06/09 07:44:17, tkent wrote:
> On 2014/06/09 07:38:31, yoichio wrote:
> > On 2014/06/09 07:26:22, tkent wrote:
> > > nit: We should not introduce new bool arguments.
> > 
> > What do you mean?
> 
> Please refer to the 10th bullet of
> http://dev.chromium.org/blink/coding-style#TOC-Names
I see. Will fix later:) Thanks.