Bug 46029 - [EFL] add pango support
Summary: [EFL] add pango support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 48116 49422
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-17 23:06 PDT by Ryuan Choi
Modified: 2010-11-14 12:15 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.96 KB, patch)
2010-09-18 00:56 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2010-09-26 02:37 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2010-11-11 21:06 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2010-09-17 23:06:10 PDT
In order to render complex text, we can choose pango as option.
Comment 1 Ryuan Choi 2010-09-18 00:56:42 PDT
Created attachment 68000 [details]
Patch
Comment 2 Rafael Antognolli 2010-09-20 11:28:03 PDT
Hi Ryuan,

Do you really need to include the gtk directory here?

+IF (WTF_USE_PANGO)
+  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
+    "${WEBCORE_DIR}/platform/graphics/gtk"
+    ${Pango_INCLUDE_DIRS}
+  )
+  LIST(APPEND WebKit_LIBRARIES
+    ${Pango_LIBRARIES}

And maybe you should also remove FIND_PACKAGE(Freetype) from the global scope and just use it when using that backend. Or is it necessary by pango too?

Other than that, the patch seems nice.
Comment 3 Ryuan Choi 2010-09-25 23:45:02 PDT
(In reply to comment #2)
> Hi Ryuan,
> 
> Do you really need to include the gtk directory here?
> 
> +IF (WTF_USE_PANGO)
> +  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> +    "${WEBCORE_DIR}/platform/graphics/gtk"
> +    ${Pango_INCLUDE_DIRS}
> +  )
> +  LIST(APPEND WebKit_LIBRARIES
> +    ${Pango_LIBRARIES}
> 

sorry about late answer.
now, Pango related implementation was in platform/graphics/gtk.
unless moving it, we should include it.

> And maybe you should also remove FIND_PACKAGE(Freetype) from the global scope and just use it when using that backend. Or is it necessary by pango too?
> 
> Other than that, the patch seems nice.

It's my mistake. I'll move it.
Comment 4 Ryuan Choi 2010-09-26 02:37:14 PDT
Created attachment 68844 [details]
Patch
Comment 5 Ryuan Choi 2010-09-26 02:38:31 PDT
(In reply to comment #4)
> Created an attachment (id=68844) [details]
> Patch

I moved FIND_PACKAGE(Freetype).
Comment 6 Rafael Antognolli 2010-10-21 06:29:38 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=68844) [details] [details]
> > Patch
> 
> I moved FIND_PACKAGE(Freetype).

(In reply to comment #4)
> Created an attachment (id=68844) [details]
> Patch

(In reply to comment #3)
> (In reply to comment #2)
> > Hi Ryuan,
> > 
> > Do you really need to include the gtk directory here?
> > 
> > +IF (WTF_USE_PANGO)
> > +  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> > +    "${WEBCORE_DIR}/platform/graphics/gtk"
> > +    ${Pango_INCLUDE_DIRS}
> > +  )
> > +  LIST(APPEND WebKit_LIBRARIES
> > +    ${Pango_LIBRARIES}
> > 
> 
> sorry about late answer.
> now, Pango related implementation was in platform/graphics/gtk.
> unless moving it, we should include it.

Ok, I understand your point, but would be better to have these files inside somewhere else (maybe platform/graphics/pango?).

Anyway, I think it's ok for now.
Comment 7 Rafael Antognolli 2010-10-21 06:31:12 PDT
Hi mrobinson,

Do you think there's any problem moving these files to some non-gtk specific folder? Currently they don't use anything gtk related...
Comment 8 Martin Robinson 2010-10-21 07:59:33 PDT
I think it's a great idea actually. A followup or pre-patch could:

1. Move all Pango backend files to a "pango" subdirectory.
2. Move all FreeType backend files to a "freetype" subdirectory.
3. Rename all inconsistently named FreeType backend files.

I believe that would allow us to remove at least some of the #ifdef magic from FontPlatformData.h.
Comment 9 Ryuan Choi 2010-11-11 21:06:55 PST
Created attachment 73698 [details]
Patch
Comment 10 Ryuan Choi 2010-11-13 21:19:24 PST
(In reply to comment #9)
> Created an attachment (id=73698) [details]
> Patch

Finally this patch can be applied because related bugs was fixed.

could you review this, if possible?
Comment 11 WebKit Commit Bot 2010-11-14 12:15:10 PST
Comment on attachment 73698 [details]
Patch

Clearing flags on attachment: 73698

Committed r71987: <http://trac.webkit.org/changeset/71987>
Comment 12 WebKit Commit Bot 2010-11-14 12:15:16 PST
All reviewed patches have been landed.  Closing bug.