Bug 78364

Summary: [GTK] Revise configuration for MHTML
Product: WebKit Reporter: ChangSeok Oh <kevin.cs.oh>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description ChangSeok Oh 2012-02-10 09:59:28 PST
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
Comment 1 ChangSeok Oh 2012-02-10 11:11:02 PST
Created attachment 126538 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-02-10 12:07:30 PST
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 =)
Comment 3 Gustavo Noronha (kov) 2012-02-10 12:09:49 PST
(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 =)
Comment 4 Martin Robinson 2012-02-10 12:24:40 PST
(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.
Comment 5 ChangSeok Oh 2012-02-10 12:59:29 PST
Created attachment 126565 [details]
Patch
Comment 6 ChangSeok Oh 2012-02-10 13:19:13 PST
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.
Comment 7 ChangSeok Oh 2012-02-10 13:19:46 PST
Created attachment 126569 [details]
Patch
Comment 8 Martin Robinson 2012-02-10 19:58:35 PST
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.
Comment 9 ChangSeok Oh 2012-02-12 15:30:37 PST
Created attachment 126686 [details]
Patch
Comment 10 ChangSeok Oh 2012-02-12 15:32:54 PST
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.
Comment 11 WebKit Review Bot 2012-02-13 06:16:48 PST
Comment on attachment 126686 [details]
Patch

Clearing flags on attachment: 126686

Committed r107565: <http://trac.webkit.org/changeset/107565>
Comment 12 WebKit Review Bot 2012-02-13 06:16:53 PST
All reviewed patches have been landed.  Closing bug.