| Summary: | Rename FontGtk.cpp to FontPango.cpp | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rafael Antognolli <antognolli+webkit> | ||||||||||||||||||||||||||
| Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | antognolli+webkit, gustavo, gyuyoung.kim, leandro, lucas.de.marchi, mrobinson, rakuco, ryuan.choi, webkit.review.bot | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Rafael Antognolli
2011-08-16 13:01:54 PDT
For what it's worth, I plan to remove this file soon once the Harfbuzz-ng code has landed. (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? (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. (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/? (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. Created attachment 104895 [details]
Move FontGtk.cpp to FontPango.cpp
Created attachment 104896 [details]
Move FontEfl.cpp to FontFreeType.cpp
Created attachment 104898 [details]
Add replacement functions for gdk ones.
Created attachment 104899 [details]
Make GTK build link with PangoUtilities.cpp too.
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 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. (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. (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. (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. Created attachment 106133 [details]
Move FontGtk.cpp to FontPango.cpp
Created attachment 106136 [details]
Move FontEfl.cpp to FontFreeType.cpp
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.
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.
(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? (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 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.
Created attachment 106503 [details]
Move FontGtk.cpp to FontPango.cpp
This patch contains the fixes so that webkit-check-style won't complain anymore.
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.
Created attachment 106506 [details]
Add replacement functions for gdk ones.
Just rebased the patch.
Thanks for the review! What about the two other patches? 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 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 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. Created attachment 106872 [details]
Move FontGtk.cpp to FontPango.cpp
Remove the empty description lines from ChangeLog.
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 on attachment 106873 [details]
Add replacement functions for gdk ones.
Looks good.
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 on attachment 106872 [details] Move FontGtk.cpp to FontPango.cpp Clearing flags on attachment: 106872 Committed r94874: <http://trac.webkit.org/changeset/94874> 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 on attachment 106505 [details] Make EFL build use the right file Clearing flags on attachment: 106505 Committed r94886: <http://trac.webkit.org/changeset/94886> All reviewed patches have been landed. Closing bug. (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 |