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