[webkit-reviews] review denied: [Bug 116611] [GTK] Need a way to enable region based columns from the command line : [Attachment 202822] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Morten Stenshorne
<mstensho at opera.com>'s request for review:
Bug 116611: [GTK] Need a way to enable region based columns from the command
line
https://bugs.webkit.org/show_bug.cgi?id=116611

Attachment 202822: Patch
https://bugs.webkit.org/attachment.cgi?id=202822&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.