[webkit-reviews] review denied: [Bug 30240] window attributes (like localStorage) that are disabled at runtime are still visible : [Attachment 40976] Addressed jorlow's style suggestions.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Andrew Wilson
<atwilson at chromium.org>'s request for review:
Bug 30240: window attributes (like localStorage) that are disabled at runtime
are still visible
https://bugs.webkit.org/show_bug.cgi?id=30240

Attachment 40976: Addressed jorlow's style suggestions.
https://bugs.webkit.org/attachment.cgi?id=40976&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Overall, this looks good, couple of nits:

>      my @disallows_shadowing;
> +    # Also separate out attributes that are enabled at runtime so we can
process them specially.
> +    my @enabled_at_runtime;

both should be @disallowsShadowing and @enabledAtRuntime. Sorry for making you
refactor old code, but those are style-boogers we shouldn't have, so it's a
good thing :)

>  
> +    # Setup the enable-at-runtime attrs if we have them
> +    foreach my $runtime_attr (@enabled_at_runtime) {
> +	   $enable_function = $interfaceName .
$codeGenerator->WK_ucfirst($runtime_attr->signature->name);

Ditto.

> +	   my $conditionalString =
GenerateConditionalString($runtime_attr->signature);
> +	   push(@implContent, "\n#if ${conditionalString}\n") if
$conditionalString;
> +	   push(@implContent, "  if (V8Custom::v8${enable_function}Enabled())
{\n");

4 spaces even in generated code :)

> +#define ACCESSOR_RUNTIME_ENABLER(NAME) bool V8Custom::v8##NAME##Enabled()

I was envisioning something of a one-place structure, which can be queried for
a feature, but I guess this is ok, too.