Invalidate caret during paint invalidation

Invalidate caret during paint invalidation

Previously we invalidated caret when it changed which caused several
issues such as accessing dirty layout, etc., which were also the same
issues when we had encountered before we had "repaint-after-layout".

This CL moves caret paint invalidation into paint invalidation stage
to avoid the issues.

It basically uses the same method as layout object paint invalidation:
save the previous visual rects of carets; when a caret changes,
invalidate both the old and the new visual rects.

BUG=682367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2665823002
Cr-Commit-Position: refs/heads/master@{#448799}
Committed: https://chromium.googlesource.com/chromium/src/+/94d07a45f859a3430ea9de1ff8c2a95708cb23b1

[Xianzhu, 2017-01-30 19:33:40]

Description was changed from

==========
Invalidate caret during paint invalidation

BUG=682367
==========

to

==========
Invalidate caret during paint invalidation

BUG=682367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-01-30 19:33:42]

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

[commit-bot: I haz the power, 2017-01-30 19:34:27]

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, 2017-01-30 20:36:14]

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

[commit-bot: I haz the power, 2017-01-30 20:36:15]

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

[Xianzhu, 2017-01-30 23:08:50]

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

[commit-bot: I haz the power, 2017-01-30 23:09:33]

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, 2017-01-31 00:43:01]

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

[commit-bot: I haz the power, 2017-01-31 00:43:02]

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

[Xianzhu, 2017-01-31 05:05:56]

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

[commit-bot: I haz the power, 2017-01-31 05:06:14]

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, 2017-01-31 06:33:18]

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

[commit-bot: I haz the power, 2017-01-31 06:33:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-01-31 06:55:31]

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

[commit-bot: I haz the power, 2017-01-31 06:55:44]

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, 2017-01-31 08:23:14]

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

[commit-bot: I haz the power, 2017-01-31 08:23:15]

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_...)

[Xianzhu, 2017-01-31 17:57:30]

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

[commit-bot: I haz the power, 2017-01-31 17:58:07]

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

[Xianzhu, 2017-01-31 18:03:09]

Description was changed from

==========
Invalidate caret during paint invalidation

BUG=682367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Invalidate caret during paint invalidation

Previously we invalidated caret when it changed which caused several
issues such as accessing dirty layout, etc., which were also the same
issues when we had encountered before we had "repaint-after-layout".

This CL moves caret paint invalidation into paint invalidation stage
to avoid the issues.

It basically uses the same method as layout object paint invalidation:
save the previous visual rects of carets; when a caret changes,
invalidate both the old and the new visual rects.

BUG=682367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-01-31 18:27:13]

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

[commit-bot: I haz the power, 2017-01-31 18:27:33]

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

[Xianzhu, 2017-01-31 18:39:51]

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

[Xianzhu, 2017-01-31 18:39:52]

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html
(left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:32:
});
This test is changed to a normal repaint test because the asserts are hard to
maintain.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/4776765-expected.txt
(left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/4776765-expected.txt:33:
},
These disappeared because the caret changed while the containing block needed
full paint invalidation.

The rects exceed the visual rect of the containing block by 1 pixel edges
because the inflate(1) for caret visual rects. The inflation seems unnecessary.
I would like to address this in a follow-up.

[commit-bot: I haz the power, 2017-01-31 19:50:10]

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

[commit-bot: I haz the power, 2017-01-31 19:50:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yosin_UTC9, 2017-01-31 20:12:56]

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

