Bug 143758

Summary: WebCore uses M_PI_2 instead of piOverTwoDouble
Product: WebKit Reporter: LRN <lrn1986>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, darin, lrn1986, mcrha
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 133028    
Attachments:
Description Flags
Use piOverTwoDouble instead of M_PI_2 darin: review+

Description LRN 2015-04-15 06:19:47 PDT
This doesn't work well when M_PI_2 is undefined.
Comment 1 LRN 2015-04-15 09:50:27 PDT
Created attachment 250803 [details]
Use piOverTwoDouble instead of M_PI_2

Because M_PI_2 is not defined everywhere.
Fixes errors like:
  CXX      Source/WebCore/platform/graphics/freetype/libPlatformGtk_la-FontPlatformDataFreeType.lo
../webkitgtk-2.4.8/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp: In function 'void WebCore::rotateCairoMatrixForVerticalOrientation(cairo_matrix_t*)':
../webkitgtk-2.4.8/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:123:34: error: 'M_PI_2' was not declared in this scope
     cairo_matrix_rotate(matrix, -M_PI_2);
                                  ^
GNUmakefile:51663: recipe for target 'Source/WebCore/platform/graphics/freetype/libPlatformGtk_la-FontPlatformDataFreeType.lo' failed
Comment 2 Darin Adler 2015-04-15 10:35:59 PDT
All these patches you uploaded are wrong; they all have No new tests (OOPS!) in them. It was a big mistake to upload tons of patches all at once before you tried one or two and learned how to do them correctly.
Comment 3 Milan Crha 2015-04-17 16:10:18 PDT
I do not need this change with webkit-2.4 branch at revision 182543
Comment 4 Milan Crha 2015-04-17 16:13:27 PDT
(In reply to comment #3)
> I do not need this change with webkit-2.4 branch at revision 182543

Oops, no, I was wrong, it's still in use here. I'm sorry for confusion.
Comment 5 Milan Crha 2015-04-17 16:40:31 PDT
I also patch the same file with this, to be able to build it in webkit-2.4 branch at revision 182543:

--- webkitgtk-2.4.9.orig/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp	2015-04-08 16:19:51 +0000
+++ webkitgtk-2.4.9/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp	2015-04-08 17:11:16 +0000
@@ -32,6 +32,7 @@
 #include <ft2build.h>
 #include FT_TRUETYPE_TABLES_H
 #include <wtf/text/WTFString.h>
+#include <wtf/MathExtras.h>
 
 #if !PLATFORM(EFL)
 #include <gdk/gdk.h>
Comment 6 LRN 2015-04-21 04:12:08 PDT
(In reply to comment #5)
> I also patch the same file with this, to be able to build it in webkit-2.4
> branch at revision 182543:
> 
> ---
> webkitgtk-2.4.9.orig/Source/WebCore/platform/graphics/freetype/
> FontPlatformDataFreeType.cpp	2015-04-08 16:19:51 +0000
> +++
> webkitgtk-2.4.9/Source/WebCore/platform/graphics/freetype/
> FontPlatformDataFreeType.cpp	2015-04-08 17:11:16 +0000
> @@ -32,6 +32,7 @@
>  #include <ft2build.h>
>  #include FT_TRUETYPE_TABLES_H
>  #include <wtf/text/WTFString.h>
> +#include <wtf/MathExtras.h>
>  
>  #if !PLATFORM(EFL)
>  #include <gdk/gdk.h>

What error do you get when you compile without this change? Do you apply any other math-related patches (apart from attachment 250803 [details] or its equivalent) that might need this include?
Comment 7 Milan Crha 2015-04-21 06:06:27 PDT
This is all what I apply on top of webkitgtk3 2.4.4, which is mostly applicable on top of webkit-2.4 branch too (some chunks fail due to being applied already in the branch):

https://git.gnome.org/browse/evolution/tree/win32/patches/webkitgtk.patch
Comment 8 LRN 2015-04-21 06:26:01 PDT
(In reply to comment #7)
> This is all what I apply on top of webkitgtk3 2.4.4, which is mostly
> applicable on top of webkit-2.4 branch too (some chunks fail due to being
> applied already in the branch):
> 
> https://git.gnome.org/browse/evolution/tree/win32/patches/webkitgtk.patch

I haven't asked "what patches are you applying".

I've asked "what error do you get when this particular patch (from https://bugs.webkit.org/show_bug.cgi?id=143758#c5 ) is not applied"?

This should help greatly in pushing these patches upstream.
Comment 9 LRN 2015-04-21 06:48:36 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > This is all what I apply on top of webkitgtk3 2.4.4, which is mostly
> > applicable on top of webkit-2.4 branch too (some chunks fail due to being
> > applied already in the branch):
> > 
> > https://git.gnome.org/browse/evolution/tree/win32/patches/webkitgtk.patch
> 
> I haven't asked "what patches are you applying".
> 
> I've asked "what error do you get when this particular patch (from
> https://bugs.webkit.org/show_bug.cgi?id=143758#c5 ) is not applied"?
> 
> This should help greatly in pushing these patches upstream.

OK, sorry, that does answer the other half of my question. You don't seem to add any other math-related patches to FontPlatformDataFreeType, and changes to math headers are minimal. So there must be a codepath that is activated in your build but not in mine. Which brings us back to the first half of the question - what error are you getting?
Comment 10 Milan Crha 2015-04-23 10:41:09 PDT
(In reply to comment #6)
> > +#include <wtf/MathExtras.h>
> 
> What error do you get when you compile without this change?

Hmm, it builds without it too. I do not know why I added it there, possibly because I prefer explicit includes, than implicit.
Comment 11 Darin Adler 2015-04-23 11:56:43 PDT
(In reply to comment #10)
> (In reply to comment #6)
> > > +#include <wtf/MathExtras.h>
> > 
> > What error do you get when you compile without this change?
> 
> Hmm, it builds without it too. I do not know why I added it there, possibly
> because I prefer explicit includes, than implicit.

OK. We’d like you to follow our project coding style guideline of not doing explicit includes.
Comment 12 Carlos Garcia Campos 2015-05-18 23:40:37 PDT
Committed to 2.4 http://trac.webkit.org/changeset/184550