Move selection invalidation to the invalidation phase

Move selection invalidation to the invalidation phase

https://codereview.chromium.org/620553009 moved some of the
selection-related invalidations to the invalidation phase.
This change fixes the original logic that wasn't totally right.
The core issue is that we have to store the old selection rectangle
as we can't compute it as part of the next frame (all the selection
information have been updated by then).

This change also includes some simplification of the
selection code now that we don't need much of the logic.

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

[Julien - ping for review, 2014-10-30 23:09:11]

jchaffraix@chromium.org changed reviewers:
+ bemjb@chromium.org, dsinclair@chromium.org, wangxianzhu@chromium.org

[Julien - ping for review, 2014-10-30 23:09:12]

Full move to RACUN. Not super happy about the shape-outside part (+Bem in case
he has some better idea).

[Xianzhu, 2014-10-30 23:35:42]

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1002: void
setpreviousSelectionRectForPaintInvalidation(const LayoutRect& rect) {
m_previousSelectionRectForPaintInvalidation = rect;
setShouldInvalidateSelection(); }
/setprevious/setPrevious/

And it seems better to make it one statement per line.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1401: LayoutRect
m_previousSelectionRectForPaintInvalidation;
Seems not the best to add another rect :(

As only a very small set of object can have selection at a certain time, can we
use the global map method (like RenderBox's
gOverrideContainingBlockLogicalHeightMap) to store it?

[dsinclair, 2014-10-31 14:03:16]

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:133: LayoutRect rect2; // Stores the
previous selection paint invalidation rect.
Would something like the following be better?

LayoutRect rect[2]; // Stores the previous paint and selection invalidation
rects.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1277:
invalidateSelectionIfNeeded(paintInvalidationContainer);
We need to update the rect regardless, but we don't have to issue the
invalidation if the PaintInvalidatinoReason is a full invalidation right?

Could we do some magic here to make sure we don't issue extra invalidations (or
is it just not worth the effort?)

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1401: LayoutRect
m_previousSelectionRectForPaintInvalidation;
On 2014/10/30 23:35:42, Xianzhu wrote:
> Seems not the best to add another rect :(
> 
> As only a very small set of object can have selection at a certain time, can
we
> use the global map method (like RenderBox's
> gOverrideContainingBlockLogicalHeightMap) to store it?


Global maps make things more confusing. Do we know if this will cause noticeable
performance or memory issues?

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:520: if (object->isBox()) {
if (!object->isBox())
    return;

I dislike indenting, heh.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:524: // but not during the invalidation
phase so we just match the current code by forcing the loading to happen here.
Can we add a crbug for this so we don't lose track?

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:553: typedef
WillBeHeapHashMap<RawPtrWillBeMember<RenderObject>, SelectionState >
SelectedObjectMap;
nit: Don't need the space between SelectionState and >  (and below)

[Xianzhu, 2014-10-31 15:52:35]

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1401: LayoutRect
m_previousSelectionRectForPaintInvalidation;
On 2014/10/31 14:03:16, dsinclair wrote:
> On 2014/10/30 23:35:42, Xianzhu wrote:
> > Seems not the best to add another rect :(
> > 
> > As only a very small set of object can have selection at a certain time, can
> we
> > use the global map method (like RenderBox's
> > gOverrideContainingBlockLogicalHeightMap) to store it?
> 
> 
> Global maps make things more confusing. Do we know if this will cause
noticeable
> performance or memory issues?

Got the feeling that every bit in RenderObject is precious when I tried to add a
m_previousBorderBox field and got an objection. I'm feeling a bit unfair that I
couldn't add that field which is also critical for correct incremental
invalidation for boxes with children visual overflow :)

No objection to the new field if we have data about RenderObject memory usage.
(And I believe these would be gone with slimming paint because there would be no
more paintInvalidationRects.)

[Julien - ping for review, 2014-10-31 22:31:03]

jchaffraix@chromium.org changed reviewers:
+ pdr@chromium.org

[Julien - ping for review, 2014-10-31 22:31:03]

New patch up for review.

+pdr (CC'ed) who helped me get the disabler going as he should review this part
of the change.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1277:
invalidateSelectionIfNeeded(paintInvalidationContainer);
On 2014/10/31 at 14:03:16, dsinclair wrote:
> We need to update the rect regardless, but we don't have to issue the
invalidation if the PaintInvalidatinoReason is a full invalidation right?
> 
> Could we do some magic here to make sure we don't issue extra invalidations
(or is it just not worth the effort?)

It was easy to do, added it.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1002: void
setpreviousSelectionRectForPaintInvalidation(const LayoutRect& rect) {
m_previousSelectionRectForPaintInvalidation = rect;
setShouldInvalidateSelection(); }
On 2014/10/30 at 23:35:42, Xianzhu wrote:
> /setprevious/setPrevious/

Doh', fixed!

> 
> And it seems better to make it one statement per line.

Good point, I refactored the logic, moving the implementation into the cpp file.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1401: LayoutRect
m_previousSelectionRectForPaintInvalidation;
On 2014/10/31 at 15:52:35, Xianzhu wrote:
> On 2014/10/31 14:03:16, dsinclair wrote:
> > On 2014/10/30 23:35:42, Xianzhu wrote:
> > > Seems not the best to add another rect :(
> > > 
> > > As only a very small set of object can have selection at a certain time,
can
> > we
> > > use the global map method (like RenderBox's
> > > gOverrideContainingBlockLogicalHeightMap) to store it?
> > 
> > 
> > Global maps make things more confusing. Do we know if this will cause
noticeable
> > performance or memory issues?
> 
> Got the feeling that every bit in RenderObject is precious when I tried to add
a m_previousBorderBox field and got an objection. I'm feeling a bit unfair that
I couldn't add that field which is also critical for correct incremental
invalidation for boxes with children visual overflow :)
> 
> No objection to the new field if we have data about RenderObject memory usage.
(And I believe these would be gone with slimming paint because there would be no
more paintInvalidationRects.)

I think I have found a way to make a map work (didn't think it was possible but
I overlooked some details). That will prevent another RenderObject size
increase.

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/665673004/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:524: // but not during the invalidation
phase so we just match the current code by forcing the loading to happen here.
On 2014/10/31 at 14:03:16, dsinclair wrote:
> Can we add a crbug for this so we don't lose track?

https://code.google.com/p/chromium/issues/detail?id=429346

[pdr., 2014-11-01 05:16:02]

The disabler + comment and bug look great to me. I'll have to defer to dsinclair
and wangxianzhu on the rest of this patch.

[Xianzhu, 2014-11-03 17:03:36]

lgtm (with a nit and a question).

https://codereview.chromium.org/665673004/diff/40001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/665673004/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:384: Bug(jchaffraix)
fast/repaint/caret-invalidation-in-overflow-scroll.html [ NeedsRebaseline ]
Nit: crbug.com/407416

https://codereview.chromium.org/665673004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderSelectionInfo.h (left):

https://codereview.chromium.org/665673004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderSelectionInfo.h:58: class RenderSelectionInfo final
: public RenderSelectionInfoBase {
Can we combine RenderSelectionInfoBase into RenderSelectionInfo?

[Julien - ping for review, 2014-11-03 17:29:21]

Thanks for the review!

https://codereview.chromium.org/665673004/diff/40001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/665673004/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:384: Bug(jchaffraix)
fast/repaint/caret-invalidation-in-overflow-scroll.html [ NeedsRebaseline ]
On 2014/11/03 17:03:35, Xianzhu wrote:
> Nit: crbug.com/407416

Done.

https://codereview.chromium.org/665673004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderSelectionInfo.h (left):

https://codereview.chromium.org/665673004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderSelectionInfo.h:58: class RenderSelectionInfo final
: public RenderSelectionInfoBase {
On 2014/11/03 17:03:36, Xianzhu wrote:
> Can we combine RenderSelectionInfoBase into RenderSelectionInfo?

Yes, however I didn't want to burden this patch with this refactoring. Also I
would like to get rid of these classes altogether first instead of just changing
the class hierarchy.

[Julien - ping for review, 2014-11-03 17:30:12]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-11-03 17:31:17]

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

[commit-bot: I haz the power, 2014-11-03 18:51:49]

Committed patchset #4 (id:60001) as 184794