Bug 221019 - Minor cleanup in CSSFontFaceSetClient
Summary: Minor cleanup in CSSFontFaceSetClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 208351
  Show dependency treegraph
 
Reported: 2021-01-26 16:29 PST by Myles C. Maxfield
Modified: 2021-01-28 20:07 PST (History)
10 users (show)

See Also:


Attachments
Patch (9.12 KB, patch)
2021-01-26 16:38 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (15.26 KB, patch)
2021-01-26 20:13 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (16.47 KB, patch)
2021-01-27 14:01 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (16.57 KB, patch)
2021-01-27 15:45 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-01-26 16:29:06 PST
Minor cleanup in CSSFontFaceSetClient
Comment 1 Myles C. Maxfield 2021-01-26 16:38:55 PST
Created attachment 418487 [details]
Patch
Comment 2 Ryosuke Niwa 2021-01-26 16:41:25 PST
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.
Comment 3 Myles C. Maxfield 2021-01-26 20:13:32 PST
Created attachment 418499 [details]
Patch
Comment 4 Jer Noble 2021-01-27 09:42:50 PST
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?
Comment 5 Myles C. Maxfield 2021-01-27 13:08:52 PST
(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
Comment 6 Myles C. Maxfield 2021-01-27 14:01:04 PST
Created attachment 418579 [details]
Patch
Comment 7 Ryosuke Niwa 2021-01-27 14:18:27 PST
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.
Comment 8 Myles C. Maxfield 2021-01-27 14:21:30 PST
(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?
Comment 9 Chris Dumez 2021-01-27 14:22:19 PST
(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.
Comment 10 Myles C. Maxfield 2021-01-27 14:32:52 PST
(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.
Comment 11 Geoffrey Garen 2021-01-27 14:39:08 PST
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.
Comment 12 Jer Noble 2021-01-27 14:39:21 PST
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.
Comment 13 Myles C. Maxfield 2021-01-27 15:45:19 PST
Created attachment 418592 [details]
Patch
Comment 14 EWS 2021-01-28 20:06:46 PST
Committed r272047: <https://trac.webkit.org/changeset/272047>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418592 [details].
Comment 15 Radar WebKit Bug Importer 2021-01-28 20:07:46 PST
<rdar://problem/73737280>