Bug 116611 – [GTK] Need a way to enable region based columns from the command line

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Morten Stenshorne]

I'm working on the region based multicol implementation, and there's currently no way to enable it in the GTK port.

[Morten Stenshorne]

Created attachment 202533 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/webkit/webkitwebsettings.cpp', u'Source/WebKit/gtk/webkit/webkitwebsettingsprivate.h', u'Source/WebKit/gtk/webkit/webkitwebview.cpp']" exit_code: 1
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:556:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:558:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:559:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:560:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:561:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 4 files


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

[Zan Dobersek]

Are you bound to the WK1 layer? The focus is now on the GTK port of the WK2 layer, with the WK1 layer in some sort of a maintenance mode, meaning that API additions are now most welcome under WK2.

[Morten Stenshorne]

I wrote a patch for WK1 because that seems to be what I use by default. I can add it for WK2 as well. But how do I enable WK2? Rebuild with --no-webkit1 ?

[Martin Robinson]

This is not really something that should be in the public API. The implementation of a CSS feature should be a private detail of the library, not something the embedder should worry about. If this setting is required it should be private. You don't say why you need it, but if it's for running tests, look at DumpRenderTreeSupport.

[Morten Stenshorne]

I'm working on the region based multicol implementation, and I need some way to run WebKit with this code enabled, so that I can see how it works, and debug it. The code for region based multicol is behind WebCore::Settings::regionBasedColumnsEnabled() checks. This patch provides a way to make that method return true when I want it to, without having to change or recompile any code.

When running LayoutTests, there's already internals.settings.setRegionBasedColumnsEnabled(true) to do this, but that doesn't work in the actual browser.

But if you know of a different or better way, maybe I don't need to add a command line switch and mess with your public API.

[Martin Robinson]

(In reply to comment #6)

> But if you know of a different or better way, maybe I don't need to add a command line switch and mess with your public API.

Perhaps an environment variable would be a better option? Other people are looking at adding settings for regions: https://bugs.webkit.org/show_bug.cgi?id=116043 so perhaps we could provide one environment variable for all experimental features.

[Morten Stenshorne]

Sure, that would work too. One environment variable to toggle experimental features sounds fine. After your last comment, a post in bug 116043 seems to suggest that it's being dropped, so I suppose I'm on my own.

Any guidelines as to what to name the variable, or where exactly to put the code for it? You guys also seem eager to move me over to WK2. :) Exactly how would I go about that?

[Martin Robinson]

(In reply to comment #8)
 
> Any guidelines as to what to name the variable, or where exactly to put the code for it? You guys also seem eager to move me over to WK2. :) Exactly how would I go about that?

You might consider integrating the code into WebSettings.cpp. When settings are applied to the WebView might be a good time to look for the environment variable. I think it's really only important if this works for MiniBrowser. WebKitTestRunner doesn't use the GObject-based API, so no need to worry about the WebSettings code overriding that. Naming is up to you, but maybe something like WEBKITGTK_EXPERIMENTAL_FEATURES="CSS_REGIONS=1,CSS_MULTICOL_WITH_REGION=1" ?

[Morten Stenshorne]

$ find . | grep WebSettings.cpp
./Source/WebKit/blackberry/Api/WebSettings.cpp

Did you mean Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp ? What function should I hook this into?

[Morten Stenshorne]

Created attachment 202822 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Morten Stenshorne]

Any comments?

[Martin Robinson]

Comment on attachment 202822 [details]
Patch

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

Looks pretty good to me. I have a few nits about the implementation.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:77
> +#if 0
> +}
> +#endif

Why is this code disabled?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:80
> +    REGION_BASED_COLUMNS

This isn't the naming convention we use for private enums in WebKit.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:91
> +bool envParsed;

