When style changes, set the LayoutObject as a client for all StyleImage loads.

When style changes, set the LayoutObject as a client for all StyleImage loads.

BUG=

[Nate Chapin, 2015-09-11 21:35:50]

japhet@chromium.org changed reviewers:
+ esprehn@chromium.org

[Nate Chapin, 2015-09-11 21:35:51]

I got looking at this for https://codereview.chromium.org/1304063016/, which
allows for a cleaner and more general way to modify a network request's
priority.

That way of modifying priorities requires LayoutObjects to be registered as
clients of the images in their ComputedStyle so that the images can accurately
determine whether or not they're in the viewport.

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

https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1853: updateImage(oldStyle ?
oldStyle->listStyleImage() : nullptr, m_style->listStyleImage());
It appears that this and only this needs to be after styleDidChange() If
updateImage() triggers a memory cache hit for listStyleImage(), we will crash,
because LayoutListMarker::imageChanged() assumes
LayoutListMarker::styleDidChange() has been called first.

[esprehn, 2015-09-12 02:10:44]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-12 02:10:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342503002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342503002/1

[commit-bot: I haz the power, 2015-09-12 05:11:33]

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

[commit-bot: I haz the power, 2015-09-12 05:11:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2015-09-15 20:59:16]

On 2015/09/11 21:35:51, Nate Chapin wrote:
> I got looking at this for https://codereview.chromium.org/1304063016/, which
> allows for a cleaner and more general way to modify a network request's
> priority.
> 
> That way of modifying priorities requires LayoutObjects to be registered as
> clients of the images in their ComputedStyle so that the images can accurately
> determine whether or not they're in the viewport.
> 
>
https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
> Source/core/layout/LayoutObject.cpp:1853: updateImage(oldStyle ?
> oldStyle->listStyleImage() : nullptr, m_style->listStyleImage());
> It appears that this and only this needs to be after styleDidChange() If
> updateImage() triggers a memory cache hit for listStyleImage(), we will crash,
> because LayoutListMarker::imageChanged() assumes
> LayoutListMarker::styleDidChange() has been called first.

Friendly ping.

[esprehn, 2015-09-17 08:20:48]

All this const_cast looks bad, I think you want to make contentData() not const?
I'm not sure why it is...

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

https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1853: updateImage(oldStyle ?
oldStyle->listStyleImage() : nullptr, m_style->listStyleImage());
On 2015/09/11 at 21:35:50, Nate Chapin wrote:
> It appears that this and only this needs to be after styleDidChange() If
updateImage() triggers a memory cache hit for listStyleImage(), we will crash,
because LayoutListMarker::imageChanged() assumes
LayoutListMarker::styleDidChange() has been called first.

That needs a comment, also can we not move them all after or all before? Or fix
LayoutListMarker...

https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2495:
const_cast<StyleImage*>(toImageContentData(m_style->contentData())->image())->removeClient(this);
This const_cast seems sketchy, why is it okay to cast away the const-ness and
mutate from here?  Actually why is the image const at all here?

[Nate Chapin, 2015-09-22 20:24:07]

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

https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1853: updateImage(oldStyle ?
oldStyle->listStyleImage() : nullptr, m_style->listStyleImage());
On 2015/09/17 08:20:48, esprehn wrote:
> On 2015/09/11 at 21:35:50, Nate Chapin wrote:
> > It appears that this and only this needs to be after styleDidChange() If
> updateImage() triggers a memory cache hit for listStyleImage(), we will crash,
> because LayoutListMarker::imageChanged() assumes
> LayoutListMarker::styleDidChange() has been called first.
> 
> That needs a comment, also can we not move them all after or all before? Or
fix
> LayoutListMarker...
> 

Moved this logic to LayoutListItem, since listStyleImage() is only used in
LayoutListItem and LayoutListMarker, and LayoutListMarker already registers
itself.

https://codereview.chromium.org/1342503002/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2495:
const_cast<StyleImage*>(toImageContentData(m_style->contentData())->image())->removeClient(this);
On 2015/09/17 08:20:48, esprehn wrote:
> This const_cast seems sketchy, why is it okay to cast away the const-ness and
> mutate from here?  Actually why is the image const at all here?

I was scared to change it. :)

Made ComputedStyle::contentData() non-const, since that seems benign.