Bug 66323 - Rename FontGtk.cpp to FontPango.cpp
Summary: Rename FontGtk.cpp to FontPango.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-16 13:01 PDT by Rafael Antognolli
Modified: 2011-09-13 20:28 PDT (History)
9 users (show)

See Also:


Attachments
Move FontGtk.cpp to FontPango.cpp (33.22 KB, patch)
2011-08-23 12:53 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Move FontEfl.cpp to FontFreeType.cpp (6.84 KB, patch)
2011-08-23 12:54 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Add replacement functions for gdk ones. (12.55 KB, patch)
2011-08-23 12:54 PDT, Rafael Antognolli
mrobinson: review-
Details | Formatted Diff | Diff
Make GTK build link with PangoUtilities.cpp too. (1.39 KB, patch)
2011-08-23 12:55 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Move FontGtk.cpp to FontPango.cpp (34.07 KB, patch)
2011-09-02 08:06 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Move FontEfl.cpp to FontFreeType.cpp (7.49 KB, patch)
2011-09-02 08:07 PDT, Rafael Antognolli
mrobinson: review-
Details | Formatted Diff | Diff
Add replacement functions for gdk ones. (13.36 KB, patch)
2011-09-02 08:08 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Move FontGtk.cpp to FontPango.cpp (34.06 KB, patch)
2011-09-06 15:41 PDT, Rafael Antognolli
mrobinson: review+
Details | Formatted Diff | Diff
Make EFL build use the right file (1.86 KB, patch)
2011-09-06 15:43 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Add replacement functions for gdk ones. (13.45 KB, patch)
2011-09-06 15:44 PDT, Rafael Antognolli
mrobinson: review-
Details | Formatted Diff | Diff
Move FontGtk.cpp to FontPango.cpp (34.06 KB, patch)
2011-09-09 08:16 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff
Add replacement functions for gdk ones. (14.21 KB, patch)
2011-09-09 08:22 PDT, Rafael Antognolli
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Antognolli 2011-08-16 13:01:54 PDT
The content of this file is useful for the EFL port too, so I wouldn't like to just duplicate this code. Can we move it to FontPango.cpp or something else, so we can share the code?

I can send a patch making this change if necessary.

Additionally, the following two functions are giving me troubles to use this code:
 - gdk_cairo_region()
 - gdk_pango_layout_line_get_clip_region()

