[Webkit-unassigned] [Bug 66323] Rename FontGtk.cpp to FontPango.cpp

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66323





--- Comment #13 from Rafael Antognolli <antognolli at profusion.mobi>  2011-08-29 09:26:12 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.