[webkit-reviews] review denied: [Bug 78364] [GTK] Revise configuration for WebGL & MHTML : [Attachment 126538] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 78364: [GTK] Revise configuration for WebGL & MHTML
https://bugs.webkit.org/show_bug.cgi?id=78364

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

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126538&action=review


r- for unrelated changes coalesced, and for the fact that we do not follow
build-webkit naming for our configure script

> Source/WebCore/ChangeLog:9
> +	   Added mhtml directory and removed target files duplicated to build
when enabling mhtml.
> +

This change looks OK, but it's not a good idea to have unrelated changes (like
the webgl one) in the same patch.

> Source/WebCore/GNUmakefile.list.am:-5696
> -	Source/WebCore/loader/archive/Archive.cpp \
> -	Source/WebCore/loader/archive/Archive.h \
> -	Source/WebCore/loader/archive/ArchiveFactory.cpp \
> -	Source/WebCore/loader/archive/ArchiveFactory.h \

These files are not used by mhtml? Any idea why they were added here in the
first place? Or are these duplicates?

> ChangeLog:10
> +	   And added new configuration messages for mhtml.

Can you add a small description of what mhtml is in the changelog?

> configure.ac:540
> -# check whether to enable WebGL support
> -AC_MSG_CHECKING([whether to enable WebGL support])
> -AC_ARG_ENABLE(webgl,
> -		 AC_HELP_STRING([--enable-webgl], [enable support for WebGL
[default=yes]]),
> -		 [], [if test "$with_target" = "x11"; then enable_webgl="yes";
else enable_webgl="no"; fi])
> -AC_MSG_RESULT([$enable_webgl])
> +# check whether to enable 3D canvas (WebGL) support
> +AC_MSG_CHECKING([whether to enable 3D canvas (WebGL) support])
> +AC_ARG_ENABLE(3d-canvas,
> +		 AC_HELP_STRING([--enable-3d-canvas], [enable support for 3D
canvas (WebGL) [default=yes]]),
> +		 [], [if test "$with_target" = "x11"; then
enable_3d_canvas="yes"; else enable_3d_canvas="no"; fi])
> +AC_MSG_RESULT([$enable_3d_canvas])

This is intended. You can give --enable-webgl to build-webkit, and the option
will be forwarded. We do not need to follow build-webkit's naming on our names
=)