[webkit-reviews] review granted: [Bug 192405] [WTF] Add environment variable helpers : [Attachment 361942] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 192405: [WTF] Add environment variable helpers
https://bugs.webkit.org/show_bug.cgi?id=192405

Attachment 361942: Patch

https://bugs.webkit.org/attachment.cgi?id=361942&action=review




--- Comment #74 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 361942
  --> https://bugs.webkit.org/attachment.cgi?id=361942
Patch

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

Looks good, r=me.

I still think you're overusing auto (one of the rare cases I will disagree with
Scott Meyers ;) but we don't have a style rule about that currently, and this
is hardly uncommon in WebKit, and the callsites are otherwise significantly
easier to read now anyway, thanks to your convenience functions.

> Tools/WebKitTestRunner/InjectedBundle/gtk/InjectedBundleGtk.cpp:48
> -    if (!g_getenv("WEBKIT_TOP_LEVEL"))
> -	   g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE);
> +    g_setenv("WEBKIT_TOP_LEVEL", TOP_LEVEL_DIR, FALSE);

Well that doesn't look finished. It should use Environment::set, right? If it
asserts, we can try to fix it in a followup (just needs moved to the top of web
process main).