JDS Patch Review
(Note: After receiving two comments and rereading my previous post, I removed it in accordance to CoC
which I have volunteerly signed. Well, we're cool now.)
Seeing people commenting on JDS patches
for their packages, I couldn't help but checking pango, cairo, and vte patches they ship. Having reviewed them, I thought I should provide some feedback :). I'm posting here, hoping that other GNOME hackers pick this up too. Later we can move on to collectively review other vendors' patches too, and help the over-busy maintainers push the right patches upstream and drop/fix the wrong ones. Not every project has a sebuild, right? ;) Brian Cameron has been doing a great job doing that for opensolaris already, but there's more room.
- pango-01-fullwidth-space.diff: I'm interested to have a Pango bug requesting what this tries to fix, as I'm not sure about what the patch tries to achieve. Other than that, both changes in the patch are wrong. The first one is adding an entry for 0x0020 after 0x2000 in a table that is supposed to remain sorted. Moreover, characters below 0x2000 are not looked up in that table at all, that's why the table starts at 0x2000. The other change too adds an entry, for 0x3000, but does not remove the old entry for 0x3000. Both can confuse the bsearch routine.
- pango-02-pua.diff: Is a very weird attempt that I believe is trying to fix bug 443206 that was fixed upstream two weekes ago. Other than that, the patch touches public pango API, something that should be avoided at all costs. It adds a new enum value PANGO_SCRIPT_OTHER that makes no sense. Just use the upstream patch please, or wait for next stable release.
- pango-03-no-xrender.diff: Removes check for xrender from pango's configure check for pangoxft. If that check is not necessary, and there's a use in compiling without it, please file a bug agaist Pango and I'll be more than happy to commit it upstream.
- cairo-01-uninstalled-pc.diff: Submitted to upstream bugzilla already. Only if someone could tell me how exactly the -uninstalled pkg-config files are used...
- cairo-02-8bit-fix.diff: This works around the infamous cairo 8-bit issues. The patch has been posted upstream. Carl has been working on getting this fixed, but it's not easy, and a clean fix is even harder.
- cairo-03-full-hinting.diff: I'm interested to hear what the patch tries to achieve. I assume this relies on custom changes to fontconfig.
- cairo-04-mediaLib.diff: In the works in upstream bugzilla already.
- cairo-05-remove-buggy_repeat.diff: Committed upstream already.
- vte-01-fcconfig.diff: According to Owen, patch is clearly wrong, and I agree. The issue is more of a fontconfig misconfiguration IMO, but not enough detail is given, so I can't test. Needs someone debugging what's going on, instead of blindly cut it out.
- vte-03-cut-copy-paste-handle.diff: The patch looks interesting. I don't remember seeing any upstream report about it, and I'm not sure what problem it tries to solve, but I like to know!
- vte-04-selection-perf-improve.diff: Well, this was submitted upstream and rejected. It removes vte's regular expression matching functionality and hardcodes hand-written matching of common URL schemes. There are many other ways to improve vte's selection performance without sacrificing functionality. Needs someone to profile/debug it.
All in all, very good for cairo, but can do better for vte and pango.
Labels: cairo, jds, pango, vte