Bug 78364 – [GTK] Revise configuration for MHTML

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ChangSeok Oh]

This bug is going to deal with two issue about WebGL & MHTML
1. The WebGL enabling option for gtk port is --enable-webgl, but this doesn't match build-webkit's --3d-canvas. So we can't enable or disable webgl by using build-webkit script.
2. MHTML enabling option already exists in configure.ac & GNUmakefile.am. But help string has been missed so that there is no message displayed on screen while configuring. In addition. if we activate this feature, build-break occurs like following...
> ./.libs/libWebCore.a(lt2-libWebCore_la-ArchiveFactory.o): In function `WebCore::ArchiveFactory::isArchiveMimeType(WTF::String const&)':
> /home/shivamidow/Documents/Projects/WebKit/WebKitBuild/Debug/../../Source/JavaScriptCore/wtf/HashTable.h:711: multiple definition of `WebCore::ArchiveFactory::isArchiveMimeType(WTF::String const&)'
> ./.libs/libWebCore.a(libWebCore_la-ArchiveFactory.o):/home/shivamidow/Documents/Projects/WebKit/WebKitBuild/Debug/../../Source/JavaScriptCore/wtf/HashTable.h:711: first defined here

[ChangSeok Oh]

Created attachment 126538 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 126538 [details]
Patch

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 =)

[Gustavo Noronha (kov)]

(In reply to comment #2)
> > -	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?

I figured this one out already, they are the duplicates =)

[Martin Robinson]

(In reply to comment #2)

> > 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 =)

I agree. The names exposed in configure.ac are for people compiling WebKitGTK+ from a tarball. "3D canvas" is a name particular to WebKit, while "WebGL" is more well-known. if the arguments to build-webkit are not properly mapped to configure arguments that's another sort of bug.

[ChangSeok Oh]

Created attachment 126565 [details]
Patch

[ChangSeok Oh]

Comment on attachment 126538 [details]
Patch

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

Thank you for the review! :)
I removed the changes related with webgl from this patch.

>> ChangeLog:10
>> +        And added new configuration messages for mhtml.
> 
> Can you add a small description of what mhtml is in the changelog?

Oops, I forgot to add a description of mhtml in the new patch. Here i note the description, "MHTML, short for MIME HTML, is a web page archive format used to combine resources that are typically represented by external links (such as images, Flash animations, Java applets, audio files) together with HTML code into a single file" -from wikipedia.
Visit here http://en.wikipedia.org/wiki/MHTML :)

>> configure.ac:540
>> +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 =)

Thank you for kind comment. I removed the webgl change from this patch.

[ChangSeok Oh]

Created attachment 126569 [details]
Patch

[Martin Robinson]

Comment on attachment 126569 [details]
Patch

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

This patch looks good, apart from one minor change. Thanks for fixing this!

> Source/WebCore/GNUmakefile.am:38
> +	-I$(srcdir)/Source/WebCore/loader/archive/mhtml \

You should probably only add this directory if mhtml is enabled.

[ChangSeok Oh]

Created attachment 126686 [details]
Patch

[ChangSeok Oh]

Comment on attachment 126569 [details]
Patch

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

Thank you for the review. :)

>> Source/WebCore/GNUmakefile.am:38
>> +	-I$(srcdir)/Source/WebCore/loader/archive/mhtml \
> 
> You should probably only add this directory if mhtml is enabled.

Done.

[WebKit Review Bot]

Comment on attachment 126686 [details]
Patch

Clearing flags on attachment: 126686

Committed r107565: <http://trac.webkit.org/changeset/107565>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.