In WebKit we avoid using abbreviations except for the most common words.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:95
> +    if (const char* data = getenv("WEBKITGTK_EXPERIMENTAL_FEATURES")) {

I would rather see an early return here than a deeply nested if statement. That's more typical in WebKit.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:96
> +        const int featureCount = sizeof(settings) / sizeof(Setting);

You could use WTF_ARRAY_LENGTH here I think.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:116
> +            while (data && *data) {
> +                if (const char* keyEnd = strpbrk(data, "=")) {
> +                    const char* keyStart = data;
> +                    const char* valStart = keyEnd + 1;
> +                    for (int i = 0; i < featureCount; i++) {
> +                        if (!strncmp(settings[i].featureStr, keyStart, keyEnd - keyStart)) {
> +                            settings[i].enabled = *valStart && *valStart - '0';
> +                            break;
> +                        }
> +                    }
> +                    data = strpbrk(valStart, ",");
> +                    if (data)
> +                        data++;
> +                } else
> +                    break;
> +            }

This code isn't performance critical, so perhaps it's better to use String.split here?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:124
> +    if (!envParsed)

It makes sense to make environmentParsed a static local instead of a global variable.

[Morten Stenshorne]

Comment on attachment 202822 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:77
>> +#endif
> 
> Why is this code disabled?

According to the WebKit coding standards, namespace blocks are not indented, but my editor doesn't agree with that. :) But I'll remove it.

[Morten Stenshorne]

Created attachment 204498 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 204498 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:55
> +
> +        applyExperimentalFeatures();

I don't think this is the best place to apply the preferences. Here we get the default value of string and custom preferences. For string preferences we cache the utf8 value to compare with the given value in set methods without doing any charset conversion nor memory allocation. We need to cache the values here to make sure that when construct properties are set to their default value, the set method returns early without sending a PreferencesDidChange message to the WebProcess for every preference when WebKitSettings object is constructed. So, default values for experimental features could be set in GObjectClass::constructed() once all other preferences has been set already.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:93
> +    const int featureCount = WTF_ARRAY_LENGTH(settings);

Use size_t or unsigned instead of int

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:95
> +        for (int i = 0; i < featureCount; i++)

Use size_t or unsigned here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:99
> +        String(data).split(",", false, variables);

Since it's a single character you could use ',' instead of "," which I think is more efficient.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:102
> +            variables[i].split("=", false, keyAndValue);

Same here '=' instead of "="

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:103
> +            if (keyAndValue.size() == 2) {

You can save an indentation level by continuing the loop if this is false

if (keyAndValue.size() != 2)
    continue;

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:105
> +                String key = keyAndValue[0];
> +                String value = keyAndValue[1];

Are strings copied here? Should they be const String& instead?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:106
> +                for (int i = 0; i < featureCount; i++) {

use size_t or unsigned, this is a bit confusing because i, is already the iterator of the first loop.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:110
> +                    }

Maybe we could move this inner loop to a helper function setEnableByName(const String& featureName, bool enable); or something like that to make it a bit easier to read.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:131
> +    preferences.get()->setRegionBasedColumnsEnabled(ExperimentalFeatures::isEnabled(ExperimentalFeatures::RegionBasedColumns));

You should check that the value we are setting is not the current one, to avoid sending a PreferencesDidChange message to the UI process.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:133
> +}
> +

I wonder if we could move all this ExperimentalFeatures to its own file, since WebKitSettings is already too long and experimental features are not actually settings from the API POV.

[Morten Stenshorne]

Created attachment 204576 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 204576 [details]
Patch

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

Looks much better, thanks!

> Source/WebKit2/ChangeLog:25
> +        * UIProcess/API/gtk/ExperimentalFeatures.cpp: Added.

Since this is not part of the API, maybe it would be better to move it to UIProcess/gtk/

> Source/WebKit2/UIProcess/API/gtk/ExperimentalFeatures.cpp:37
> +    const char* featureStr;

Sorry, I didn't notice this in previous patch, instead of abbreviation Str, better use featureString or even featureName.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:146
> +    WebKitSettingsPrivate* priv = WEBKIT_SETTINGS(object)->priv;
> +    WebPreferences* prefs = priv->preferences.get();

Since we don't need priv you can get the prefs directly with

WebPreferences* prefs = WEBKIT_SETTINGS(object)->priv->preferences.get();

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:1023
> +<SECTION>
> +<FILE>ExperimentalFeatures</FILE>
> +isEnabled
> +
> +<SUBSECTION Private>
> +parseEnvironment
> +setEnableByName
> +</SECTION>

Do not add symbols here, this is only for public API.

[Morten Stenshorne]

Comment on attachment 204576 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:1023
>> +</SECTION>
> 
> Do not add symbols here, this is only for public API.

I got warnings during building. That was my only reason for adding it. But perhaps the warnings won't be generated if I move the code as you suggested in another comment.

[Carlos Garcia Campos]

(In reply to comment #20)
> (From update of attachment 204576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204576&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:1023
> >> +</SECTION>
> > 
> > Do not add symbols here, this is only for public API.
> 
> I got warnings during building. That was my only reason for adding it. But perhaps the warnings won't be generated if I move the code as you suggested in another comment.

Yes, moving the file would fix the warnings.

[Morten Stenshorne]

Created attachment 204685 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 204685 [details]
Patch

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

This looks good to me. Martin?

> Source/WebKit2/ChangeLog:21
> +        Reviewed by NOBODY (OOPS!).

The explanation should be after this line, sorry I didn't notice this in previous patches.

> Source/WebKit2/UIProcess/gtk/ExperimentalFeatures.cpp:41
> +static Setting settings[] = {

We can probably make this const

[Morten Stenshorne]

Comment on attachment 204685 [details]
Patch

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

>> Source/WebKit2/UIProcess/gtk/ExperimentalFeatures.cpp:41
>> +static Setting settings[] = {
> 
> We can probably make this const

It cannot, since it's changed when the environment variable is parsed.

[Morten Stenshorne]

Created attachment 204695 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #24)
> (From update of attachment 204685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204685&action=review
> 
> >> Source/WebKit2/UIProcess/gtk/ExperimentalFeatures.cpp:41
> >> +static Setting settings[] = {
> > 
> > We can probably make this const
> 
> It cannot, since it's changed when the environment variable is parsed.

Indeed, you are right

[Morten Stenshorne]

So is the last patch okay then?

[Martin Robinson]

Comment on attachment 204695 [details]
Patch

Looks good to me! A great improvement from the original patch. I think this just needs an owner to sign off on it.

[Morten Stenshorne]

Can someone cq+ me, please?

[Martin Robinson]

(In reply to comment #29)
> Can someone cq+ me, please?

Please ask one of the WebKit2 owners to cq this patch. It is required that an owner give final approval to all WebKit2 patches.

[Morten Stenshorne]

Picked three random people from Source/WebKit2/Owners

May I have a final approval from one of the WebKit2 owners, please?

[Gustavo Noronha (kov)]

Comment on attachment 204695 [details]
Patch

This is only touching GTK+ stuff, and per the discussions in the contributors meeting I think it does not require owners approval to go in, so I'm cq+ing.

[Martin Robinson]

(In reply to comment #32)
> (From update of attachment 204695 [details])
> This is only touching GTK+ stuff, and per the discussions in the contributors meeting I think it does not require owners approval to go in, so I'm cq+ing.

Apologies! The rules changed and I didn't keep up with them.

[WebKit Commit Bot]

Comment on attachment 204695 [details]
Patch

Clearing flags on attachment: 204695

Committed r152922: <http://trac.webkit.org/changeset/152922>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.