Bug 221019 – Minor cleanup in CSSFontFaceSetClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Minor cleanup in CSSFontFaceSetClient

[Myles C. Maxfield]

Created attachment 418487 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 418487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418487&action=review

> Source/WebCore/css/CSSFontFaceSet.h:46
> +    class Client {

Make this CanMakeWeakPtr.

> Source/WebCore/css/CSSFontFaceSet.h:138
> -    HashSet<CSSFontFaceSetClient*> m_clients;
> +    HashSet<Client*> m_clients;

Use WeakHashSet.

[Myles C. Maxfield]

Created attachment 418499 [details]
Patch

[Jer Noble]

Comment on attachment 418499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418499&action=review

> Source/WebCore/css/FontFaceSet.h:41
> -class FontFaceSet final : public RefCounted<FontFaceSet>, private CSSFontFaceSetClient, public EventTargetWithInlineData, public ActiveDOMObject {
> +class FontFaceSet final : public RefCounted<FontFaceSet>, public EventTargetWithInlineData, public ActiveDOMObject {

Can you now remove the `#include "CSSFontFaceSet.h"` from FontFaceSet.h after this change?

[Myles C. Maxfield]

(In reply to Jer Noble from comment #4)
> Comment on attachment 418499 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418499&action=review
> 
> > Source/WebCore/css/FontFaceSet.h:41
> > -class FontFaceSet final : public RefCounted<FontFaceSet>, private CSSFontFaceSetClient, public EventTargetWithInlineData, public ActiveDOMObject {
> > +class FontFaceSet final : public RefCounted<FontFaceSet>, public EventTargetWithInlineData, public ActiveDOMObject {
> 
> Can you now remove the `#include "CSSFontFaceSet.h"` from FontFaceSet.h
> after this change?

Nope. FontFaceSet.h mentions things like CSSFontFaceSet::FontEventTypes

[Myles C. Maxfield]

Created attachment 418579 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 418579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418579&action=review

> Source/WebCore/css/CSSFontFaceSet.cpp:63
> +    auto result = m_fontModifiedObservers.add(fontModifiedObserver);
> +    ASSERT_UNUSED(result, result.isNewEntry);

Where are we deleting these?
WeakHashSet doesn't know how to shrink itself automatically so you'd need to manually delete items.

[Myles C. Maxfield]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 418579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418579&action=review
> 
> > Source/WebCore/css/CSSFontFaceSet.cpp:63
> > +    auto result = m_fontModifiedObservers.add(fontModifiedObserver);
> > +    ASSERT_UNUSED(result, result.isNewEntry);
> 
> Where are we deleting these?
> WeakHashSet doesn't know how to shrink itself automatically so you'd need to
> manually delete items.

I'm confused. I thought the whole point of using weak pointers was that the derived class didn't have to run any code in its destructor. But now it seems like we would have to run code in its destructor any way?

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 418579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418579&action=review
> 
> > Source/WebCore/css/CSSFontFaceSet.cpp:63
> > +    auto result = m_fontModifiedObservers.add(fontModifiedObserver);
> > +    ASSERT_UNUSED(result, result.isNewEntry);
> 
> Where are we deleting these?
> WeakHashSet doesn't know how to shrink itself automatically so you'd need to
> manually delete items.

Oh really? then I think we may need to review other uses of WeakHashSet. I think I have seen a few patches that were using WeakHashSet as a convenient mean to not have to remove objects from the set upon destruction.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #8)
> (In reply to Ryosuke Niwa from comment #7)
> > Comment on attachment 418579 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=418579&action=review
> > 
> > > Source/WebCore/css/CSSFontFaceSet.cpp:63
> > > +    auto result = m_fontModifiedObservers.add(fontModifiedObserver);
> > > +    ASSERT_UNUSED(result, result.isNewEntry);
> > 
> > Where are we deleting these?
> > WeakHashSet doesn't know how to shrink itself automatically so you'd need to
> > manually delete items.
> 
> I'm confused. I thought the whole point of using weak pointers was that the
> derived class didn't have to run any code in its destructor. But now it
> seems like we would have to run code in its destructor any way?

Rather than having the derived class run code in its destructor, another solution would be to give WeakHashSet a public rehash() function, and have users call that at strategic times.

[Geoffrey Garen]

Comment on attachment 418579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418579&action=review

>>>>> Source/WebCore/css/CSSFontFaceSet.cpp:63
>>>>> +    ASSERT_UNUSED(result, result.isNewEntry);
>>>> 
>>>> Where are we deleting these?
>>>> WeakHashSet doesn't know how to shrink itself automatically so you'd need to manually delete items.
>>> 
>>> I'm confused. I thought the whole point of using weak pointers was that the derived class didn't have to run any code in its destructor. But now it seems like we would have to run code in its destructor any way?
>> 
>> Oh really? then I think we may need to review other uses of WeakHashSet. I think I have seen a few patches that were using WeakHashSet as a convenient mean to not have to remove objects from the set upon destruction.
> 
> Rather than having the derived class run code in its destructor, another solution would be to give WeakHashSet a public rehash() function, and have users call that at strategic times.

My preferred suggestion is for WeakHashSet to rehash on its own, based on a counter. Increment the counter in add() (and in other functions if you like). When the counter reaches a certain percentage of set capacity, rehash. Reasons to prefer: It's automatic and still average case amortized O(1); no client will ever have a better notion of when to rehash.

[Jer Noble]

Comment on attachment 418579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418579&action=review

>>>>> Source/WebCore/css/CSSFontFaceSet.cpp:63
>>>>> +    ASSERT_UNUSED(result, result.isNewEntry);
>>>> 
>>>> Where are we deleting these?
>>>> WeakHashSet doesn't know how to shrink itself automatically so you'd need to manually delete items.
>>> 
>>> I'm confused. I thought the whole point of using weak pointers was that the derived class didn't have to run any code in its destructor. But now it seems like we would have to run code in its destructor any way?
>> 
>> Oh really? then I think we may need to review other uses of WeakHashSet. I think I have seen a few patches that were using WeakHashSet as a convenient mean to not have to remove objects from the set upon destruction.
> 
> Rather than having the derived class run code in its destructor, another solution would be to give WeakHashSet a public rehash() function, and have users call that at strategic times.

WeakHashSet will delete empty WeakPtr's inside computeSizes(), which itself is called from map(), which itself is called from forEach().  So if you use forEach() to iterate rather than a for(item : set), it'll automatically shrink itself.

[Myles C. Maxfield]

Created attachment 418592 [details]
Patch

[EWS]

Committed r272047: <https://trac.webkit.org/changeset/272047>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418592 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73737280>