That's because they expect different parameters or return different types when using GTK 2 or GTK 3 api. Is there any cairo equivalent function to these two? And if not, can we just paste their code to FontPango.cpp (or whatever it gets to be named) and use it directly? They are two very small functions...
Comment 1 Martin Robinson 2011-08-16 13:08:45 PDT
For what it's worth, I plan to remove this file soon once the Harfbuzz-ng code has landed.
Comment 2 Rafael Antognolli 2011-08-16 13:42:10 PDT
(In reply to comment #1)
> For what it's worth, I plan to remove this file soon once the Harfbuzz-ng code has landed.

Hmm... ok, that's nice. bug 26741 seems to be quite old, is this code getting in anytime soon?

And anyway, can't we have the current code shared for now, so the EFL port will also have the complex text support fully functional?
Comment 3 Martin Robinson 2011-08-16 14:29:51 PDT
(In reply to comment #2)
> Hmm... ok, that's nice. bug 26741 seems to be quite old, is this code getting in anytime soon?

Harfbuzz-ng has been packaged and has shapers now, so we can make the switch as soon as I can coordinate with the Chromium team.

> And anyway, can't we have the current code shared for now, so the EFL port will also have the complex text support fully functional?

Sounds reasonable. Might want to move all of the pango specific files to the "pango" subdirectory as well.
Comment 4 Ryuan Choi 2011-08-16 16:38:44 PDT
(In reply to comment #0)
> Additionally, the following two functions are giving me troubles to use this code:
>  - gdk_cairo_region()
>  - gdk_pango_layout_line_get_clip_region()

For above functions,
How do you think about introducing CairoUtilities and PangoUtilities in efl/?
Comment 5 Rafael Antognolli 2011-08-17 14:46:40 PDT
(In reply to comment #4)
> (In reply to comment #0)
> > Additionally, the following two functions are giving me troubles to use this code:
> >  - gdk_cairo_region()
> >  - gdk_pango_layout_line_get_clip_region()
> 
> For above functions,
> How do you think about introducing CairoUtilities and PangoUtilities in efl/?

I think that idea is nice, but maybe just one of them for now. Will submit a patch for it.
Comment 6 Rafael Antognolli 2011-08-23 12:53:36 PDT
Created attachment 104895 [details]
Move FontGtk.cpp to FontPango.cpp
Comment 7 Rafael Antognolli 2011-08-23 12:54:05 PDT
Created attachment 104896 [details]
Move FontEfl.cpp to FontFreeType.cpp
Comment 8 Rafael Antognolli 2011-08-23 12:54:37 PDT
Created attachment 104898 [details]
Add replacement functions for gdk ones.
Comment 9 Rafael Antognolli 2011-08-23 12:55:47 PDT
Created attachment 104899 [details]
Make GTK build link with PangoUtilities.cpp too.
Comment 10 Rafael Antognolli 2011-08-23 12:58:21 PDT
I created the pango/PangoUtilities.cpp file, with the needed functions, and added another one to cairo/CairoUtilites.cpp. Of course I can move it to an efl/ specific directory, but I think it's useful for the GTK port too.

The patches are split based on what they do, but I can split them even more if necessary or just merge all of them (or open a bug for each of them).

What do you think about these patches? If they are ok, I'm going to send the final versions with respective changelogs.
Comment 11 Martin Robinson 2011-08-23 16:46:00 PDT
Comment on attachment 104898 [details]
Add replacement functions for gdk ones.

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

> Source/WebCore/platform/graphics/pango/FontPango.cpp:50
> +#ifdef GTK_API_VERSION_2
>  #include <gdk/gdk.h>
> +#else
> +#include "PangoUtilities.h"
> +#endif

This should be split into a separate block.

> Source/WebCore/platform/graphics/pango/FontPango.cpp:246
> +#ifdef GTK_API_VERSION_2
>          gdk_cairo_region(context, renderRegion);
> +#else
> +        appendRegionToCairoContext(context, renderRegion);
> +#endif

This isn't correct, because it will have GTK+ 3.x use appendRegionToCairoContext and I don't think that's what you want. If you can use PLATFORM(GTK) or USE(GTK), that would be better.

> Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:6
> +/*
> + * Copyright (C) 2000 Red Hat, Inc.
> + * Copyright (C) 2011 ProFUSION embedded systems
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public

Where is this code from. What license was it originally under?

> Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:37
> +    PangoRectangle logical_rect;
> +    PangoLayoutLine* line = pango_layout_iter_get_line_readonly(iter);
> +    cairo_region_t* clip_region = cairo_region_create();
> +    pango_layout_iter_get_line_extents(iter, NULL, &logical_rect);
> +    int baseline = pango_layout_iter_get_baseline(iter);
> +
> +    int i = 0;
> +    while (i < n_ranges) {
> +        int *pixel_ranges = NULL;
> +        int n_pixel_ranges = 0;

This code isn't in WebKit style.

> Source/WebCore/platform/graphics/pango/PangoUtilities.h:27
> +#include <cairo.h>
> +#include <pango/pango.h>
> +

Typically we try to avoid system includes in headers if possible.

> Source/WebCore/platform/graphics/pango/PangoUtilities.h:30
> +cairo_region_t* pangoLayoutLineGetClipRegion(PangoLayoutLine* line, int x_origin, int y_origin, const int* index_ranges, int n_ranges);

Usually for function names we try to have the verb first like, getClipRegionFromPangoLayoutLine.
Comment 12 Martin Robinson 2011-08-23 16:47:34 PDT
(In reply to comment #9)
> Created an attachment (id=104899) [details]
> Make GTK build link with PangoUtilities.cpp too.

Why is this patch necessary. PangoUtilities just contains a copy-and-pasted version of a GDK function.
Comment 13 Rafael Antognolli 2011-08-29 09:26:12 PDT
(In reply to comment #11)
> (From update of attachment 104898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104898&action=review
> 
> > Source/WebCore/platform/graphics/pango/FontPango.cpp:50
> > +#ifdef GTK_API_VERSION_2
> >  #include <gdk/gdk.h>
> > +#else
> > +#include "PangoUtilities.h"
> > +#endif
> 
> This should be split into a separate block.

Ok.

> > Source/WebCore/platform/graphics/pango/FontPango.cpp:246
> > +#ifdef GTK_API_VERSION_2
> >          gdk_cairo_region(context, renderRegion);
> > +#else
> > +        appendRegionToCairoContext(context, renderRegion);
> > +#endif
> 
> This isn't correct, because it will have GTK+ 3.x use appendRegionToCairoContext and I don't think that's what you want. If you can use PLATFORM(GTK) or USE(GTK), that would be better.

Ok, I thought it would be good for GTK to don't use Gdk functions too (I understood that you were moving away from it). Anyway, then I'm not changing the GTK parts of the code on the next patch.

> > Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:6
> > +/*
> > + * Copyright (C) 2000 Red Hat, Inc.
> > + * Copyright (C) 2011 ProFUSION embedded systems
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> 
> Where is this code from. What license was it originally under?

It's from gdk-pango, and the license is this one (LGPL-2). Do you mean that I should reference the source?

> > Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:37
> > +    PangoRectangle logical_rect;
> > +    PangoLayoutLine* line = pango_layout_iter_get_line_readonly(iter);
> > +    cairo_region_t* clip_region = cairo_region_create();
> > +    pango_layout_iter_get_line_extents(iter, NULL, &logical_rect);
> > +    int baseline = pango_layout_iter_get_baseline(iter);
> > +
> > +    int i = 0;
> > +    while (i < n_ranges) {
> > +        int *pixel_ranges = NULL;
> > +        int n_pixel_ranges = 0;
> 
> This code isn't in WebKit style.

Ok, sorry for that.

> > Source/WebCore/platform/graphics/pango/PangoUtilities.h:27
> > +#include <cairo.h>
> > +#include <pango/pango.h>
> > +
> 
> Typically we try to avoid system includes in headers if possible.

Ok, moving this to .cpp

> > Source/WebCore/platform/graphics/pango/PangoUtilities.h:30
> > +cairo_region_t* pangoLayoutLineGetClipRegion(PangoLayoutLine* line, int x_origin, int y_origin, const int* index_ranges, int n_ranges);
> 
> Usually for function names we try to have the verb first like, getClipRegionFromPangoLayoutLine.

Right.

Thanks a lot for reviewing, I'm updating the patch and sending it again soon.
Comment 14 Rafael Antognolli 2011-08-29 09:27:58 PDT
(In reply to comment #12)
> (In reply to comment #9)
> > Created an attachment (id=104899) [details] [details]
> > Make GTK build link with PangoUtilities.cpp too.
> 
> Why is this patch necessary. PangoUtilities just contains a copy-and-pasted version of a GDK function.

Because I thought it would be better to use the functions from PangoUtilities instead of GDK. Please don't consider this patch, as I told above I'm not changing GTK anymore on the next ones.
Comment 15 Rafael Antognolli 2011-09-02 08:06:12 PDT
Created attachment 106133 [details]
Move FontGtk.cpp to FontPango.cpp
Comment 16 Rafael Antognolli 2011-09-02 08:07:05 PDT
Created attachment 106136 [details]
Move FontEfl.cpp to FontFreeType.cpp
Comment 17 Rafael Antognolli 2011-09-02 08:08:57 PDT
Created attachment 106137 [details]
Add replacement functions for gdk ones.

Now I passed check-webkit-style on the patch, there's no complaint from it anymore. Please check if the mention about where the code comes from is correct. And thanks again for reviewing.
Comment 18 WebKit Review Bot 2011-09-02 10:12:23 PDT
Attachment 106133 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/pango/FontPango.cpp:337:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/pango/FontPango.cpp:355:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/pango/FontPango.cpp:426:  x_pos is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Rafael Antognolli 2011-09-02 10:51:56 PDT
(In reply to comment #18)
> Attachment 106133 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/platform/graphics/pango/FontPango.cpp:337:  Missing space after ,  [whitespace/comma] [3]
> Source/WebCore/platform/graphics/pango/FontPango.cpp:355:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> Source/WebCore/platform/graphics/pango/FontPango.cpp:426:  x_pos is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 3 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I didn't change this file on this patch, these errors were already there. Should I fix them too?
Comment 20 Martin Robinson 2011-09-06 09:37:48 PDT
(In reply to comment #19)

> I didn't change this file on this patch, these errors were already there. Should I fix them too?

Yes, please fix the other errors since you're touching the entire file.
Comment 21 Martin Robinson 2011-09-06 09:41:10 PDT
Comment on attachment 106136 [details]
Move FontEfl.cpp to FontFreeType.cpp

The internals of the GrahpicsContext are orthogonal to the font engine, thus I do not believe this patch makes sense. GTK+ uses Freetype for font layout and rendering, yet this file does not apply to it. I think this file is very particular to the EFL port.
Comment 22 Rafael Antognolli 2011-09-06 15:41:10 PDT
Created attachment 106503 [details]
Move FontGtk.cpp to FontPango.cpp

This patch contains the fixes so that webkit-check-style won't complain anymore.
Comment 23 Rafael Antognolli 2011-09-06 15:43:20 PDT
Created attachment 106505 [details]
Make EFL build use the right file

Now I don't rename the file anymore, just make the EFL build system use it in case we are not using Pango.
Comment 24 Rafael Antognolli 2011-09-06 15:44:22 PDT
Created attachment 106506 [details]
Add replacement functions for gdk ones.

Just rebased the patch.
Comment 25 Rafael Antognolli 2011-09-08 10:35:05 PDT
Thanks for the review!
What about the two other patches?
Comment 26 Martin Robinson 2011-09-08 16:07:30 PDT
Comment on attachment 106506 [details]
Add replacement functions for gdk ones.

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

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:89
> +    const int nBoxes = cairo_region_num_rectangles(region);

nBoxes -> rectCount?

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:94
> +        cairo_rectangle_int_t box;
> +        cairo_region_get_rectangle(region, i, &box);
> +        cairo_rectangle(to, box.x, box.y, box.width, box.height);
> +    }

box -> rect?

> Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:32
> +static cairo_region_t* getLineClipRegionFromLayoutIter(PangoLayoutIter *iter, int xOrigin, int yOrigin, const int *indexRanges, int nRanges)

The asterisk on indexRanges should be on the type name.

> Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:43
> +        int nPixelRanges = 0;

nPixelRange -> pixelRangeCount.

> Source/WebCore/platform/graphics/pango/PangoUtilities.cpp:71
> +cairo_region_t* getClipRegionFromPangoLayoutLine(PangoLayoutLine* line, int xOrigin, int yOrigin, const int* indexRanges, int nRanges)

Instead of two variables to get the origin, I suggest using a const IntPoint&. It looks like the origin is always 0, 0? Perhaps just ditch the parameters altogether. nRanges -> rangeCount. Please try to avoid not-well-known abbreviations.

> Source/WebCore/platform/graphics/pango/PangoUtilities.h:31
> +cairo_region_t* getClipRegionFromPangoLayoutLine(PangoLayoutLine*, int, int, const int*, int);

You need the variable names here for generic types (int and int*).
Comment 27 Martin Robinson 2011-09-08 16:08:28 PDT
Comment on attachment 106503 [details]
Move FontGtk.cpp to FontPango.cpp

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

> Source/WebCore/ChangeLog:29
> +        (WebCore::destroyPangoRegion):
> +        (WebCore::getPangoRegionExtents):
> +        (WebCore::utf16ToUtf8):
> +        (WebCore::convertUniCharToUTF8):
> +        (WebCore::setPangoAttributes):
> +        (WebCore::Font::canReturnFallbackFontsForComplexText):
> +        (WebCore::Font::canExpandAroundIdeographsInComplexText):
> +        (WebCore::drawGlyphsShadow):
> +        (WebCore::Font::drawComplexText):
> +        (WebCore::Font::drawEmphasisMarksForComplexText):
> +        (WebCore::getDefaultPangoLayout):
> +        (WebCore::Font::floatWidthForComplexText):
> +        (WebCore::Font::offsetForPositionForComplexText):
> +        (WebCore::Font::selectionRectForComplexText):
> +

You can just remove these lines, since you aren't filling them in.
Comment 28 Gyuyoung Kim 2011-09-08 21:52:05 PDT
Comment on attachment 106506 [details]
Add replacement functions for gdk ones.

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

> Source/cmake/FindPango.cmake:43
>  set(Pango_PROCESS_INCLUDES Pango_INCLUDE_DIR Freetype_INCLUDE_DIRS)

I think you need to fix FreetypeXXX with PangoXXX as well.
Comment 29 Rafael Antognolli 2011-09-09 08:16:46 PDT
Created attachment 106872 [details]
Move FontGtk.cpp to FontPango.cpp

Remove the empty description lines from ChangeLog.
Comment 30 Rafael Antognolli 2011-09-09 08:22:43 PDT
Created attachment 106873 [details]
Add replacement functions for gdk ones.

I changed the function to receive only indexRange, with no origin, as you requested, and no rangeCount since it's always one range too. Is it ok now?

Gyuyoung, I didn't understand your comment, what do you want me to change?
Comment 31 Martin Robinson 2011-09-09 12:17:47 PDT
Comment on attachment 106873 [details]
Add replacement functions for gdk ones.

Looks good.
Comment 32 WebKit Review Bot 2011-09-09 12:45:54 PDT
Comment on attachment 106873 [details]
Add replacement functions for gdk ones.

Rejecting attachment 106873 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
at 234.
Hunk #4 FAILED at 256.
Hunk #5 FAILED at 291.
Hunk #6 FAILED at 301.
6 out of 6 hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/pango/FontPango.cpp.rej
patching file Source/WebCore/platform/graphics/pango/PangoUtilities.cpp
patching file Source/WebCore/platform/graphics/pango/PangoUtilities.h
patching file Source/cmake/FindPango.cmake

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/9621969
Comment 33 WebKit Review Bot 2011-09-09 14:39:10 PDT
Comment on attachment 106872 [details]
Move FontGtk.cpp to FontPango.cpp

Clearing flags on attachment: 106872

Committed r94874: <http://trac.webkit.org/changeset/94874>
Comment 34 WebKit Review Bot 2011-09-09 15:05:09 PDT
Comment on attachment 106873 [details]
Add replacement functions for gdk ones.

Clearing flags on attachment: 106873

Committed r94876: <http://trac.webkit.org/changeset/94876>
Comment 35 WebKit Review Bot 2011-09-09 16:13:19 PDT
Comment on attachment 106505 [details]
Make EFL build use the right file

Clearing flags on attachment: 106505

Committed r94886: <http://trac.webkit.org/changeset/94886>
Comment 36 WebKit Review Bot 2011-09-09 16:13:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Gyuyoung Kim 2011-09-13 20:28:15 PDT
(In reply to comment #30)
> Created an attachment (id=106873) [details]

> Gyuyoung, I didn't understand your comment, what do you want me to change?

I file a bug for my comment. 
https://bugs.webkit.org/show_bug.cgi?id=68052