Use anchor node, rather than current selected node, to check editability.

Use anchor node, rather than current selected node, to check editability.

BUG=645665
Committed: https://crrev.com/c0d0bd4e6f7e38c0208b78b2df9750ba7b0ceb0d
Cr-Commit-Position: refs/heads/master@{#424790}

[wkorman, 2016-10-10 19:32:38]

wkorman@chromium.org changed reviewers:
+ wangxianzhu@chromium.org, yoichio@chromium.org

[wkorman, 2016-10-10 19:32:39]

The added test crashes at ToT with an under-invalidation, and passes with patch.
I believe this crash was due to localCaretRectOfPositionTemplate() building up
an invalidation rect via the anchor node, whereas FrameCaret logic was
invalidating via the selected node, which could be a child of the anchor node,
and so a different layout object/rect (and potentially non-editable, which was
the core issue for this bug).

The more long term fix is to address yoichio@ comment on
CaretBase::invalidateLocalCaretRect:

// TODO(yoichio): |node| is FrameSelection::m_previousCaretNode and this is bad
// design. We should use only previous layoutObject or Rectangle to invalidate
// old caret.

but that is a larger rework and I think it's worth fixing this issue first. I
also noted we could potentially store Position rather than Node, but again,
that's a larger change.

[wkorman, 2016-10-10 23:11:49]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 23:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 00:14:45]

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

[commit-bot: I haz the power, 2016-10-11 00:14:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoichio, 2016-10-11 04:26:52]

Many editing layout tests failure on bot.

[wkorman, 2016-10-11 05:37:29]

On 2016/10/11 04:26:52, yoichio wrote:
> Many editing layout tests failure on bot.

Sorry, in fixing a few failing tests I broke many. I'll update when the change
is ready to be looked at again.

[wkorman, 2016-10-11 05:50:20]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 05:50:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wkorman, 2016-10-11 07:09:52]

On 2016/10/11 05:37:29, wkorman wrote:
> On 2016/10/11 04:26:52, yoichio wrote:
> > Many editing layout tests failure on bot.
> 
> Sorry, in fixing a few failing tests I broke many. I'll update when the change
> is ready to be looked at again.

Passing tests now, PTAL.

[commit-bot: I haz the power, 2016-10-11 07:42:56]

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

[commit-bot: I haz the power, 2016-10-11 07:42:57]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-10-11 15:51:25]

https://codereview.chromium.org/2401363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right):

https://codereview.chromium.org/2401363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:254:
m_previousCaretAnchorNode = nullptr;
Do you also need to do this when node==m_previousCaretAnchorNode?

[wkorman, 2016-10-11 18:13:00]

https://codereview.chromium.org/2401363003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right):

https://codereview.chromium.org/2401363003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:254:
m_previousCaretAnchorNode = nullptr;
On 2016/10/11 15:51:24, Xianzhu wrote:
> Do you also need to do this when node==m_previousCaretAnchorNode?

Ah, yes, good catch, thanks. Also changed invalidateLocalCaretRect() call above,
and in ::dataWillChange() above that, to pass m_previousCaretAnchorNode as we do
elsewhere now.

[wkorman, 2016-10-11 18:13:18]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 18:13:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-10-11 18:15:49]

lgtm

[wkorman, 2016-10-11 18:43:33]

wkorman@chromium.org changed reviewers:
+ yosin@google.com

[wkorman, 2016-10-11 18:43:34]

+yosin for core/editing OWNERS.

[commit-bot: I haz the power, 2016-10-11 19:50:20]

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

[commit-bot: I haz the power, 2016-10-11 19:50:21]

Dry run: This issue passed the CQ dry run.

[wkorman, 2016-10-12 18:03:54]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-10-12 18:04:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 18:25:56]

Committed patchset #6 (id:80002)

[commit-bot: I haz the power, 2016-10-12 18:29:01]

Description was changed from

==========
Use anchor node, rather than current selected node, to check editability.

BUG=645665
==========

to

==========
Use anchor node, rather than current selected node, to check editability.

BUG=645665

Committed: https://crrev.com/c0d0bd4e6f7e38c0208b78b2df9750ba7b0ceb0d
Cr-Commit-Position: refs/heads/master@{#424790}
==========

[commit-bot: I haz the power, 2016-10-12 18:29:02]

Patchset 6 (id:??) landed as
https://crrev.com/c0d0bd4e6f7e38c0208b78b2df9750ba7b0ceb0d
Cr-Commit-Position: refs/heads/master@{#424790}

[yosin_UTC9, 2016-10-13 03:58:47]

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

[yosin_UTC9, 2016-10-13 03:58:48]

Sorry for late response.

I'll work this during new caret architecture by lazy visible canonicalization:
http://crrev.com/1958093002

https://codereview.chromium.org/2401363003/diff/80002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right):

https://codereview.chromium.org/2401363003/diff/80002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:165: Node* newAnchorNode =
I think we should use layout object associated DisplayItem passed to
CaretBase::paintCaret(),
since actual painting will be done that.

Anyway, I'll visit again for http://crrev.com/1958093002