Summary: | [GTK] Revise configuration for MHTML | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | ChangSeok Oh <kevin.cs.oh> | ||||||||||
Component: | WebKitGTK | Assignee: | 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: |
|
Created attachment 126538 [details]
Patch
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 =) (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 =) (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. Created attachment 126565 [details]
Patch
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. Created attachment 126569 [details]
Patch
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. Created attachment 126686 [details]
Patch
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 on attachment 126686 [details] Patch Clearing flags on attachment: 126686 Committed r107565: <http://trac.webkit.org/changeset/107565> All reviewed patches have been landed. Closing bug. |
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