Make ImportedStyleSheetClient GarbageCollected

Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663
Committed: https://crrev.com/bd681d849993450a8b55cd31b1b412fd1f9c4ef8
Cr-Commit-Position: refs/heads/master@{#397160}

[yhirano, 2016-05-20 08:23:22]

yhirano@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, timloh@chromium.org

[yhirano, 2016-05-20 08:23:23]

My understanding is that in order to make StyleSheetResourceClient
GarbageCollectedMixin ImportedStyleSheetClient must stop being a part class. Is
that right? > oilpan-reviews@

timloh@, is this change expected to cause measurable performance regression? I'm
not sure why this class is a part class now.

[haraken, 2016-05-20 09:21:10]

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

[haraken, 2016-05-20 09:21:10]

LGTM

https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleRuleImport.cpp (right):

https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleRuleImport.cpp:59:
m_resource->removeClient(m_styleSheetClient);

Can you move this to a pre-finalizer of ImportedStyleSheetClient? (Then you can
remove a pre-finalizer from StyleRuleImport.)

The model is to let all resource clients remove itself from the owner resource
in their pre-finalizers (If the client inherits from ResourceOwner, the client
doesn't need to have a pre-finalizer).

[yhirano, 2016-05-23 11:29:05]

https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleRuleImport.cpp (right):

https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleRuleImport.cpp:59:
m_resource->removeClient(m_styleSheetClient);
On 2016/05/20 09:21:10, haraken wrote:
> 
> Can you move this to a pre-finalizer of ImportedStyleSheetClient? (Then you
can
> remove a pre-finalizer from StyleRuleImport.)
> 
> The model is to let all resource clients remove itself from the owner resource
> in their pre-finalizers (If the client inherits from ResourceOwner, the client
> doesn't need to have a pre-finalizer).

In this case StyleRuleImport has the resource and calls |addClient|, so it looks
natural to me that StyleRuleImport is responsible for removing the client from
the resource. It is possible to access the resource from the client
(m_owner->m_resource), but is it better than the current code?

[haraken, 2016-05-23 23:03:01]

On 2016/05/23 11:29:05, yhirano wrote:
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/StyleRuleImport.cpp (right):
> 
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/StyleRuleImport.cpp:59:
> m_resource->removeClient(m_styleSheetClient);
> On 2016/05/20 09:21:10, haraken wrote:
> > 
> > Can you move this to a pre-finalizer of ImportedStyleSheetClient? (Then you
> can
> > remove a pre-finalizer from StyleRuleImport.)
> > 
> > The model is to let all resource clients remove itself from the owner
resource
> > in their pre-finalizers (If the client inherits from ResourceOwner, the
client
> > doesn't need to have a pre-finalizer).
> 
> In this case StyleRuleImport has the resource and calls |addClient|, so it
looks
> natural to me that StyleRuleImport is responsible for removing the client from
> the resource. It is possible to access the resource from the client
> (m_owner->m_resource), but is it better than the current code?

Maybe would it be better to make the ImportedStyleSheetClient call both
addClient and removeClient?

Ideally:

- ResourceClients should be responsible for adding/removing the clients from the
target resource.
- All ResouceClients should inherit from ResourceOwner (btw, the name
"ResourceOwner" sounds strange to me; it's a client).

What do you think?

[yhirano, 2016-05-24 08:05:52]

Description was changed from

==========
Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663
==========

to

==========
Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663
==========

[yhirano, 2016-05-24 08:05:52]

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

[yhirano, 2016-05-24 08:09:20]

yhirano@chromium.org changed reviewers:
+ hiroshige@chromium.org

[yhirano, 2016-05-24 08:22:45]

On 2016/05/23 23:03:01, haraken wrote:
> On 2016/05/23 11:29:05, yhirano wrote:
> >
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/css/StyleRuleImport.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/css/StyleRuleImport.cpp:59:
> > m_resource->removeClient(m_styleSheetClient);
> > On 2016/05/20 09:21:10, haraken wrote:
> > > 
> > > Can you move this to a pre-finalizer of ImportedStyleSheetClient? (Then
you
> > can
> > > remove a pre-finalizer from StyleRuleImport.)
> > > 
> > > The model is to let all resource clients remove itself from the owner
> resource
> > > in their pre-finalizers (If the client inherits from ResourceOwner, the
> client
> > > doesn't need to have a pre-finalizer).
> > 
> > In this case StyleRuleImport has the resource and calls |addClient|, so it
> looks
> > natural to me that StyleRuleImport is responsible for removing the client
from
> > the resource. It is possible to access the resource from the client
> > (m_owner->m_resource), but is it better than the current code?
> 
> Maybe would it be better to make the ImportedStyleSheetClient call both
> addClient and removeClient?
> 
> Ideally:
> 
> - ResourceClients should be responsible for adding/removing the clients from
the
> target resource.
> - All ResouceClients should inherit from ResourceOwner (btw, the name
> "ResourceOwner" sounds strange to me; it's a client).
> 
> What do you think?

+japhet@ and hiroshige@ for resource owner concept.

My understanding is that "resource owner" (not the class name, but a concept)
means someone that holds a resource and is responsible for calling addClient /
removeClient appropriately. In that sense StyleRuleImport is a resource owner, I
think.

With on-heap ResourceClient and the weak MemoryCache, we won't have to call
removeClient any more. In that world, resource owner will be merely a holder of
a resource and the current StyleRuleImport design will make more sense.

[haraken, 2016-05-26 03:45:35]

On 2016/05/24 08:22:45, yhirano wrote:
> On 2016/05/23 23:03:01, haraken wrote:
> > On 2016/05/23 11:29:05, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/css/StyleRuleImport.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/css/StyleRuleImport.cpp:59:
> > > m_resource->removeClient(m_styleSheetClient);
> > > On 2016/05/20 09:21:10, haraken wrote:
> > > > 
> > > > Can you move this to a pre-finalizer of ImportedStyleSheetClient? (Then
> you
> > > can
> > > > remove a pre-finalizer from StyleRuleImport.)
> > > > 
> > > > The model is to let all resource clients remove itself from the owner
> > resource
> > > > in their pre-finalizers (If the client inherits from ResourceOwner, the
> > client
> > > > doesn't need to have a pre-finalizer).
> > > 
> > > In this case StyleRuleImport has the resource and calls |addClient|, so it
> > looks
> > > natural to me that StyleRuleImport is responsible for removing the client
> from
> > > the resource. It is possible to access the resource from the client
> > > (m_owner->m_resource), but is it better than the current code?
> > 
> > Maybe would it be better to make the ImportedStyleSheetClient call both
> > addClient and removeClient?
> > 
> > Ideally:
> > 
> > - ResourceClients should be responsible for adding/removing the clients from
> the
> > target resource.
> > - All ResouceClients should inherit from ResourceOwner (btw, the name
> > "ResourceOwner" sounds strange to me; it's a client).
> > 
> > What do you think?
> 
> +japhet@ and hiroshige@ for resource owner concept.
> 
> My understanding is that "resource owner" (not the class name, but a concept)
> means someone that holds a resource and is responsible for calling addClient /
> removeClient appropriately. In that sense StyleRuleImport is a resource owner,
I
> think.
> 
> With on-heap ResourceClient and the weak MemoryCache, we won't have to call
> removeClient any more. In that world, resource owner will be merely a holder
of
> a resource and the current StyleRuleImport design will make more sense.

This CL is harmless, so LGTM.

Though I think it would make more sense to remove the concept of "resource
owner" and instead introduce a concept of "resource client". Then all resource
clients inherit from ResourceClient (we can just rename the current
ResourceOwner to ResourceClient). ResourceClient provides setResource() (to
update the resource it observes) and clearResource() (to stop observing the
resource). That way we can centralize the place to add/removeClient into one
place (=ResourceClient), which would be better than spreading out the
add/removeClient to "resource owner"s IMO.

We can remove ResourceClient's pre-finalizer when we make ResourceClient
GarbageCollectedMixin.

[haraken, 2016-05-26 04:00:53]

On 2016/05/26 03:45:35, haraken wrote:
> On 2016/05/24 08:22:45, yhirano wrote:
> > On 2016/05/23 23:03:01, haraken wrote:
> > > On 2016/05/23 11:29:05, yhirano wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/css/StyleRuleImport.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1996293002/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/css/StyleRuleImport.cpp:59:
> > > > m_resource->removeClient(m_styleSheetClient);
> > > > On 2016/05/20 09:21:10, haraken wrote:
> > > > > 
> > > > > Can you move this to a pre-finalizer of ImportedStyleSheetClient?
(Then
> > you
> > > > can
> > > > > remove a pre-finalizer from StyleRuleImport.)
> > > > > 
> > > > > The model is to let all resource clients remove itself from the owner
> > > resource
> > > > > in their pre-finalizers (If the client inherits from ResourceOwner,
the
> > > client
> > > > > doesn't need to have a pre-finalizer).
> > > > 
> > > > In this case StyleRuleImport has the resource and calls |addClient|, so
it
> > > looks
> > > > natural to me that StyleRuleImport is responsible for removing the
client
> > from
> > > > the resource. It is possible to access the resource from the client
> > > > (m_owner->m_resource), but is it better than the current code?
> > > 
> > > Maybe would it be better to make the ImportedStyleSheetClient call both
> > > addClient and removeClient?
> > > 
> > > Ideally:
> > > 
> > > - ResourceClients should be responsible for adding/removing the clients
from
> > the
> > > target resource.
> > > - All ResouceClients should inherit from ResourceOwner (btw, the name
> > > "ResourceOwner" sounds strange to me; it's a client).
> > > 
> > > What do you think?
> > 
> > +japhet@ and hiroshige@ for resource owner concept.
> > 
> > My understanding is that "resource owner" (not the class name, but a
concept)
> > means someone that holds a resource and is responsible for calling addClient
/
> > removeClient appropriately. In that sense StyleRuleImport is a resource
owner,
> I
> > think.
> > 
> > With on-heap ResourceClient and the weak MemoryCache, we won't have to call
> > removeClient any more. In that world, resource owner will be merely a holder
> of
> > a resource and the current StyleRuleImport design will make more sense.
> 
> This CL is harmless, so LGTM.
> 
> Though I think it would make more sense to remove the concept of "resource
> owner" and instead introduce a concept of "resource client". Then all resource
> clients inherit from ResourceClient (we can just rename the current
> ResourceOwner to ResourceClient). ResourceClient provides setResource() (to
> update the resource it observes) and clearResource() (to stop observing the
> resource). That way we can centralize the place to add/removeClient into one
> place (=ResourceClient), which would be better than spreading out the
> add/removeClient to "resource owner"s IMO.
> 
> We can remove ResourceClient's pre-finalizer when we make ResourceClient
> GarbageCollectedMixin.

BTW, here is one question:

If you have:

  class XXXResourceClient : public ResourceOwner<XXXResource, XXXResourceClient>
{ };

then XXXResourceClient holds a strong reference to the XXXResource. In that
sense, it is natural to call the class "ResourceOwner". But on the other hand,
it sounds a bit strange that a "client" holds a strong reference to the
observing resource.

Question: Should the reference really be strong? Or should we change it a weak
reference?

[yhirano, 2016-05-26 10:44:07]

timloh@, do you have any comments?

[yhirano, 2016-05-26 10:53:47]

On 2016/05/26 04:00:53, haraken wrote:
> BTW, here is one question:
> 
> If you have:
> 
>   class XXXResourceClient : public ResourceOwner<XXXResource,
XXXResourceClient>
> { };

There is a circular inheritance because ResourceOwner<R, C> inherits C. Maybe
you mean

class YYYResourceClient : public ResourceOwner<XXXResource, XXXResourceClient> ?

> 
> then XXXResourceClient holds a strong reference to the XXXResource. In that
> sense, it is natural to call the class "ResourceOwner". But on the other hand,
> it sounds a bit strange that a "client" holds a strong reference to the
> observing resource.
> 
> Question: Should the reference really be strong? Or should we change it a weak
> reference?

Some clients have a strong reference to the resource (e.g.,
DocumentThreadableLoader) and it feels fairly natural. I'm not sure why weak
reference is preferred. Can you tell me?

[haraken, 2016-05-26 15:46:00]

On 2016/05/26 10:53:47, yhirano wrote:
> On 2016/05/26 04:00:53, haraken wrote:
> > BTW, here is one question:
> > 
> > If you have:
> > 
> >   class XXXResourceClient : public ResourceOwner<XXXResource,
> XXXResourceClient>
> > { };
> 
> There is a circular inheritance because ResourceOwner<R, C> inherits C. Maybe
> you mean
> 
> class YYYResourceClient : public ResourceOwner<XXXResource, XXXResourceClient>
?
> 
> > 
> > then XXXResourceClient holds a strong reference to the XXXResource. In that
> > sense, it is natural to call the class "ResourceOwner". But on the other
hand,
> > it sounds a bit strange that a "client" holds a strong reference to the
> > observing resource.
> > 
> > Question: Should the reference really be strong? Or should we change it a
weak
> > reference?
> 
> Some clients have a strong reference to the resource (e.g.,
> DocumentThreadableLoader) and it feels fairly natural. I'm not sure why weak
> reference is preferred. Can you tell me?

Sorry for not being clear.

What I'm asking is: conceptually, should the class be "resource owner", or
"resource client"?

As far as I read your comment in #10, it looks like you want to keep the concept
of "resource owner" whereas I'm suggesting replacing the concept with "resource
client" in #11. So I wanted to figure out what the desired design is. If it's a
"resource owner", it would make sense to make the reference strong. If it's a
"resource client", it would make sense to make the reference weak.

[yhirano, 2016-05-30 11:43:33]

On 2016/05/26 15:46:00, haraken wrote:
> On 2016/05/26 10:53:47, yhirano wrote:
> > On 2016/05/26 04:00:53, haraken wrote:
> > > BTW, here is one question:
> > > 
> > > If you have:
> > > 
> > >   class XXXResourceClient : public ResourceOwner<XXXResource,
> > XXXResourceClient>
> > > { };
> > 
> > There is a circular inheritance because ResourceOwner<R, C> inherits C.
Maybe
> > you mean
> > 
> > class YYYResourceClient : public ResourceOwner<XXXResource,
XXXResourceClient>
> ?
> > 
> > > 
> > > then XXXResourceClient holds a strong reference to the XXXResource. In
that
> > > sense, it is natural to call the class "ResourceOwner". But on the other
> hand,
> > > it sounds a bit strange that a "client" holds a strong reference to the
> > > observing resource.
> > > 
> > > Question: Should the reference really be strong? Or should we change it a
> weak
> > > reference?
> > 
> > Some clients have a strong reference to the resource (e.g.,
> > DocumentThreadableLoader) and it feels fairly natural. I'm not sure why weak
> > reference is preferred. Can you tell me?
> 
> Sorry for not being clear.
> 
> What I'm asking is: conceptually, should the class be "resource owner", or
> "resource client"?
> 
> As far as I read your comment in #10, it looks like you want to keep the
concept
> of "resource owner" whereas I'm suggesting replacing the concept with
"resource
> client" in #11. So I wanted to figure out what the desired design is. If it's
a
> "resource owner", it would make sense to make the reference strong. If it's a
> "resource client", it would make sense to make the reference weak.

I'm fine with the situation that some resource clients hold strong references to
resources, some hold weak references to resources and others don't hold
reference to resources (in that case the client only needs notification). Maybe
I'm affected by the current design.

[yhirano, 2016-06-01 14:32:13]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-06-01 14:32:29]

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

[commit-bot: I haz the power, 2016-06-01 16:51:52]

Description was changed from

==========
Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663
==========

to

==========
Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663
==========

[commit-bot: I haz the power, 2016-06-01 16:51:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-01 16:53:58]

Description was changed from

==========
Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663
==========

to

==========
Make ImportedStyleSheetClient GarbageCollected

ImportedStyleSheetClient is currently a part class of StyleRuleImport, but
this change makes it GarbageCollected as a preparation to make
StyleSheetResourceClient GarbageCollectedMixin.

BUG=587663

Committed: https://crrev.com/bd681d849993450a8b55cd31b1b412fd1f9c4ef8
Cr-Commit-Position: refs/heads/master@{#397160}
==========

[commit-bot: I haz the power, 2016-06-01 16:54:00]

Patchset 2 (id:??) landed as
https://crrev.com/bd681d849993450a8b55cd31b1b412fd1f9c4ef8
Cr-Commit-Position: refs/heads/master@{#397160}