Remove outdated FIXME

Remove outdated FIXME

paintInvalidationContainer pointer of
LayoutObject::invalidatePaintUsingContainer is not null,
so replaced by reference. 
Meanwhile containerForPaintInvalidation will be deprecated,
replaced by containerForPaintInvalidationOnRootedTree.

This is cleanup of https://codereview.chromium.org/247803003.

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

[huayong.xu, 2015-05-28 05:41:20]

huayong.xu@samsung.com changed reviewers:
+ leviw@chromium.org

[huayong.xu, 2015-05-28 05:46:53]

It seems that paintInvalidationContainer pointer
LayoutObject::invalidatePaintUsingContainer is never null,
so it is changed to reference. Is this reasonable?
Please take a look!
Thanks!

[huayong.xu, 2015-05-29 07:08:46]

huayong.xu@samsung.com changed reviewers:
+ jchaffraix@chromium.org

[Julien - ping for review, 2015-05-29 18:19:12]

The change looks reasonable, some comments.

https://codereview.chromium.org/1161913002/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1161913002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1156: "data",
InspectorPaintInvalidationTrackingEvent::data(this,
&paintInvalidationContainer));
It seems like we should change this function to use a reference too.

https://codereview.chromium.org/1161913002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1233: const LayoutBoxModelObject&
paintInvalidationContainer = *containerForPaintInvalidation();
I think we should still check that the returned pointer is not NULL (it should
never be NULL because we check for isRooted() above).

The best would be to add a new containerForPaintInvalidationOnRootedTree that
does the following:
- ASSERT(isRooted());
- Call containerForPaintInvalidation()
- Check that the returned result is not NULL.
- Return a reference to it.

That would ensure that we catch bad assumptions in the code. Hopefully we could
convert every callers to containerForPaintInvalidationOnRootedTree and fold it
into containerForPaintInvalidation but that's a stretch goal.

[huayong.xu, 2015-06-01 02:42:39]

@jchaffraix, please take a look again.
Thanks!

https://codereview.chromium.org/1161913002/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1161913002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1156: "data",
InspectorPaintInvalidationTrackingEvent::data(this,
&paintInvalidationContainer));
On 2015/05/29 18:19:12, Julien Chaffraix - PST wrote:
> It seems like we should change this function to use a reference too.

Done.

https://codereview.chromium.org/1161913002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1233: const LayoutBoxModelObject&
paintInvalidationContainer = *containerForPaintInvalidation();
On 2015/05/29 18:19:12, Julien Chaffraix - PST wrote:
> I think we should still check that the returned pointer is not NULL (it should
> never be NULL because we check for isRooted() above).
> 
> The best would be to add a new containerForPaintInvalidationOnRootedTree that
> does the following:
> - ASSERT(isRooted());
> - Call containerForPaintInvalidation()
> - Check that the returned result is not NULL.
> - Return a reference to it.
> 
> That would ensure that we catch bad assumptions in the code. Hopefully we
could
> convert every callers to containerForPaintInvalidationOnRootedTree and fold it
> into containerForPaintInvalidation but that's a stretch goal.

Done.

[Julien - ping for review, 2015-06-03 21:10:13]

lgtm!

https://codereview.chromium.org/1161913002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (left):

https://codereview.chromium.org/1161913002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:864: // FIXME: |paintInvalidationContainer|
should never be 0. See crbug.com/363699.
Great that you're updating this comment!

The long-term goal is for this function to die, replaced by
containerForPaintInvalidationOnRootedTree() and it would be nice to put a
comment along these lines.

[huayong.xu, 2015-06-04 02:20:17]

On 2015/06/03 21:10:13, Julien Chaffraix - PST wrote:
> lgtm!
> 
>
https://codereview.chromium.org/1161913002/diff/20001/Source/core/layout/Layo...
> File Source/core/layout/LayoutObject.h (left):
> 
>
https://codereview.chromium.org/1161913002/diff/20001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.h:864: // FIXME: |paintInvalidationContainer|
> should never be 0. See crbug.com/363699.
> Great that you're updating this comment!
> 
> The long-term goal is for this function to die, replaced by
> containerForPaintInvalidationOnRootedTree() and it would be nice to put a
> comment along these lines.

Done.

A comment is put near the method containerForPaintInvalidation.

[huayong.xu, 2015-06-04 02:20:41]

The CQ bit was checked by huayong.xu@samsung.com

[huayong.xu, 2015-06-04 02:20:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1161913002/#ps40001
(title: "add comments")

[commit-bot: I haz the power, 2015-06-04 02:21:13]

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

[commit-bot: I haz the power, 2015-06-04 06:06:45]

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

[commit-bot: I haz the power, 2015-06-04 06:06:47]

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

[huayong.xu, 2015-06-04 06:42:12]

The CQ bit was checked by huayong.xu@samsung.com

[commit-bot: I haz the power, 2015-06-04 06:42:37]

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

[commit-bot: I haz the power, 2015-06-04 07:41:55]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196476

[huayong.xu, 2015-06-26 10:47:05]

huayong.xu@samsung.com changed reviewers:
+ dsinclair@chromium.org

[huayong.xu, 2015-06-26 10:47:06]

@dsinclair, please take a look. Thanks!
Does this patch cause the crash
https://code.google.com/p/chromium/issues/detail?id=504688 ?
But in this patch, I did not change the behavior of code.
I just changed from the pointer to the reference in
LayoutObjectChildList.cpp and LayoutView.cpp.
Before my patch, *paintInvalidationContainer is not checked
either.

[dsinclair, 2015-06-26 13:44:03]

I don't think it's related to your CL. I've assigned to schenney@ due to a
bunch of the crashes happening inside DisplayListClient code.

dan



On Fri, Jun 26, 2015 at 6:47 AM <huayong.xu@samsung.com> wrote:

> @dsinclair, please take a look. Thanks!
> Does this patch cause the crash
> https://code.google.com/p/chromium/issues/detail?id=504688 ?
> But in this patch, I did not change the behavior of code.
> I just changed from the pointer to the reference in
> LayoutObjectChildList.cpp and LayoutView.cpp.
> Before my patch, *paintInvalidationContainer is not checked
> either.
>
> https://codereview.chromium.org/1161913002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.