[webkit-reviews] review granted: [Bug 178321] [Settings] Split non-macro generated parts of Settings into SettingsBase base class : [Attachment 323847] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 178321: [Settings] Split non-macro generated parts of Settings into
SettingsBase base class
https://bugs.webkit.org/show_bug.cgi?id=178321

Attachment 323847: Patch

https://bugs.webkit.org/attachment.cgi?id=323847&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 323847
  --> https://bugs.webkit.org/attachment.cgi?id=323847
Patch

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

> Source/WebCore/page/SettingsBase.cpp:144
> +SettingsBase::SettingsBase(Page* page)

Take a reference to a page?

> Source/WebCore/page/SettingsBase.cpp:145
>      : m_page(nullptr)

Initialize in class definition instead of here?

> Source/WebCore/page/SettingsBase.cpp:146
>      , m_mediaTypeOverride("screen")

Initialize in class definition instead of here?

> Source/WebCore/page/SettingsBase.cpp:148
>      , m_storageBlockingPolicy(SecurityOrigin::AllowAllStorage)

Initialize in class definition instead of here?

> Source/WebCore/page/SettingsBase.cpp:168
>      , m_timeWithoutMouseMovementBeforeHidingControls(3_s)

Initialize in class definition instead of here?

> Source/WebCore/page/SettingsBase.h:102
> +    ~SettingsBase();

I’d think this would need to be virtual, or the reference counting would need
to be in the derived class, not the base. Or some other trick to make sure we
call ~Settings when we do the last deref.