Emit dummy display item when recorded picture is empty

Emit dummy display item when recorded picture is empty

Previously we don't emit display item when DrawingRecorder recorded
an empty picture. This causes trouble when we merge the new paint list
containing CachedDisplayItems: when we emit CachedDisplayItem, we don't
know if the last recorded picture is empty; during merging, if we can't
find the original display item correponding the CachedDisplayItem,
assert will fail in findNextMatchingCachedItem.

We could let findNextMatchingCachedItem return m_paintList.end and
discard the CachedDisplayItem, but that would cause O(n^2) complexity
in worst case.

Emit dummy display item when recorded picture is empty to avoid the
problem.

Add DisplayItem::name() for testing and debugging, especially for dummy
display items.

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

P.S.
BUG=441108

[Xianzhu, 2014-12-12 18:05:49]

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

[Xianzhu, 2014-12-12 18:05:49]

Actually this is the real root reason of the caret issue.

[pdr., 2014-12-12 18:39:04]

I think this is a reasonable approach overall.

Instead of creating a new dummy type, why don't we just add support for a
deleted picture reference in DrawingDisplayItem? I think this would be simpler
and a bit cleaner since we won't need to support a stubbed-out version of
DrawingDisplayItem's functions.

https://codereview.chromium.org/794323004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItem.cpp (left):

https://codereview.chromium.org/794323004/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItem.cpp:82: if (isCached())
Any reason for removing this?

https://codereview.chromium.org/794323004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItem.cpp (right):

https://codereview.chromium.org/794323004/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItem.cpp:10: namespace {
Does this need to be in a namespace block?

[Stephen Chennney, 2014-12-12 18:40:12]

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

[Stephen Chennney, 2014-12-12 18:40:13]

Or, alternatively, just implement the virtuals on DisplayItem.

[Xianzhu, 2014-12-12 19:11:17]

Patch Set 3 takes the way that Stephen suggested.

There seem some pros not to create a DrawingDisplayItem with a null SkPicture:
- no need of memory for a null pointer;
- no new condition checks in DrawingDisplayItem.

https://codereview.chromium.org/794323004/diff/20001/Source/platform/graphics...
File Source/platform/graphics/paint/DisplayItem.cpp (left):

https://codereview.chromium.org/794323004/diff/20001/Source/platform/graphics...
Source/platform/graphics/paint/DisplayItem.cpp:82: if (isCached())
On 2014/12/12 18:39:04, pdr wrote:
> Any reason for removing this?

Now the new "name" attribute can show the same information, so this becomes
redundant.

[pdr., 2014-12-12 19:49:34]

LGTM

[Xianzhu, 2014-12-12 19:50:05]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-12-12 19:50:41]

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

[commit-bot: I haz the power, 2014-12-12 22:08:32]

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

[commit-bot: I haz the power, 2014-12-12 22:08:33]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41346)

[Xianzhu, 2014-12-12 22:22:22]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-12-12 22:22:53]

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

[commit-bot: I haz the power, 2014-12-13 00:37:25]

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

[commit-bot: I haz the power, 2014-12-13 00:37:26]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/41373)

[Xianzhu, 2014-12-13 01:25:28]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-12-13 01:26:46]

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

[commit-bot: I haz the power, 2014-12-13 07:30:21]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187088

[chrishtr, 2015-01-05 22:12:07]

Is there a bug that should be associated with this issue?

[Xianzhu, 2015-01-05 22:20:56]

On 2015/01/05 22:12:07, chrishtr wrote:
> Is there a bug that should be associated with this issue?

Sorry, I should have referenced crbug.com/441108.

Just noticed we still have an issue about painting carets (crbug.com/444163).
Working on it.