[webkit-reviews] review denied: [Bug 61940] [Qt] [WK2] Debug info leaks to stdout from plugins in Qt WebKit2 layout tests : [Attachment 97613] fix patch 3

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 61940: [Qt] [WK2] Debug info leaks to stdout from plugins in Qt WebKit2
layout tests
https://bugs.webkit.org/show_bug.cgi?id=61940

Attachment 97613: fix patch 3
https://bugs.webkit.org/attachment.cgi?id=97613&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97613&action=review

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:43
> +#if OS(UNIX)
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#endif

I don't think OS(UNIX) is needed here; using X11 pretty much implies Unix.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:51
> +class StdoutRedirect {

Maybe a noun would be better here. StdoutRedirector?

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:53
> +    StdoutRedirect(const char* redirectPath);

I don't think we need to support redirecting to an arbitrary path. In fact,
maybe the class name could be StdoutDevNullRedirector?

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:63
> +#if OS(UNIX)

Again, I don't think OS(UNIX) is needed here.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:86
> +#if OS(UNIX)
> +#define SUPPRESS_STDOUT_LOCALLY StdoutRedirect stdoutRedirect("/dev/null")
> +#else
> +#define SUPPRESS_STDOUT_LOCALLY
> +#endif

There's no real reason to have a macro here. The class is already a no-op on
Non-Unix platforms (which I again don't think we need to have an #ifdef check
for)

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:114
> +    SUPPRESS_STDOUT_LOCALLY;

Again, just declaring the object instead of using a macro here is more clear.
Also, I'd like to see a comment explaining why this is here.