Bug 178321 – [Settings] Split non-macro generated parts of Settings into SettingsBase base class

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

[Sam Weinig]

[Settings] Split non-macro generated parts of Settings into SettingsBase base class

[Sam Weinig]

Created attachment 323847 [details]
Patch

[Build Bot]

Attachment 323847 [details] did not pass style-queue:


ERROR: Source/WebCore/page/SettingsBase.h:140:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/page/SettingsBase.h:190:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/page/SettingsBase.cpp:314:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/page/SettingsBase.cpp:417:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/SettingsBase.cpp:455:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 323847 [details]
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.

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 323847 [details]
> 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?

Alas, we can't, there is caller who passes nullptr in Document for frameless Documents.

> 
> > Source/WebCore/page/SettingsBase.cpp:145
> >      : m_page(nullptr)
> 
> Initialize in class definition instead of here?

Yup, will fix all of these.


> > 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.

I'm going to move the RefCounted back to Settings.

[Sam Weinig]

Created attachment 323855 [details]
Patch

[Build Bot]

Attachment 323855 [details] did not pass style-queue:


ERROR: Source/WebCore/page/SettingsBase.h:140:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/page/SettingsBase.h:190:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/page/SettingsBase.cpp:311:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/page/SettingsBase.cpp:414:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/page/SettingsBase.cpp:452:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 323855 [details]
Patch

Clearing flags on attachment: 323855

Committed r223332: <https://trac.webkit.org/changeset/223332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35000638>