Move Image ownership out of CSSValue

Move Image ownership out of CSSValue

Move Image ownership out of CSSValue, keeping CSSValues as immutable
objects. Created a class CSStyleImageMap which is stored on
ElementStyleResources and ComputedStyle (although ComputedStyle needs to
stay small, so this isn't a good place for it).

Still a WIP. DNL.

[sashab, 2015-09-03 03:46:09]

sashab@chromium.org changed reviewers:
+ timloh@chromium.org

[sashab, 2015-09-03 03:46:10]

Quick question, no need to look at this patch in detail -- where do you think is
a good place to own images referred to by CSS? I've got ElementStyleResources as
the owner for during style resolve, but after that I'm not sure. Should I store
them on ComputedStyle? StylePropertySet? Document? etc.

[esprehn, 2015-09-03 04:29:36]

On 2015/09/03 at 03:46:10, sashab wrote:
> Quick question, no need to look at this patch in detail -- where do you think
is a good place to own images referred to by CSS? I've got ElementStyleResources
as the owner for during style resolve, but after that I'm not sure. Should I
store them on ComputedStyle? StylePropertySet? Document? etc.

I don't think we can do all these hash table lookups... while I do like the
purity of making them immutable doing a table lookup for every image doesn't
seem like a good idea.

[esprehn, 2015-09-03 04:31:21]

On 2015/09/03 at 04:29:36, esprehn wrote:
> On 2015/09/03 at 03:46:10, sashab wrote:
> > Quick question, no need to look at this patch in detail -- where do you
think is a good place to own images referred to by CSS? I've got
ElementStyleResources as the owner for during style resolve, but after that I'm
not sure. Should I store them on ComputedStyle? StylePropertySet? Document? etc.
> 
> I don't think we can do all these hash table lookups... while I do like the
purity of making them immutable doing a table lookup for every image doesn't
seem like a good idea.

So to answer your question, the StyleImage needs to own the image resource, and
the ComputedStyle (specifically the FillLayers probably) should have a RefPtr to
them.

[sashab, 2015-09-03 07:21:05]

On 2015/09/03 04:31:21, esprehn wrote:
> On 2015/09/03 at 04:29:36, esprehn wrote:
> > On 2015/09/03 at 03:46:10, sashab wrote:
> > > Quick question, no need to look at this patch in detail -- where do you
> think is a good place to own images referred to by CSS? I've got
> ElementStyleResources as the owner for during style resolve, but after that
I'm
> not sure. Should I store them on ComputedStyle? StylePropertySet? Document?
etc.
> > 
> > I don't think we can do all these hash table lookups... while I do like the
> purity of making them immutable doing a table lookup for every image doesn't
> seem like a good idea.
> 
> So to answer your question, the StyleImage needs to own the image resource,
and
> the ComputedStyle (specifically the FillLayers probably) should have a RefPtr
to
> them.

Thanks; OK, I'll have a look at FillLayers.

I agree with the hash table lookups being slow. Another idea is to create the
CSSValues with a reference to the image, stored in a list on
ElementStyleResources and then in FillLayers or wherever. This would be similar
to the CSSValues owning a copy of the image, except that the CSSValues can be
created/destroyed safely while the image can have their ownership transferred.

I.e.:

class CSSImageValue {
public:
  create(StyleImageEntry&) { ... }
  StyleImage* cachedOrPendingImage() const
  {
     ... logic ...
     image.value = PendingImage::create() // can still be const because this
doesn't count as modifying CSSImageValue
     return image.value;
  }
  ~CSSImageValue() { image.parent->delete(this); }

private:
  StyleImageEntry& image;
}

Some places would still have to remove & re-create the image completely, e.g.
see the CL changes in Element.cpp when the image URL is re-resolved.

The only problem is if they're created in the parser etc where the
StyleImageEntry isn't available yet... Maybe we'd have to plumb
ElementStyleResources through so we can make space for it.