Show debug information for invalid display items

Show debug information for invalid display items

This may be helpful when we call showDebugData() during the merge
algorithm.

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

[Xianzhu, 2015-09-03 01:45:40]

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

[pdr., 2015-09-03 01:59:20]

LGTM

https://codereview.chromium.org/1323263002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItem.cpp (right):

https://codereview.chromium.org/1323263002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItem.cpp:217:
stringBuilder.append(clientDebugString());
Can you add quotes around this so it's valid-ish json?

[Xianzhu, 2015-09-03 05:48:44]

https://codereview.chromium.org/1323263002/diff/1/Source/platform/graphics/pa...
File Source/platform/graphics/paint/DisplayItem.cpp (right):

https://codereview.chromium.org/1323263002/diff/1/Source/platform/graphics/pa...
Source/platform/graphics/paint/DisplayItem.cpp:217:
stringBuilder.append(clientDebugString());
On 2015/09/03 01:59:20, pdr wrote:
> Can you add quotes around this so it's valid-ish json?

It's the original display item's debug string which is already in json format.
Will add a comment here.

[Xianzhu, 2015-09-03 05:48:47]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-09-03 05:48:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1323263002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-09-03 05:48:51]

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

[commit-bot: I haz the power, 2015-09-03 06:00:35]

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

[commit-bot: I haz the power, 2015-09-03 06:00:36]

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

[Xianzhu, 2015-09-04 16:24:51]

Changed a bit to avoid ContiguousContainer's dependency on constructor
prototype. Ptal.

[pdr., 2015-09-04 17:01:39]

On 2015/09/04 at 16:24:51, wangxianzhu wrote:
> Changed a bit to avoid ContiguousContainer's dependency on constructor
prototype. Ptal.

What caused you to make this change? Noticing that "item" in
DisplayItems::appendByMoving is the old item isn't completely obvious.

[Xianzhu, 2015-09-04 17:18:50]

On 2015/09/04 17:01:39, pdr wrote:
> On 2015/09/04 at 16:24:51, wangxianzhu wrote:
> > Changed a bit to avoid ContiguousContainer's dependency on constructor
> prototype. Ptal.
> 
> What caused you to make this change? Noticing that "item" in
> DisplayItems::appendByMoving is the old item isn't completely obvious.

In the previous patch set, ContiguousContainer has a requirement that
BaseElementType must have a constructor taking a string parameter in debug mode,
but the requirement is meaningful for DisplayItem only. The latest patch removes
this requirement and put it in DisplayItems.

Will a comment in DisplayItem::appendByMoving about old item make it clear?

[Xianzhu, 2015-09-04 17:19:50]

On 2015/09/04 17:18:50, Xianzhu wrote:
> On 2015/09/04 17:01:39, pdr wrote:
> > On 2015/09/04 at 16:24:51, wangxianzhu wrote:
> > > Changed a bit to avoid ContiguousContainer's dependency on constructor
> > prototype. Ptal.
> > 
> > What caused you to make this change? Noticing that "item" in
> > DisplayItems::appendByMoving is the old item isn't completely obvious.
> 
> In the previous patch set, ContiguousContainer has a requirement that
> BaseElementType must have a constructor taking a string parameter in debug
mode,
> but the requirement is meaningful for DisplayItem only.

P.S. ContiguousContainerTest was broken with the previous patch set.

> The latest patch removes
> this requirement and put it in DisplayItems.
> 
> Will a comment in DisplayItem::appendByMoving about old item make it clear?

[pdr., 2015-09-04 17:20:50]

On 2015/09/04 at 17:19:50, wangxianzhu wrote:
> On 2015/09/04 17:18:50, Xianzhu wrote:
> > On 2015/09/04 17:01:39, pdr wrote:
> > > On 2015/09/04 at 16:24:51, wangxianzhu wrote:
> > > > Changed a bit to avoid ContiguousContainer's dependency on constructor
> > > prototype. Ptal.
> > > 
> > > What caused you to make this change? Noticing that "item" in
> > > DisplayItems::appendByMoving is the old item isn't completely obvious.
> > 
> > In the previous patch set, ContiguousContainer has a requirement that
> > BaseElementType must have a constructor taking a string parameter in debug
mode,
> > but the requirement is meaningful for DisplayItem only.
> 
> P.S. ContiguousContainerTest was broken with the previous patch set.
> 
> > The latest patch removes
> > this requirement and put it in DisplayItems.
> > 
> > Will a comment in DisplayItem::appendByMoving about old item make it clear?

SGTM, yeah please do add a comment in DIs::appendByMoving.

LGTM

[Xianzhu, 2015-09-04 17:24:28]

On 2015/09/04 17:20:50, pdr wrote:
> On 2015/09/04 at 17:19:50, wangxianzhu wrote:
> > On 2015/09/04 17:18:50, Xianzhu wrote:
> > > On 2015/09/04 17:01:39, pdr wrote:
> > > > On 2015/09/04 at 16:24:51, wangxianzhu wrote:
> > > > > Changed a bit to avoid ContiguousContainer's dependency on constructor
> > > > prototype. Ptal.
> > > > 
> > > > What caused you to make this change? Noticing that "item" in
> > > > DisplayItems::appendByMoving is the old item isn't completely obvious.
> > > 
> > > In the previous patch set, ContiguousContainer has a requirement that
> > > BaseElementType must have a constructor taking a string parameter in debug
> mode,
> > > but the requirement is meaningful for DisplayItem only.
> > 
> > P.S. ContiguousContainerTest was broken with the previous patch set.
> > 
> > > The latest patch removes
> > > this requirement and put it in DisplayItems.
> > > 
> > > Will a comment in DisplayItem::appendByMoving about old item make it
clear?
> 
> SGTM, yeah please do add a comment in DIs::appendByMoving.
> 

Done.

> LGTM

[Xianzhu, 2015-09-04 17:30:00]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-09-04 17:30:01]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1323263002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-09-04 17:30:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323263002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323263002/80001

[commit-bot: I haz the power, 2015-09-04 17:40:21]

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

[commit-bot: I haz the power, 2015-09-04 17:40:22]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[Xianzhu, 2015-09-04 18:32:25]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-09-04 18:32:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323263002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323263002/80001

[commit-bot: I haz the power, 2015-09-04 19:48:45]

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