Bug 66323 – Rename FontGtk.cpp to FontPango.cpp

• Component: Text
• Status: RESOLVED FIXED

[Rafael Antognolli]

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...

[Martin Robinson]

For what it's worth, I plan to remove this file soon once the Harfbuzz-ng code has landed.

[Rafael Antognolli]

(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?

[Martin Robinson]

(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.

[Ryuan Choi]

(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/?

[Rafael Antognolli]

(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.

[Rafael Antognolli]

Created attachment 104895 [details]
Move FontGtk.cpp to FontPango.cpp

[Rafael Antognolli]

Created attachment 104896 [details]
Move FontEfl.cpp to FontFreeType.cpp

[Rafael Antognolli]

Created attachment 104898 [details]
Add replacement functions for gdk ones.

[Rafael Antognolli]

Created attachment 104899 [details]
Make GTK build link with PangoUtilities.cpp too.

[Rafael Antognolli]

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.

[Martin Robinson]

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.

[Martin Robinson]

(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.

[Rafael Antognolli]

(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.

[Rafael Antognolli]

(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.

[Rafael Antognolli]

Created attachment 106133 [details]
Move FontGtk.cpp to FontPango.cpp

[Rafael Antognolli]

Created attachment 106136 [details]
Move FontEfl.cpp to FontFreeType.cpp

[Rafael Antognolli]

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.

[WebKit Review Bot]

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.

[Rafael Antognolli]

(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?

[Martin Robinson]

(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.

[Martin Robinson]

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.

[Rafael Antognolli]

Created attachment 106503 [details]
Move FontGtk.cpp to FontPango.cpp

This patch contains the fixes so that webkit-check-style won't complain anymore.

[Rafael Antognolli]

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.

[Rafael Antognolli]

Created attachment 106506 [details]
Add replacement functions for gdk ones.

Just rebased the patch.

[Rafael Antognolli]

Thanks for the review!
What about the two other patches?

[Martin Robinson]

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*).

[Martin Robinson]

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.

[Gyuyoung Kim]

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.

[Rafael Antognolli]

Created attachment 106872 [details]
Move FontGtk.cpp to FontPango.cpp

Remove the empty description lines from ChangeLog.

[Rafael Antognolli]

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?

[Martin Robinson]

Comment on attachment 106873 [details]
Add replacement functions for gdk ones.

Looks good.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 106872 [details]
Move FontGtk.cpp to FontPango.cpp

Clearing flags on attachment: 106872

Committed r94874: <http://trac.webkit.org/changeset/94874>

[WebKit Review Bot]

Comment on attachment 106873 [details]
Add replacement functions for gdk ones.

Clearing flags on attachment: 106873

Committed r94876: <http://trac.webkit.org/changeset/94876>

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

(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