[yosin_UTC9, 2017-01-31 20:12:59]

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:151: if (LayoutBlock*
block = caretLayoutBlock())
Let's use early-return style to avoid to have (single line) else-clause.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:159:
block->setCaretsNeedPaintInvalidation();
Let's use early-return style to avoid to have (single line) else-clause.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:235: caretLocalRect(),
paintOffset, DisplayItem::kCaret);
Yes, this is what I thought. It is redundant to calculate caret rectangle twice,
one for invalidation and another for paint.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameCaret.h (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.h:104: void
contextDestroyed(Document*) final;
Since FrameCaret doesn't hold |Node| anymore, FrameCaret should not be derived
from |ynchronousMutationObserver|.
|contextDestroyed()| just does stopping caret blink time, we can safely remove
|contextDestroyed()|.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:112: const
DisplayItemClient& FrameSelection::getCaretDisplayItemClient() const {
I guess this renaming to help grate reformatting == member function name will
collide class name.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: || (isLayoutBlock()
&& toLayoutBlock(this)->hasCaret()) ||
Finally, You did it! (^_^)b

My approach[1] was rejected...

[1] http://crrev.com/2644793007: Defer caret color checking to invalidation
phase

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:20: bool
hasCursorCaret = false;
It is better to move |bool| member to end of member variables to avoid having
alignment padding?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:26: typedef
HashMap<const LayoutBlock*, CaretVisualRects> CaretVisualRectsMap;
nit: Let's use using to C++-ish.

using  CaretVisualRectsMap = HashMap<const LayoutBlock*, CaretVisualRects>;

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:34:
DCHECK(block.hasPreviousCaretVisualRects() ==
nit: DCHECK_EQ

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:37:
BlockPaintInvalidator::clearPreviousCaretVisualRects(block);
nit: early return style to decrease indentation of else-clause.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:45:
DCHECK(block.hasPreviousCaretVisualRects() ==
nit: DCHECK_EQ

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:53:
DCHECK(&localFrameRoot == localFrameRoot.localFrameRoot());
nit: DCHECK_EQ

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:54: for (auto&
item : caretVisualRectsMap()) {
nit: s/auto&/const auto&/
We don't never change entries in |caretVisualRectsMa()|

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:68: if
(block.hasPreviousCaretVisualRects()) {
nit: early-return style to decrease indentation.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:83:
newVisualRect.inflate(1);
Agree. Let's getting rid of this patch to see which layout tests failed.
I think there are not reset failures.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:108:
DCHECK(hadCarets == caretVisualRectsMap().contains(&m_block));
nit: DCHECK_EQ

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:110: bool
hasCursorCaret = m_block.hasCursorCaret();
nit: s/bool/const bool/

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:111: bool
hasDragCaret = m_block.hasDragCaret();
nit: s/bool/const bool/

[wkorman, 2017-01-31 20:40:29]

lgtm

This looks like a great change. I had only one minor documentation comment,
otherwise yosin@ feedback. If the layout tests pass then we should be in pretty
good shape :)

I think editing team wanted to change the Position type used, like maybe use
Position rather than PositionWithAffinity or something like that? But even there
this change would still be an improvement and would probably make that easier.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameCaret.h (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.h:77: // Return the caret rect
in local coordinate space of |caretLayoutBlock|.
Suggest adding to comment to define rect more explicitly, like, is it already
enclosing. Similar for other rect methods added/changed, as long as we are here.
Could do in separate change where you note you'll look into the expand by 1
which may no longer be needed.

[yosin_UTC9, 2017-01-31 21:03:05]

On 2017/01/31 at 20:40:29, wkorman wrote:
> lgtm
> 
> This looks like a great change. I had only one minor documentation comment,
otherwise yosin@ feedback. If the layout tests pass then we should be in pretty
good shape :)
> 
> I think editing team wanted to change the Position type used, like maybe use
Position rather than PositionWithAffinity or something like that? But even there
this change would still be an improvement and would probably make that easier.
> 
Editing team wants to use PositionWithAffinity, because
 1. Position.anchorNode() doesn't represents a right node where caret is in for
PositionAnchroType: BeforeAnchor, AfterAnchor. {Before,After}Anchor are
introduced for calculating node index for performance reason, e.g. Position in
large number of child nodes.
 2. Affinity is needed for caret at end of wrapped line.
Anyway, I'll handle this later.

As wkorman@ mentioned this is grate patch. I really love it and makes my life
easier.
I hope this patch also solves LoginTest.GaiaAuthOffline failure in [1] regarding
[1:1:0128/043956.679780:FATAL:CompositedLayerMapping.cpp(872)] Check failed:
layers[i].paintLayer->transformAncestor() == commonTransformAncestor.



[1] http://crrev.com/2637013002: Make FrameSelection to hold non-canonicalized
positions\

[chrishtr, 2017-01-31 22:51:04]

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-contenteditable-content-after-expected.txt
(left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-contenteditable-content-after-expected.txt:73:
},
Shouldn't there be at least one new invalidation to replace this list?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-invalidation-in-overflow-scroll-expected.txt
(right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-invalidation-in-overflow-scroll-expected.txt:11:
"rect": [360, 7, 26, 24],
Is the change of rect because it's now invalidating on the block rather than the
LayoutText?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-invalidation-in-overflow-scroll-expected.txt:30:
"reason": "outline"
Why this change?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-with-composited-scroll-expected.txt
(left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-with-composited-scroll-expected.txt:37:
"object": "LayoutBlockFlow DIV id='inner-editor'",
Should there be a new rect to invalidate for the caret to replace these?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/invalidate-caret-in-composited-scrolling-container-expected.txt
(right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/invalidate-caret-in-composited-scrolling-container-expected.txt:51:
"underPaintInvalidations": [
Under-invalidations?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameCaret.cpp (left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:161: if
(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
Why is this no longer needed?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3029:
BlockPaintInvalidator::setMayNeedPaintInvalidationForCaretsIfNeeded(frame());
Why is this needed?

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3256:
BlockPaintInvalidator::setMayNeedPaintInvalidationForCaretsIfNeeded(
Same quesiton.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBlock.h (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBlock.h:510: // Called when a cursor
caret or drag caret in this LayoutBlock need paint
Nit: "needs"

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BlockPaintInvalidator.cpp:28:
DEFINE_STATIC_LOCAL(CaretVisualRectsMap, map, ());
Why can't you store these on the block instead of in a global map?

[Xianzhu, 2017-02-02 04:51:27]

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

[commit-bot: I haz the power, 2017-02-02 04:51:49]

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, 2017-02-02 05:21:22]

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

[commit-bot: I haz the power, 2017-02-02 05:21:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-02-02 21:31:55]

Patchset #8 (id:140001) has been deleted

[Xianzhu, 2017-02-02 23:41:35]

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

[Xianzhu, 2017-02-02 23:42:07]

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-contenteditable-content-after-expected.txt
(left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-contenteditable-content-after-expected.txt:73:
},
On 2017/01/31 22:51:03, chrishtr wrote:
> Shouldn't there be at least one new invalidation to replace this list?

Now we skip caret rect invalidation if its containing block is doing full
invalidation. We have full rect invalidation at line 10-28.

We still invalidate the display item client of the caret.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-invalidation-in-overflow-scroll-expected.txt
(right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-invalidation-in-overflow-scroll-expected.txt:11:
"rect": [360, 7, 26, 24],
On 2017/01/31 22:51:03, chrishtr wrote:
> Is the change of rect because it's now invalidating on the block rather than
the
> LayoutText?

This shows a regression about outline invalidation.
https://codereview.chromium.org/2669003002/ addresses it.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-with-composited-scroll-expected.txt
(left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-with-composited-scroll-expected.txt:37:
"object": "LayoutBlockFlow DIV id='inner-editor'",
On 2017/01/31 22:51:03, chrishtr wrote:
> Should there be a new rect to invalidate for the caret to replace these?

These smaller invalidations are omitted because the object is doing full paint
invalidation.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/invalidate-caret-in-composited-scrolling-container-expected.txt
(right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/invalidate-caret-in-composited-scrolling-container-expected.txt:51:
"underPaintInvalidations": [
On 2017/01/31 22:51:03, chrishtr wrote:
> Under-invalidations?

Oops. Wondering why I overlooked this. We should always invalidate caret on the
scrolling layer. Fixed.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameCaret.cpp (left):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.cpp:161: if
(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
On 2017/01/31 22:51:03, chrishtr wrote:
> Why is this no longer needed?

Now block->setMayNeedPaintInvalidation() marks the ancestor chain.

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/FrameCaret.h (right):

https://codereview.chromium.org/2665823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/FrameCaret.h:104: void
contextDestroyed(Document*) final;
On 2017/01/31 20:12:58, yosin_BlinkOn_slow wrote:
> Since FrameCaret doesn't hold |Node| anymore, FrameCaret should not be derived
> from |ynchronousMutationObserver|.
> |contextDestroyed()| just does stopping caret blink time, we can safely remove
> |contextDestroyed()|.

Great. Done.

[commit-bot: I haz the power, 2017-02-02 23:42:21]

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

[Xianzhu, 2017-02-02 23:45:05]

The latest patch is refactored. Ptal.

- Removed the global map in BlockPaintInvalidator. Now enclose all paint
invalidation status in CaretDisplayItemClient.

- Fixed issue of composited scrolled caret in the previous patch.

- Address reviewer comments.

[Xianzhu, 2017-02-02 23:46:46]

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

[commit-bot: I haz the power, 2017-02-02 23:47:03]

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, 2017-02-03 01:14:48]

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

[commit-bot: I haz the power, 2017-02-03 01:14:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-02-03 02:07:01]

https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: || (isLayoutBlock()
&& toLayoutBlock(this)->hasCaret()) ||
Why can this line go?

[Xianzhu, 2017-02-03 04:27:59]

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

[commit-bot: I haz the power, 2017-02-03 04:28:28]

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

[Xianzhu, 2017-02-03 04:28:58]

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

[commit-bot: I haz the power, 2017-02-03 04:29:23]

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

[Xianzhu, 2017-02-03 04:31:28]

https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: || (isLayoutBlock()
&& toLayoutBlock(this)->hasCaret()) ||
On 2017/02/03 02:07:01, chrishtr wrote:
> Why can this line go?

Now we always check for caret change in each frame in
CaretDisplayItemClient::updateForPaintInvalidation() which is indirectly called
from FrameView::prePaint and FrameView::invalidateTreeIfNeededRecursive(). This
makes all paint invalidations of carets enclosed in CaretDisplayItemClient, with
little additional cost*. Caret color is also checked in
CaretDisplayItemClient::updateForPaintInvalidation().

This is also the reason that some setCaretRectNeedsUpdate calls are removed.

* Actually in many cases the cost is reduced because now we call
CaretDisplayItemClient::computeCaretRect() and
CaretDisplayItemClient::caretLayoutObject() only once in each frame, instead of
many times before.

[commit-bot: I haz the power, 2017-02-03 06:47:38]

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

[commit-bot: I haz the power, 2017-02-03 06:47:40]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chrishtr, 2017-02-03 16:13:20]

On 2017/02/03 at 04:31:28, wangxianzhu wrote:
>
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):
> 
>
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: ||
(isLayoutBlock() && toLayoutBlock(this)->hasCaret()) ||
> On 2017/02/03 02:07:01, chrishtr wrote:
> > Why can this line go?
> 
> Now we always check for caret change in each frame in
CaretDisplayItemClient::updateForPaintInvalidation() which is indirectly called
from FrameView::prePaint and FrameView::invalidateTreeIfNeededRecursive(). This
makes all paint invalidations of carets enclosed in CaretDisplayItemClient, with
little additional cost*. Caret color is also checked in
CaretDisplayItemClient::updateForPaintInvalidation().
> 
> This is also the reason that some setCaretRectNeedsUpdate calls are removed.
> 
> * Actually in many cases the cost is reduced because now we call
CaretDisplayItemClient::computeCaretRect() and
CaretDisplayItemClient::caretLayoutObject() only once in each frame, instead of
many times before.

Why should caret get special treatment in this way? Shouldn't it be recomputed
only if a
layout or other event has caused it to need re-computing? Similarly, if between
blinks of
a caret, a style changes its color, I would expect adjustSyleDifference to be
where
invalidation is registered.

I considered commenting earlier on the special code in FrameView being strange,
but didn't
...

[Xianzhu, 2017-02-03 18:37:55]

On 2017/02/03 16:13:20, chrishtr wrote:
> On 2017/02/03 at 04:31:28, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):
> > 
> >
>
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: ||
> (isLayoutBlock() && toLayoutBlock(this)->hasCaret()) ||
> > On 2017/02/03 02:07:01, chrishtr wrote:
> > > Why can this line go?
> > 
> > Now we always check for caret change in each frame in
> CaretDisplayItemClient::updateForPaintInvalidation() which is indirectly
called
> from FrameView::prePaint and FrameView::invalidateTreeIfNeededRecursive().
This
> makes all paint invalidations of carets enclosed in CaretDisplayItemClient,
with
> little additional cost*. Caret color is also checked in
> CaretDisplayItemClient::updateForPaintInvalidation().
> > 
> > This is also the reason that some setCaretRectNeedsUpdate calls are removed.
> > 
> > * Actually in many cases the cost is reduced because now we call
> CaretDisplayItemClient::computeCaretRect() and
> CaretDisplayItemClient::caretLayoutObject() only once in each frame, instead
of
> many times before.
> 
> Why should caret get special treatment in this way? Shouldn't it be recomputed
> only if a
> layout or other event has caused it to need re-computing? Similarly, if
between
> blinks of
> a caret, a style changes its color, I would expect adjustSyleDifference to be
> where
> invalidation is registered.
> 
> I considered commenting earlier on the special code in FrameView being
strange,
> but didn't
> ...

I considered two choices:
1. like what you said, marking caret needs paint invalidation or may need paint
invalidation on every event that may change the caret;
2. like in the latest patch, checking for changes of caret in each frame.

Here are the reasons that I chose 2:

Choice 2 is like that we always mark the caret mayNeedPaintInvalidation (so we
don't need to mark it mayNeedPaintInvalidation everywhere that is needed and can
avoid any under-invalidation in theory). If we could do this for layout objects
then we could also avoid many under-invalidations easily, but we can't because
of performance. However for caret the extra cost is little because there is only
1 caret in each FrameView.

Choice 2 can avoid the following complex logics for marking the caret
needs/mayNeed paint invalidation:
- detection of CharacterData change causing the caret position to point to a
different layout object.
- detection of color change. (The previous logic was incomplete e.g.
<div><span></span></div> span color change can also affect caret color. Using
choice 2 we don't need to bother fixing it.)
- many hookups in selection code to detect situations that will possibly change
the caret.
- detection of other events that may change the caret, e.g. change of editable
status of the containing block.

I think choice 2 can make selection code and paint invalidation code less
coupled and make paint invalidation of caret less fragile. It also enables
caching of results of CaretDisplayItemClient::computeCaretRect() and
caretLayoutObject().

[chrishtr, 2017-02-06 22:54:11]

On 2017/02/03 at 18:37:55, wangxianzhu wrote:
> On 2017/02/03 16:13:20, chrishtr wrote:
> > On 2017/02/03 at 04:31:28, wangxianzhu wrote:
> > >
> >
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):
> > > 
> > >
> >
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: ||
> > (isLayoutBlock() && toLayoutBlock(this)->hasCaret()) ||
> > > On 2017/02/03 02:07:01, chrishtr wrote:
> > > > Why can this line go?
> > > 
> > > Now we always check for caret change in each frame in
> > CaretDisplayItemClient::updateForPaintInvalidation() which is indirectly
called
> > from FrameView::prePaint and FrameView::invalidateTreeIfNeededRecursive().
This
> > makes all paint invalidations of carets enclosed in CaretDisplayItemClient,
with
> > little additional cost*. Caret color is also checked in
> > CaretDisplayItemClient::updateForPaintInvalidation().
> > > 
> > > This is also the reason that some setCaretRectNeedsUpdate calls are
removed.
> > > 
> > > * Actually in many cases the cost is reduced because now we call
> > CaretDisplayItemClient::computeCaretRect() and
> > CaretDisplayItemClient::caretLayoutObject() only once in each frame, instead
of
> > many times before.
> > 
> > Why should caret get special treatment in this way? Shouldn't it be
recomputed
> > only if a
> > layout or other event has caused it to need re-computing? Similarly, if
between
> > blinks of
> > a caret, a style changes its color, I would expect adjustSyleDifference to
be
> > where
> > invalidation is registered.
> > 
> > I considered commenting earlier on the special code in FrameView being
strange,
> > but didn't
> > ...
> 
> I considered two choices:
> 1. like what you said, marking caret needs paint invalidation or may need
paint invalidation on every event that may change the caret;
> 2. like in the latest patch, checking for changes of caret in each frame.
> 
> Here are the reasons that I chose 2:
> 
> Choice 2 is like that we always mark the caret mayNeedPaintInvalidation (so we
don't need to mark it mayNeedPaintInvalidation everywhere that is needed and can
avoid any under-invalidation in theory). If we could do this for layout objects
then we could also avoid many under-invalidations easily, but we can't because
of performance. However for caret the extra cost is little because there is only
1 caret in each FrameView.

I can see three downsides to this approach:
- (perf) Caret-work is done even when there is no caret
- (complexity) Carets are not treated uniformly like other DisplayItemClients;
need to understand special code here and there


> 
> Choice 2 can avoid the following complex logics for marking the caret
needs/mayNeed paint invalidation:
> - detection of CharacterData change causing the caret position to point to a
different layout object.
> - detection of color change. (The previous logic was incomplete e.g.
<div><span></span></div> span color change can also affect caret color. Using
choice 2 we don't need to bother fixing it.)
> - many hookups in selection code to detect situations that will possibly
change the caret.

Don't all of the already exist in the code before this CL? Can you just
conservatively sprinkle setMayNeedPaintInvalidation in the caret code when it
does anything?

> - detection of other events that may change the caret, e.g. change of editable
status of the containing block.

Same point here - it's handled somewhere already, add a callsite there?

> 
> I think choice 2 can make selection code and paint invalidation code less
coupled and make paint invalidation of caret less fragile. It also enables
caching of results of CaretDisplayItemClient::computeCaretRect() and
caretLayoutObject().

Expanding on my comment above, a nice compromise could be to very conservatively
setMayNeedPaintInvalidation in the
caret code whenever anything related to carets happens. This minimizes
complexity and under-painting risk, and
also has better performance when there is no caret & no code in FrameView.

[Xianzhu, 2017-02-07 03:51:04]

On 2017/02/06 22:54:11, chrishtr wrote:
> On 2017/02/03 at 18:37:55, wangxianzhu wrote:
> > On 2017/02/03 16:13:20, chrishtr wrote:
> > > On 2017/02/03 at 04:31:28, wangxianzhu wrote:
> > > >
> > >
>
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: ||
> > > (isLayoutBlock() && toLayoutBlock(this)->hasCaret()) ||
> > > > On 2017/02/03 02:07:01, chrishtr wrote:
> > > > > Why can this line go?
> > > > 
> > > > Now we always check for caret change in each frame in
> > > CaretDisplayItemClient::updateForPaintInvalidation() which is indirectly
> called
> > > from FrameView::prePaint and FrameView::invalidateTreeIfNeededRecursive().
> This
> > > makes all paint invalidations of carets enclosed in
CaretDisplayItemClient,
> with
> > > little additional cost*. Caret color is also checked in
> > > CaretDisplayItemClient::updateForPaintInvalidation().
> > > > 
> > > > This is also the reason that some setCaretRectNeedsUpdate calls are
> removed.
> > > > 
> > > > * Actually in many cases the cost is reduced because now we call
> > > CaretDisplayItemClient::computeCaretRect() and
> > > CaretDisplayItemClient::caretLayoutObject() only once in each frame,
instead
> of
> > > many times before.
> > > 
> > > Why should caret get special treatment in this way? Shouldn't it be
> recomputed
> > > only if a
> > > layout or other event has caused it to need re-computing? Similarly, if
> between
> > > blinks of
> > > a caret, a style changes its color, I would expect adjustSyleDifference to
> be
> > > where
> > > invalidation is registered.
> > > 
> > > I considered commenting earlier on the special code in FrameView being
> strange,
> > > but didn't
> > > ...
> > 
> > I considered two choices:
> > 1. like what you said, marking caret needs paint invalidation or may need
> paint invalidation on every event that may change the caret;
> > 2. like in the latest patch, checking for changes of caret in each frame.
> > 
> > Here are the reasons that I chose 2:
> > 
> > Choice 2 is like that we always mark the caret mayNeedPaintInvalidation (so
we
> don't need to mark it mayNeedPaintInvalidation everywhere that is needed and
can
> avoid any under-invalidation in theory). If we could do this for layout
objects
> then we could also avoid many under-invalidations easily, but we can't because
> of performance. However for caret the extra cost is little because there is
only
> 1 caret in each FrameView.
> 

Actually I see advantages about performance and complexity :) Perhaps it would
help if I named CaretDisplayItemClient::updateForPaintInvalidation() as
updateStyleAndLayoutIfNeeded() whose implementation can be abstracted to:
  if (hasCaret)
    updateStyleAndLayout();

> I can see three downsides to this approach:
> - (perf) Caret-work is done even when there is no caret

- CaretDisplayItemClient::updateForPaintInvalidation() is fast. It costs less
than the original "m_frame->selection().invalidateCaretRect()" in FrameView.cpp.

- When there is no caret, updateForPaintInvalidation() basically just checks if
there is no caret and return. This costs the same as the original
LayoutBlock::hasCaret().

- LayoutBlock::hasCaret() (now shouldPaintCarets()) is hot because it's checked
for every LayoutBlock needing to check for paint invalidation. Now it's super
fast with cached results of updateForPaintInvalidation().

> - (complexity) Carets are not treated uniformly like other DisplayItemClients;
> need to understand special code here and there

If we think of updateForPaintInvalidation() as updateStyleAndLayoutIfNeeded(),
then CaretDisplayItemClient has no big difference from the others:

  - updateSytleAndLayoutIfNeeded(): update style and geometries if needed, mark
for paint invalidation if needed;
  - invalidatePaintIfNeeded(): check for changes and issue paint invalidation if
needed.

> > 
> > Choice 2 can avoid the following complex logics for marking the caret
> needs/mayNeed paint invalidation:
> > - detection of CharacterData change causing the caret position to point to a
> different layout object.
> > - detection of color change. (The previous logic was incomplete e.g.
> <div><span></span></div> span color change can also affect caret color. Using
> choice 2 we don't need to bother fixing it.)
> > - many hookups in selection code to detect situations that will possibly
> change the caret.
> 
> Don't all of the already exist in the code before this CL? Can you just
> conservatively sprinkle setMayNeedPaintInvalidation in the caret code when it
> does anything?
> 
> > - detection of other events that may change the caret, e.g. change of
editable
> status of the containing block.
> 
> Same point here - it's handled somewhere already, add a callsite there?
>
> > 
> > I think choice 2 can make selection code and paint invalidation code less
> coupled and make paint invalidation of caret less fragile. It also enables
> caching of results of CaretDisplayItemClient::computeCaretRect() and
> caretLayoutObject().
> 
> Expanding on my comment above, a nice compromise could be to very
conservatively
> setMayNeedPaintInvalidation in the
> caret code whenever anything related to carets happens. This minimizes
> complexity and under-painting risk, and
> also has better performance when there is no caret & no code in FrameView.

I'm not sure if we could make setMayNeedPaintInvalidation completely for each of
such situations, and if there are still hidden bugs of incomplete change
notification. updateForPaintInvalidation() is a solution for all known and
potential issues.

Here are some complexities that existed in the original code which were handled
with complex logic or had bugs, and the updateForPaintInvalidation() method can
avoid (and I just newly noticed some of them when reading more code):

- About caret color change: previously we had a bug about color change of inline
containing a caret. If we used setNeedsPaintInvalidation(), when an inline
changes color, we would need to check if the caret is affected by the inline
which is not easy: LayoutObject::setStyle() may need to traverse the layout
tree, and know how the caret system determines which block paints the caret.
Using updateForPaintInvalidation() this is very simple.

- A chicken-and-egg issue: previously hasCaret() depends on layout tree which
may be dirty during style recalc when called from setStyle(). Previously we
ignored the dirty layout tree:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/E....
With this CL, we access hasCaret() (now shouldPaintCaret()) only after layout,
and it's fast now.

- It's not trivial to detect hasEditableStyle change of a block because it may
be affected by ancestors. Even if did it, we might need to separate hasCaret()
and shouldPaintCaret() concepts of a block, because at first we need to know if
the current caret position is in a block, then check hasEditableStyle change of
the block to determine if we should invalidate the caret. With this CL we don't
need the hasCaret() concept.

- Other previous complex logics (e.g. CharacterData change notification and
SynchronousMutationObserver in FrameCaret) removed by this CL.

[chrishtr, 2017-02-07 05:29:06]

On 2017/02/07 at 03:51:04, wangxianzhu wrote:
> On 2017/02/06 22:54:11, chrishtr wrote:
> > On 2017/02/03 at 18:37:55, wangxianzhu wrote:
> > > On 2017/02/03 16:13:20, chrishtr wrote:
> > > > On 2017/02/03 at 04:31:28, wangxianzhu wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > > > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/2665823002/diff/180001/third_party/WebKit/Sou...
> > > > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1523: ||
> > > > (isLayoutBlock() && toLayoutBlock(this)->hasCaret()) ||
> > > > > On 2017/02/03 02:07:01, chrishtr wrote:
> > > > > > Why can this line go?
> > > > > 
> > > > > Now we always check for caret change in each frame in
> > > > CaretDisplayItemClient::updateForPaintInvalidation() which is indirectly
> > called
> > > > from FrameView::prePaint and
FrameView::invalidateTreeIfNeededRecursive().
> > This
> > > > makes all paint invalidations of carets enclosed in
CaretDisplayItemClient,
> > with
> > > > little additional cost*. Caret color is also checked in
> > > > CaretDisplayItemClient::updateForPaintInvalidation().
> > > > > 
> > > > > This is also the reason that some setCaretRectNeedsUpdate calls are
> > removed.
> > > > > 
> > > > > * Actually in many cases the cost is reduced because now we call
> > > > CaretDisplayItemClient::computeCaretRect() and
> > > > CaretDisplayItemClient::caretLayoutObject() only once in each frame,
instead
> > of
> > > > many times before.
> > > > 
> > > > Why should caret get special treatment in this way? Shouldn't it be
> > recomputed
> > > > only if a
> > > > layout or other event has caused it to need re-computing? Similarly, if
> > between
> > > > blinks of
> > > > a caret, a style changes its color, I would expect adjustSyleDifference
to
> > be
> > > > where
> > > > invalidation is registered.
> > > > 
> > > > I considered commenting earlier on the special code in FrameView being
> > strange,
> > > > but didn't
> > > > ...
> > > 
> > > I considered two choices:
> > > 1. like what you said, marking caret needs paint invalidation or may need
> > paint invalidation on every event that may change the caret;
> > > 2. like in the latest patch, checking for changes of caret in each frame.
> > > 
> > > Here are the reasons that I chose 2:
> > > 
> > > Choice 2 is like that we always mark the caret mayNeedPaintInvalidation
(so we
> > don't need to mark it mayNeedPaintInvalidation everywhere that is needed and
can
> > avoid any under-invalidation in theory). If we could do this for layout
objects
> > then we could also avoid many under-invalidations easily, but we can't
because
> > of performance. However for caret the extra cost is little because there is
only
> > 1 caret in each FrameView.
> > 
> 
> Actually I see advantages about performance and complexity :) Perhaps it would
help if I named CaretDisplayItemClient::updateForPaintInvalidation() as
updateStyleAndLayoutIfNeeded() whose implementation can be abstracted to:
>   if (hasCaret)
>     updateStyleAndLayout();
> 
> > I can see three downsides to this approach:
> > - (perf) Caret-work is done even when there is no caret
> 
> - CaretDisplayItemClient::updateForPaintInvalidation() is fast. It costs less
than the original "m_frame->selection().invalidateCaretRect()" in FrameView.cpp.
> 
> - When there is no caret, updateForPaintInvalidation() basically just checks
if there is no caret and return. This costs the same as the original
LayoutBlock::hasCaret().
> 
> - LayoutBlock::hasCaret() (now shouldPaintCarets()) is hot because it's
checked for every LayoutBlock needing to check for paint invalidation. Now it's
super fast with cached results of updateForPaintInvalidation().
> 
> > - (complexity) Carets are not treated uniformly like other
DisplayItemClients;
> > need to understand special code here and there
> 
> If we think of updateForPaintInvalidation() as updateStyleAndLayoutIfNeeded(),
then CaretDisplayItemClient has no big difference from the others:
> 
>   - updateSytleAndLayoutIfNeeded(): update style and geometries if needed,
mark for paint invalidation if needed;
>   - invalidatePaintIfNeeded(): check for changes and issue paint invalidation
if needed.
> 
> > > 
> > > Choice 2 can avoid the following complex logics for marking the caret
> > needs/mayNeed paint invalidation:
> > > - detection of CharacterData change causing the caret position to point to
a
> > different layout object.
> > > - detection of color change. (The previous logic was incomplete e.g.
> > <div><span></span></div> span color change can also affect caret color.
Using
> > choice 2 we don't need to bother fixing it.)
> > > - many hookups in selection code to detect situations that will possibly
> > change the caret.
> > 
> > Don't all of the already exist in the code before this CL? Can you just
> > conservatively sprinkle setMayNeedPaintInvalidation in the caret code when
it
> > does anything?
> > 
> > > - detection of other events that may change the caret, e.g. change of
editable
> > status of the containing block.
> > 
> > Same point here - it's handled somewhere already, add a callsite there?
> >
> > > 
> > > I think choice 2 can make selection code and paint invalidation code less
> > coupled and make paint invalidation of caret less fragile. It also enables
> > caching of results of CaretDisplayItemClient::computeCaretRect() and
> > caretLayoutObject().
> > 
> > Expanding on my comment above, a nice compromise could be to very
conservatively
> > setMayNeedPaintInvalidation in the
> > caret code whenever anything related to carets happens. This minimizes
> > complexity and under-painting risk, and
> > also has better performance when there is no caret & no code in FrameView.
> 
> I'm not sure if we could make setMayNeedPaintInvalidation completely for each
of such situations, and if there are still hidden bugs of incomplete change
notification. updateForPaintInvalidation() is a solution for all known and
potential issues.
> 
> Here are some complexities that existed in the original code which were
handled with complex logic or had bugs, and the updateForPaintInvalidation()
method can avoid (and I just newly noticed some of them when reading more code):
> 
> - About caret color change: previously we had a bug about color change of
inline containing a caret. If we used setNeedsPaintInvalidation(), when an
inline changes color, we would need to check if the caret is affected by the
inline which is not easy: LayoutObject::setStyle() may need to traverse the
layout tree, and know how the caret system determines which block paints the
caret. Using updateForPaintInvalidation() this is very simple.

In this particular case you could just always invalidate the caret right?

> 
> - A chicken-and-egg issue: previously hasCaret() depends on layout tree which
may be dirty during style recalc when called from setStyle(). Previously we
ignored the dirty layout tree:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/E....
With this CL, we access hasCaret() (now shouldPaintCaret()) only after layout,
and it's fast now.
> 
> - It's not trivial to detect hasEditableStyle change of a block because it may
be affected by ancestors. Even if did it, we might need to separate hasCaret()
and shouldPaintCaret() concepts of a block, because at first we need to know if
the current caret position is in a block, then check hasEditableStyle change of
the block to determine if we should invalidate the caret. With this CL we don't
need the hasCaret() concept.
> 
> - Other previous complex logics (e.g. CharacterData change notification and
SynchronousMutationObserver in FrameCaret) removed by this CL.

[chrishtr, 2017-02-07 05:29:12]

https://codereview.chromium.org/2665823002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2665823002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3045:
frameView.m_frame->page()->dragCaret().updateForPaintInvalidation();
How about conceptually adding this at the end of layout instead?
Then the code would be named and put in that place also.

[Xianzhu, 2017-02-07 19:57:34]

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

[commit-bot: I haz the power, 2017-02-07 19:58:17]

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

[Xianzhu, 2017-02-07 20:00:15]

https://codereview.chromium.org/2665823002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2665823002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:3045:
frameView.m_frame->page()->dragCaret().updateForPaintInvalidation();
On 2017/02/07 05:29:12, chrishtr wrote:
> How about conceptually adding this at the end of layout instead?
> Then the code would be named and put in that place also.

Done, with the functions renamed to updateStyleAndLayoutIfNeeded().

[commit-bot: I haz the power, 2017-02-07 21:31:02]

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

[commit-bot: I haz the power, 2017-02-07 21:31:04]

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

[chrishtr, 2017-02-07 21:33:07]

lgtm

[Xianzhu, 2017-02-07 21:50:12]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-07 21:50:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
wkorman@chromium.org, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2665823002/#ps240001
(title: "NeedsRebaseline")

[commit-bot: I haz the power, 2017-02-07 21:50:53]

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, 2017-02-07 23:51:28]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486504212153930,
"parent_rev": "8db16cac28a12030e03f709c266b8cef1e5b22ed", "commit_rev":
"94d07a45f859a3430ea9de1ff8c2a95708cb23b1"}

[commit-bot: I haz the power, 2017-02-07 23:52:01]

Description was changed from

==========
Invalidate caret during paint invalidation

Previously we invalidated caret when it changed which caused several
issues such as accessing dirty layout, etc., which were also the same
issues when we had encountered before we had "repaint-after-layout".

This CL moves caret paint invalidation into paint invalidation stage
to avoid the issues.

It basically uses the same method as layout object paint invalidation:
save the previous visual rects of carets; when a caret changes,
invalidate both the old and the new visual rects.

BUG=682367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Invalidate caret during paint invalidation

Previously we invalidated caret when it changed which caused several
issues such as accessing dirty layout, etc., which were also the same
issues when we had encountered before we had "repaint-after-layout".

This CL moves caret paint invalidation into paint invalidation stage
to avoid the issues.

It basically uses the same method as layout object paint invalidation:
save the previous visual rects of carets; when a caret changes,
invalidate both the old and the new visual rects.

BUG=682367
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2665823002
Cr-Commit-Position: refs/heads/master@{#448799}
Committed:
https://chromium.googlesource.com/chromium/src/+/94d07a45f859a3430ea9de1ff8c2...
==========

[commit-bot: I haz the power, 2017-02-07 23:52:03]

Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/94d07a45f859a3430ea9de1ff8c2...