[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
bugzilla-daemon at webkit.org
Sun Oct 15 17:38:54 PDT 2017
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.
More information about the webkit-reviews
mailing list