Invalidate paint of tickmarks on document change

Invalidate paint of tickmarks on document change

Invalidate paint of tickmarks are changed or removed because of
document change. We missed this kind of invalidations before.

Invalidation of tickmarks during find-in-page is still driven by
TextFinder as before because of the throttling algorithm.

BUG=477856
TEST=fast/repaint/text-match-document-change.html

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

[Xianzhu, 2015-04-17 18:25:02]

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

[Xianzhu, 2015-04-17 18:25:04]

https://codereview.chromium.org/1053123007/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/1053123007/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:3680: // Update the markers for spelling and
grammar checking.
This is removed because it's incorrect and not meaningful.

https://codereview.chromium.org/1053123007/diff/1/Source/web/TextFinder.cpp
File Source/web/TextFinder.cpp (left):

https://codereview.chromium.org/1053123007/diff/1/Source/web/TextFinder.cpp#o...
Source/web/TextFinder.cpp:154: ownerFrame().invalidateAll();
We don't need to invalidate all. DocumentMarkerController will invalidate the
layout objects containing the new markers.

https://codereview.chromium.org/1053123007/diff/1/Source/web/WebLocalFrameImp...
File Source/web/WebLocalFrameImpl.cpp (left):

https://codereview.chromium.org/1053123007/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.cpp:1511: invalidateScrollbar();
Now frameView will invalidate paint of tickmarks in setTickmarks().

https://codereview.chromium.org/1053123007/diff/1/Source/web/WebLocalFrameImpl.h
File Source/web/WebLocalFrameImpl.h (left):

https://codereview.chromium.org/1053123007/diff/1/Source/web/WebLocalFrameImp...
Source/web/WebLocalFrameImpl.h:332: 
These were for TextFinder only. Now TextFinder uses
FrameView::invalidatePaintOfTickmarks() instead. Now the fact that we are
painting tickmarks on vertical scrollbar is contained in core)

invalidateAll() is no longer used. The logic of it is also incorrect for
slimming paint.

[chrishtr, 2015-04-17 23:21:42]

https://codereview.chromium.org/1053123007/diff/1/Source/core/dom/DocumentMar...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/1053123007/diff/1/Source/core/dom/DocumentMar...
Source/core/dom/DocumentMarkerController.cpp:541:
m_textMatchMarkersRemovedOrChanged = true;
Why not just invalidate the tickmarks within each of these functions?

https://codereview.chromium.org/1053123007/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1053123007/diff/1/Source/core/frame/FrameView...
Source/core/frame/FrameView.h:349: void invalidatePaintOfTickmarks() const;
invalidatePaintForTickmarks

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

https://codereview.chromium.org/1053123007/diff/1/Source/web/TextFinder.cpp#n...
Source/web/TextFinder.cpp:154:
ownerFrame().frameView()->invalidatePaintOfTickmarks();
Why special code here just for tickmarks?

[Xianzhu, 2015-04-18 02:13:48]

https://codereview.chromium.org/1053123007/diff/1/Source/core/dom/DocumentMar...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/1053123007/diff/1/Source/core/dom/DocumentMar...
Source/core/dom/DocumentMarkerController.cpp:541:
m_textMatchMarkersRemovedOrChanged = true;
On 2015/04/17 23:21:42, chrishtr wrote:
> Why not just invalidate the tickmarks within each of these functions?

Was concerned about multiple calls of these functions in a cycle during a big
document change. Realized that calls to FrameView::invalidatePaintForTickmarks()
should be cheap or if not we should optimize it.

Done.

https://codereview.chromium.org/1053123007/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1053123007/diff/1/Source/core/frame/FrameView...
Source/core/frame/FrameView.h:349: void invalidatePaintOfTickmarks() const;
On 2015/04/17 23:21:42, chrishtr wrote:
> invalidatePaintForTickmarks

Done.

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

https://codereview.chromium.org/1053123007/diff/1/Source/web/TextFinder.cpp#n...
Source/web/TextFinder.cpp:154:
ownerFrame().frameView()->invalidatePaintOfTickmarks();
On 2015/04/17 23:21:42, chrishtr wrote:
> Why special code here just for tickmarks?

DocumentMarkerController invalidates the layoutObjects that are added markers
during addTextMatchMarker().

Tickmarks are not invalidated in DocumentMarkerController because of
http://crbug.com/6819. For now invalidations of tickmarks are throttled by
TextFinder.

[chrishtr, 2015-04-20 17:53:25]

https://codereview.chromium.org/1053123007/diff/60001/Source/core/dom/Documen...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/1053123007/diff/60001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:538: if (!node.isTextNode()) //
MarkerRemoverPridicate requires a Text node.
sp: MarkerRemoverPredicate

Also, why could the previous code get away without this check?

https://codereview.chromium.org/1053123007/diff/60001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1053123007/diff/60001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1081: DocumentMarkerController& markers =
frame().document()->markers();
The change here looks unnecessary.

https://codereview.chromium.org/1053123007/diff/60001/Source/web/TextFinder.cpp
File Source/web/TextFinder.cpp (left):

https://codereview.chromium.org/1053123007/diff/60001/Source/web/TextFinder.c...
Source/web/TextFinder.cpp:209: // Let the frame know that we don't want
tickmarks or highlighting anymore.
Highlighting is not relevant here? Just checking based on the previous comment.

[Xianzhu, 2015-04-20 18:13:33]

Patchset #5 (id:80001) has been deleted

[Xianzhu, 2015-04-20 18:13:48]

https://codereview.chromium.org/1053123007/diff/60001/Source/core/dom/Documen...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/1053123007/diff/60001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:538: if (!node.isTextNode()) //
MarkerRemoverPridicate requires a Text node.
On 2015/04/20 17:53:25, chrishtr wrote:
> sp: MarkerRemoverPredicate

Done. 

> Also, why could the previous code get away without this check?

It was at old line 534 in the inner-most loop.

https://codereview.chromium.org/1053123007/diff/60001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1053123007/diff/60001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1081: DocumentMarkerController& markers =
frame().document()->markers();
On 2015/04/20 17:53:25, chrishtr wrote:
> The change here looks unnecessary.

Done.

https://codereview.chromium.org/1053123007/diff/60001/Source/web/TextFinder.cpp
File Source/web/TextFinder.cpp (left):

https://codereview.chromium.org/1053123007/diff/60001/Source/web/TextFinder.c...
Source/web/TextFinder.cpp:209: // Let the frame know that we don't want
tickmarks or highlighting anymore.
On 2015/04/20 17:53:25, chrishtr wrote:
> Highlighting is not relevant here? Just checking based on the previous
comment.

Highlightings have been removed by line 205, not relevant to the next line.

[chrishtr, 2015-04-20 18:16:43]

lgtm

[Xianzhu, 2015-04-20 18:23:14]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-04-20 18:23:39]

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

[commit-bot: I haz the power, 2015-04-20 21:35:39]

Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194054