Issue 834173003: Change about caret painting in slimming paint mode

Issue 834173003: Change about caret painting in slimming paint mode (Closed)

Created:
5 years, 11 months ago by Xianzhu

Modified:
5 years, 11 months ago

Reviewers:
chrishtr, pdr.

CC:
blink-reviews, blink-reviews-paint_chromium.org, blink-reviews-rendering, Rik, danakj, Dominik Röttsches, krit, eae+blinkwatch, f(malita), jbroman, jchaffraix+rendering, leviw+renderwatch, pdr+graphicswatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Change about caret painting in slimming paint mode - To avoid DisplayItemList from being confused by caret display items originally with PaintPhaseForeground phase, add PaintPhaseCaret for carets; - Issue display item for caret only when the object has caret. This avoids too many dummy display items for carets for blocks that don't have carets; (Originally we also output dummy display items for carets in different order when an object's layer status changes but there is no invalidation. This change also avoid this issue.) - Remove ASSERT(displayList.size()) because we may result empty display list if nothing needs painting. BUG=444163, 441108 TEST=fast/text/caret-crash.html TEST=fast/layers/layer-status-change-crash.html TEST=ViewDisplayListTest.FullDocumentPaintingWithCaret Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187933

Patch Set 1 #

Total comments: 2

Patch Set 2 : Add tests #

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+90 lines, -3 lines)			Patch
A	LayoutTests/fast/layers/layer-status-change-crash.html	View	1	1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/layers/layer-status-change-crash-expected.txt	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A	LayoutTests/fast/text/caret-crash.html	View	1	1 chunk	+21 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/text/caret-crash-expected.txt	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/paint/BlockPainter.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/paint/ViewDisplayListTest.cpp	View	1	3 chunks	+39 lines, -0 lines	0 comments	Download
M	Source/core/rendering/PaintInfo.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/PaintPhase.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/platform/graphics/ContentLayerDelegate.cpp	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/platform/graphics/paint/DisplayItem.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/platform/graphics/paint/DisplayItem.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 17 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

pdr.

I think this is a good approach. I had a testcase for this bug in ...

5 years, 11 months ago (2015-01-05 23:02:19 UTC) #3

pdr.

On 2015/01/05 at 23:02:19, pdr wrote: > I think this is a good approach. > ...

5 years, 11 months ago (2015-01-05 23:03:04 UTC) #4

chrishtr

What is the exact scenario here? Carets are painted but the render object is not ...

5 years, 11 months ago (2015-01-05 23:24:39 UTC) #5

Xianzhu

On 2015/01/05 23:24:39, chrishtr wrote: > What is the exact scenario here? Carets are painted ...

5 years, 11 months ago (2015-01-05 23:50:27 UTC) #6

chrishtr

On 2015/01/05 at 23:50:27, wangxianzhu wrote: > On 2015/01/05 23:24:39, chrishtr wrote: > > What ...

5 years, 11 months ago (2015-01-06 01:20:09 UTC) #7

Xianzhu

On 2015/01/06 01:20:09, chrishtr wrote: > On 2015/01/05 at 23:50:27, wangxianzhu wrote: > > On ...

5 years, 11 months ago (2015-01-06 01:30:08 UTC) #8

