Bug 217551 – Use WebPreference definitions from shared configuration files in WebCore (Part 1)

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Use WebPreference definitions from shared configuration files in WebCore

[Sam Weinig]

Created attachment 410997 [details]
Patch

[Sam Weinig]

Created attachment 411009 [details]
Patch

[Sam Weinig]

Created attachment 411010 [details]
Patch

[Sam Weinig]

Comment on attachment 411010 [details]
Patch

This is ready for review now.

[Sam Weinig]

Created attachment 411012 [details]
Patch

[Darin Adler]

Comment on attachment 411012 [details]
Patch

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

> Source/WTF/ChangeLog:20
> +        This also adds some new keys from WebCore's Settings.yaml:

Not thrilled with the "webcore" prefix.

> Source/WTF/ChangeLog:21
> +        - 'webcoreOnChange: *' called by WebCore::Settings when the setting changes.

There’s something really strange about having WebCore function names in WTF. I could easily imagine overlooking these when doing a global search.

[EWS]

Committed r268313: <https://trac.webkit.org/changeset/268313>

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

[Radar WebKit Bug Importer]

<rdar://problem/70174046>

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 411012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411012&action=review
> 
> > Source/WTF/ChangeLog:20
> > +        This also adds some new keys from WebCore's Settings.yaml:
> 
> Not thrilled with the "webcore" prefix.

Is it the capitalization? Would "WebCore" work better for you, or just the idea of WebCore being mentioned in WTF?

> 
> > Source/WTF/ChangeLog:21
> > +        - 'webcoreOnChange: *' called by WebCore::Settings when the setting changes.
> 
> There’s something really strange about having WebCore function names in WTF.
> I could easily imagine overlooking these when doing a global search.

I agree completely, though not yet sure on how to resolve this. I have a few ideas though:

One option, that I don't love all that much, is to have something in WebCore be able to augment the WebPreferences*.yaml files with more data. I like the centrality of having all the data in one place of the WebPreferences*.yaml and that defeats it a bit.

Another option would be to change things around a bit, and instead of generating Settings, we generate a new class, SettingsGenerated, that a hand written Settings class derives from. Then, in the SettingsGenerated, we could do something like have each setter have a virtual didSetFoo() function, that is initially empty and only overriden by Settings if wanted. e.g.

class SettingsGenerated : public SettingsBase {
public:
    bool allowFileAccessFromFileURLs() const { return m_allowFileAccessFromFileURLs; }
    void setAllowFileAccessFromFileURLs(bool value) { m_allowFileAccessFromFileURLs = value; didSetAllowFileAccessFromFileURLs(); }
    virtual void didSetAllowFileAccessFromFileURLs() { }

    bool authorAndUserStylesEnabled() const { return m_authorAndUserStylesEnabled; }
    void setAuthorAndUserStylesEnabled(bool value) { m_authorAndUserStylesEnabled = value; didSetAuthorAndUserStylesEnabled(); }
    virtual void didSetAuthorAndUserStylesEnabled() { }

    ...
};


class Settings final : public SettingsGenerated {
public:
    virtual void didSetAuthorAndUserStylesEnabled() override { setNeedsRecalcStyleInAllFrames(); }
};

[Darin Adler]

(In reply to Sam Weinig from comment #9)
> > Not thrilled with the "webcore" prefix.
> 
> Is it the capitalization? Would "WebCore" work better for you, or just the
> idea of WebCore being mentioned in WTF?

I think it’s a combination of the capitalization and also the feeling that inside WebKit, WebCore shouldn’t have to go by its full name. In the past I named functions things like "core" and maybe that was not great.

> Another option would be to change things around a bit, and instead of
> generating Settings, we generate a new class, SettingsGenerated, that a hand
> written Settings class derives from. Then, in the SettingsGenerated, we
> could do something like have each setter have a virtual didSetFoo()
> function, that is initially empty and only overriden by Settings if wanted.

I like this as long as the compiler can hep us check that we overrode correctly. Strangely it would help us if we were using virtual functions but I’m not sure that you can use override on a non-virtual function.

[Darin Adler]

(In reply to Darin Adler from comment #10)
> > Another option would be to change things around a bit, and instead of
> > generating Settings, we generate a new class, SettingsGenerated, that a hand
> > written Settings class derives from. Then, in the SettingsGenerated, we
> > could do something like have each setter have a virtual didSetFoo()
> > function, that is initially empty and only overriden by Settings if wanted.

A little sad that these would all be virtual functions, but otherwise seems a good pattern. Could also have these be functions on a passed in class to a class template.