> On Feb. 20, 2013, 1:06 a.m., Àlex Fiestas wrote: > > I guess this will break compatibility with old versions then? if so, do you > > know from which version will it work? > > If this was not long ago, can we check the version and keep supporting the > > old and the new one? > > Àlex Fiestas wrote: > Code-wise the patch looks fine. > > Marco Gulino wrote: > You're right, there's a compatibility break, and we can't even know > starting from which version, since it's an internal database not documented > (I might try looking around, but I don't expect to find much). > If it makes sense, I might conditionally choose the right query depending > on the schema found (assuming the new table didn't exist in the previous > version, which makes sense). > I also noticed that the new table is meant to support multiple icon > sizes, so I might add an order by clause to choose the wider one as default. > > Àlex Fiestas wrote: > To be clear, personally I'd be fine if we drop support for some old > Chromium version, thing is those guys bump versions like crazy (today we are > at chromium 20 and tomorrow we are at 30), so we need to know how old is this > change to be able to decide if we want to drop support for older versions. > > Marco Gulino wrote: > Hopefully not :P > I think I found the commit date on chromium git server: > > http://git.chromium.org/gitweb/?p=git/chromium.git;a=commit;h=eded41c5c28bf94e128b6ab4cdd8030fb8b6d2f3 > The commit date is august 2012, so maybe it's a september/october release?
For 4.11 we can drop it, but not for 4.10. Ideal solution will be supporting both though, this seems like a recent change. Also, this up to the maintainer (I'm not) but supporting both will always be preferred :p - Àlex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/#review27760 ----------------------------------------------------------- On Feb. 19, 2013, 10:42 p.m., Marco Gulino wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109049/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2013, 10:42 p.m.) > > > Review request for kde-workspace and Plasma. > > > Description > ------- > > As reported by an user ( https://bugs.kde.org/show_bug.cgi?id=305633 ), > chrome bookmarks database changed, and favicon wasn't shown anymore (not > either the default "star" icon). > I added the functionality back, and added a safety guard for displaying the > default icon if something similar happens again. > (note: I didn't set the "bugs" field here, since that bug was already closed, > and was about something else). > > > Diffs > ----- > > plasma/generic/runners/bookmarks/faviconfromblob.cpp 93c720c > > Diff: http://git.reviewboard.kde.org/r/109049/diff/ > > > Testing > ------- > > with chrome as default browser, install the plugin, restart krunner, type > "bookmarks" to view all bookmarks: proper favicon is shown. > Removing the database query fix, but leaving the safety guard, and cleaning > favicon cache (to have again a "broken" feature case), the default icon is > shown. > > > Thanks, > > Marco Gulino > >