On 2015/01/06 01:20:09, chrishtr wrote:
> On 2015/01/05 at 23:50:27, wangxianzhu wrote:
> > On 2015/01/05 23:24:39, chrishtr wrote:
> > > What is the exact scenario here? Carets are painted but the render object
is
> not
> > > invalidated? It's invalidated only by rectangle and not by object?
> > 
> > For now I haven't observed invalidation issue for carets. (I had believed
that
> which was finally proved not.)
> > 
> > This patch resolves the issue of reordering of dummy display items
(introduced
> in https://codereview.chromium.org/794323004) for carets in the following
> scenario:
> > 
> > 1. <HTML><BODY></BODY></HTML>
> >   display list:
> >     nth: dummy display item for the non-existent caret of HTML,
> PaintPhaseForeground
> >     (n+1)th: dummy display item for the non-existent caret of BODY,
> PaintPhaseForeground
> > 
> > 2. When BODY becomes a layer (without invalidation because both BODY and
HTML
> are empty), the dummy display items for the non-existent carets of HTML/BODY
are
> inverted because we call paintCaret for normal children and sublayers in
> different orders.
> > 
> > By not issuing dummy display items for non-existent carets, the recordering
> issue can be avoided.
> > 
> > I think we should also revolve the paintCaret different ordering issue for
> normal children and sublayers, but would like to do it in another change.
> 
> But if there is no invalidation, why is it ok to emit a new display item of
any
> kind?
> I don't yet understand why in general dummy display items are different than
> caret ones with their own phase.

Sorry, I missed the point: there should be invalidation (for some really changed
node under BODY), but unrelated to HTML and BODY. The complete case should be:
<HTML><BODY><DIV></DIV></BODY></HTML>
the body becomes a layer and DIV is invalidated.

During the second paint, the dummy display items for non-existence caret of HTML
and BODY are inverted.

Letting caret be with their own phase is to avoid possible confusion about
multiple display items (e.g. one for real foreground, one for caret) with the
same id (which seems not a big deal), and to make debug output more
understandable.

Xianzhu

About original number of dummy display items for non-existence carets: we have one for each ...

5 years, 11 months ago (2015-01-06 17:23:52 UTC) #9

chrishtr

On 2015/01/06 at 01:30:08, wangxianzhu wrote: > On 2015/01/06 01:20:09, chrishtr wrote: > > On ...

5 years, 11 months ago (2015-01-06 17:29:33 UTC) #10

On 2015/01/06 at 01:30:08, wangxianzhu wrote:
> On 2015/01/06 01:20:09, chrishtr wrote:
> > On 2015/01/05 at 23:50:27, wangxianzhu wrote:
> > > On 2015/01/05 23:24:39, chrishtr wrote:
> > > > What is the exact scenario here? Carets are painted but the render
object is
> > not
> > > > invalidated? It's invalidated only by rectangle and not by object?
> > > 
> > > For now I haven't observed invalidation issue for carets. (I had believed
that
> > which was finally proved not.)
> > > 
> > > This patch resolves the issue of reordering of dummy display items
(introduced
> > in https://codereview.chromium.org/794323004) for carets in the following
> > scenario:
> > > 
> > > 1. <HTML><BODY></BODY></HTML>
> > >   display list:
> > >     nth: dummy display item for the non-existent caret of HTML,
> > PaintPhaseForeground
> > >     (n+1)th: dummy display item for the non-existent caret of BODY,
> > PaintPhaseForeground
> > > 
> > > 2. When BODY becomes a layer (without invalidation because both BODY and
HTML
> > are empty), the dummy display items for the non-existent carets of HTML/BODY
are
> > inverted because we call paintCaret for normal children and sublayers in
> > different orders.
> > > 
> > > By not issuing dummy display items for non-existent carets, the
recordering
> > issue can be avoided.
> > > 
> > > I think we should also revolve the paintCaret different ordering issue for
> > normal children and sublayers, but would like to do it in another change.
> > 
> > But if there is no invalidation, why is it ok to emit a new display item of
any
> > kind?
> > I don't yet understand why in general dummy display items are different than
> > caret ones with their own phase.
> 
> Sorry, I missed the point: there should be invalidation (for some really
changed node under BODY), but unrelated to HTML and BODY. The complete case
should be:
> <HTML><BODY><DIV></DIV></BODY></HTML>
> the body becomes a layer and DIV is invalidated.
> 
> During the second paint, the dummy display items for non-existence caret of
HTML and BODY are inverted.
> 
> Letting caret be with their own phase is to avoid possible confusion about
multiple display items (e.g. one for real foreground, one for caret) with the
same id (which seems not a big deal), and to make debug output more
understandable.

I still don't understand. Before this CL, it sounds like the display items were:

Frame 1:
  [HTML dummy for caret, with id of foreground phase]
  [BODY dummy for caret, with id of foreground phase]
Frame 2:
  [BODY dummy for caret, with id of foreground phase]
  [HTML dummy for caret, with id of foreground phase]


Now it's:

Frame 1:
  [HTML dummy for caret, with id of caret phase]
  [BODY dummy for caret, with id of caret phase]
Frame 2:
  [BODY dummy for caret, with id of caret phase]
  [HTML dummy for caret, with id of caret phase]

Is that correct? If so, I don't get what exactly this buys you.

Also, two display items with the same id is not good, how else can we
distinguish mutating one of them?

I do agree that carets should be in their own paint phase, just trying to
understand the other issues.

Xianzhu

On 2015/01/06 17:29:33, chrishtr wrote: > On 2015/01/06 at 01:30:08, wangxianzhu wrote: > > On ...

5 years, 11 months ago (2015-01-06 17:34:22 UTC) #11

On 2015/01/06 17:29:33, chrishtr wrote:
> On 2015/01/06 at 01:30:08, wangxianzhu wrote:
> > On 2015/01/06 01:20:09, chrishtr wrote:
> > > On 2015/01/05 at 23:50:27, wangxianzhu wrote:
> > > > On 2015/01/05 23:24:39, chrishtr wrote:
> > > > > What is the exact scenario here? Carets are painted but the render
> object is
> > > not
> > > > > invalidated? It's invalidated only by rectangle and not by object?
> > > > 
> > > > For now I haven't observed invalidation issue for carets. (I had
believed
> that
> > > which was finally proved not.)
> > > > 
> > > > This patch resolves the issue of reordering of dummy display items
> (introduced
> > > in https://codereview.chromium.org/794323004) for carets in the following
> > > scenario:
> > > > 
> > > > 1. <HTML><BODY></BODY></HTML>
> > > >   display list:
> > > >     nth: dummy display item for the non-existent caret of HTML,
> > > PaintPhaseForeground
> > > >     (n+1)th: dummy display item for the non-existent caret of BODY,
> > > PaintPhaseForeground
> > > > 
> > > > 2. When BODY becomes a layer (without invalidation because both BODY and
> HTML
> > > are empty), the dummy display items for the non-existent carets of
HTML/BODY
> are
> > > inverted because we call paintCaret for normal children and sublayers in
> > > different orders.
> > > > 
> > > > By not issuing dummy display items for non-existent carets, the
> recordering
> > > issue can be avoided.
> > > > 
> > > > I think we should also revolve the paintCaret different ordering issue
for
> > > normal children and sublayers, but would like to do it in another change.
> > > 
> > > But if there is no invalidation, why is it ok to emit a new display item
of
> any
> > > kind?
> > > I don't yet understand why in general dummy display items are different
than
> > > caret ones with their own phase.
> > 
> > Sorry, I missed the point: there should be invalidation (for some really
> changed node under BODY), but unrelated to HTML and BODY. The complete case
> should be:
> > <HTML><BODY><DIV></DIV></BODY></HTML>
> > the body becomes a layer and DIV is invalidated.
> > 
> > During the second paint, the dummy display items for non-existence caret of
> HTML and BODY are inverted.
> > 
> > Letting caret be with their own phase is to avoid possible confusion about
> multiple display items (e.g. one for real foreground, one for caret) with the
> same id (which seems not a big deal), and to make debug output more
> understandable.
> 
> I still don't understand. Before this CL, it sounds like the display items
were:
> 
> Frame 1:
>   [HTML dummy for caret, with id of foreground phase]
>   [BODY dummy for caret, with id of foreground phase]
> Frame 2:
>   [BODY dummy for caret, with id of foreground phase]
>   [HTML dummy for caret, with id of foreground phase]
> 
> 
> Now it's:
> 
> Frame 1:
>   [HTML dummy for caret, with id of caret phase]
>   [BODY dummy for caret, with id of caret phase]
> Frame 2:
>   [BODY dummy for caret, with id of caret phase]
>   [HTML dummy for caret, with id of caret phase]
> 
> Is that correct? If so, I don't get what exactly this buys you.
> 
> Also, two display items with the same id is not good, how else can we
> distinguish mutating one of them?
> 
> I do agree that carets should be in their own paint phase, just trying to
> understand the other issues.

With this change, we no longer issue dummy display items for carets :)
(See change at line 230 of
https://codereview.chromium.org/834173003/diff/1/Source/core/paint/BlockPaint...)

chrishtr

On 2015/01/06 at 17:34:22, wangxianzhu wrote: > On 2015/01/06 17:29:33, chrishtr wrote: > > On ...

5 years, 11 months ago (2015-01-06 17:37:17 UTC) #12

On 2015/01/06 at 17:34:22, wangxianzhu wrote:
> On 2015/01/06 17:29:33, chrishtr wrote:
> > On 2015/01/06 at 01:30:08, wangxianzhu wrote:
> > > On 2015/01/06 01:20:09, chrishtr wrote:
> > > > On 2015/01/05 at 23:50:27, wangxianzhu wrote:
> > > > > On 2015/01/05 23:24:39, chrishtr wrote:
> > > > > > What is the exact scenario here? Carets are painted but the render
> > object is
> > > > not
> > > > > > invalidated? It's invalidated only by rectangle and not by object?
> > > > > 
> > > > > For now I haven't observed invalidation issue for carets. (I had
believed
> > that
> > > > which was finally proved not.)
> > > > > 
> > > > > This patch resolves the issue of reordering of dummy display items
> > (introduced
> > > > in https://codereview.chromium.org/794323004) for carets in the
following
> > > > scenario:
> > > > > 
> > > > > 1. <HTML><BODY></BODY></HTML>
> > > > >   display list:
> > > > >     nth: dummy display item for the non-existent caret of HTML,
> > > > PaintPhaseForeground
> > > > >     (n+1)th: dummy display item for the non-existent caret of BODY,
> > > > PaintPhaseForeground
> > > > > 
> > > > > 2. When BODY becomes a layer (without invalidation because both BODY
and
> > HTML
> > > > are empty), the dummy display items for the non-existent carets of
HTML/BODY
> > are
> > > > inverted because we call paintCaret for normal children and sublayers in
> > > > different orders.
> > > > > 
> > > > > By not issuing dummy display items for non-existent carets, the
> > recordering
> > > > issue can be avoided.
> > > > > 
> > > > > I think we should also revolve the paintCaret different ordering issue
for
> > > > normal children and sublayers, but would like to do it in another
change.
> > > > 
> > > > But if there is no invalidation, why is it ok to emit a new display item
of
> > any
> > > > kind?
> > > > I don't yet understand why in general dummy display items are different
than
> > > > caret ones with their own phase.
> > > 
> > > Sorry, I missed the point: there should be invalidation (for some really
> > changed node under BODY), but unrelated to HTML and BODY. The complete case
> > should be:
> > > <HTML><BODY><DIV></DIV></BODY></HTML>
> > > the body becomes a layer and DIV is invalidated.
> > > 
> > > During the second paint, the dummy display items for non-existence caret
of
> > HTML and BODY are inverted.
> > > 
> > > Letting caret be with their own phase is to avoid possible confusion about
> > multiple display items (e.g. one for real foreground, one for caret) with
the
> > same id (which seems not a big deal), and to make debug output more
> > understandable.
> > 
> > I still don't understand. Before this CL, it sounds like the display items
were:
> > 
> > Frame 1:
> >   [HTML dummy for caret, with id of foreground phase]
> >   [BODY dummy for caret, with id of foreground phase]
> > Frame 2:
> >   [BODY dummy for caret, with id of foreground phase]
> >   [HTML dummy for caret, with id of foreground phase]
> > 
> > 
> > Now it's:
> > 
> > Frame 1:
> >   [HTML dummy for caret, with id of caret phase]
> >   [BODY dummy for caret, with id of caret phase]
> > Frame 2:
> >   [BODY dummy for caret, with id of caret phase]
> >   [HTML dummy for caret, with id of caret phase]
> > 
> > Is that correct? If so, I don't get what exactly this buys you.
> > 
> > Also, two display items with the same id is not good, how else can we
> > distinguish mutating one of them?
> > 
> > I do agree that carets should be in their own paint phase, just trying to
> > understand the other issues.
> 
> With this change, we no longer issue dummy display items for carets :)
> (See change at line 230 of
https://codereview.chromium.org/834173003/diff/1/Source/core/paint/BlockPaint...)

Ah I see. We don't issue caret display items without hasCaret() going on or off,
which only happens if the containing RenderObject is invalidated.

commit-bot: I haz the power

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

5 years, 11 months ago (2015-01-06 19:26:47 UTC) #16

commit-bot: I haz the power

5 years, 11 months ago (2015-01-06 20:47:39 UTC) #17

Message was sent while issue was closed.

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187933

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages