<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/109049/">http://git.reviewboard.kde.org/r/109049/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 20th, 2013, 1:06 a.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On February 20th, 2013, 1:11 a.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Code-wise the patch looks fine.</pre>
</blockquote>
<p>On February 20th, 2013, 11:06 a.m. UTC, <b>Marco Gulino</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On February 20th, 2013, 11:12 a.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On February 20th, 2013, 11:42 a.m. UTC, <b>Marco Gulino</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On February 21st, 2013, 12:47 a.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I had to add some indirection, as the "FaviconFromBlob" didn't have the database for querying the presence of our table. With the new patch we can query the sqlite database, and choose the right query if the "favicon_bitmaps" table is there, supporting both versions.
I also added the "order by" clause for multi-favicon support.
As for the maintainer, I don't know if there's an "official" maintainer for this runner in particular, but I rewrote most of it to support chrome, so probably it's just up to me :)</pre>
<br />
<p>- Marco</p>
<br />
<p>On February 19th, 2013, 10:42 p.m. UTC, Marco Gulino wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kde-workspace and Plasma.</div>
<div>By Marco Gulino.</div>
<p style="color: grey;"><i>Updated Feb. 19, 2013, 10:42 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plasma/generic/runners/bookmarks/faviconfromblob.cpp <span style="color: grey">(93c720c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109049/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>