<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/111050/">http://git.reviewboard.kde.org/r/111050/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In addition to the issues below, I suspect it changes behavior with HTTP urls. But you'll find that out when actually running kmimetypetest.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111050/diff/1/?file=150664#file150664line324" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">324</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n">path</span><span class="p">.</span><span class="n">endsWith</span><span class="p">(</span><span class="n">QLatin1Char</span><span class="p">(</span><span class="sc">'/'</span><span class="p">)))</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The URL for a folder doesn't necessarily end with a slash.
You can use the mode_t argument to detect folders, when it's set.
If it's not set, you have to stat(), no other choice.
Did you actually run kmimetypetest BTW? I expect a number of regressions, from this patch. I won't accept a patch which introduces regressions, however.
(BTW all this code is dead code, I rewrote all of it for Qt5's QMimeType...)
</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111050/diff/1/?file=150664#file150664line874" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetype.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">int KMimeTypePrivate::serviceOffersOffset() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">806</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">iconNameCache</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">m_strName</span><span class="p">,</span> <span class="n">result</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">858</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">icon</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">WTH? Where did the support for icons specified in the XML file, go?
Sure, you can make things faster by removing features... let's go back to Atari ST whose OS ran on 8 MHz CPUs?</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111050/diff/1/?file=150665#file150665line646" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetyperepository.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void KMimeTypeRepository::checkEssentialMimeTypes()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">637</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">KMimeType</span><span class="o">::</span><span class="n">mimeType</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span class="s">"inode/directory"</span><span class="p">)))</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">646</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">KMimeType</span><span class="o">::</span><span class="n">mimeType</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span class="s">"inode/directory"</span><span class="p">)<span class="hl">,</span></span><span class="hl"> </span><span class="n"><span class="hl">KMimeType</span></span><span class="o"><span class="hl">::</span></span><span class="n"><span class="hl">DontResolveAlias</span></span><span class="p">))</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Adding DontResolveAlias to all lookups in this method is an interesting idea. Probably OK for inode/directory. But probably not for the application/* mimetypes: if someone changes freedesktop.org.xml in the future to make e.g. application/x-shellscript an alias rather than a canonical mime name, it will break this code. (And such changes do happen regularly). So I'd rather we don't do that (this code is only run once anyway).</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111050/diff/1/?file=150665#file150665line690" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kmimetyperepository.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KMimeType::Ptr KMimeTypeRepository::defaultMimeTypePtr()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">690</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">KMimeType</span><span class="o">::</span><span class="n">Ptr</span> <span class="n">KMimeTypeRepository</span><span class="o">::</span><span class="n">defaultMimeTypePtrFast</span><span class="p">()</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Basically the same as defaultMimeTypePtr(), but with race conditions for lack of the mutex, and without error handling in case the mimetype definitions are not found? For code that is run only once, what's the point?</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On June 16th, 2013, 4:42 p.m. UTC, Mark Gaiser 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 kdelibs, David Faure and Frank Reininghaus.</div>
<div>By Mark Gaiser.</div>
<p style="color: grey;"><i>Updated June 16, 2013, 4: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;">Hi,
I've recently seen Frank Reininghaus do his best in speeding up the rendering in dolphin with regards to the app icons. And trying to prevent icon flickering between "unknown" and the actual icon.
While reading his posts on the mailing list i was beginning to wonder: "is fast mime detection actually fast"? While it was certainly faster then "slow" mime detection, it still didn't really seem fast to me. A small benchmark app hat ran fast mime detection in /usr/bin took ~40ms to complete. That's for just 2656 items.
After quite a bit of profiling i managed to to bring the duration down from ~40ms to ~3ms sometimes ~4ms. That's well over 10x faster.
Mime detection by extension (like "file.tar.bz") is done as follows:
file.tar.bz
Loop - find first dot
- "tar.gz"
if that matches a mime type then it's returned if it doesn't then it proceeds on to the next dot:
- next dot: "gz"
if that matches.. return.
Otherwise it will return the default mime type.
I am getting an inconsistency. Using the unpatched fast mime detection on a file like: "test.tar.gz" gets detected as "application-x-compressed-tar" where the patched version detects it as "application-gzip". The slow and detailed mime detection detects the same file as "application-x-compressed-tar". What should it be? application-gzip or application-x-compressed-tar?
Note: This improved detection does expect folders to end with a "/". Otherwise they will be detected as application-octet-stream (the default). But i think this is common sense to let folders end with a "/". If any apps that don't do that, they should fix it i suppose.
Best thing, it's all internal and private api change. No public function is changed.
All feedback is welcome! If possible, i would like to put this in KDE 4.11.</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;">Tested this using just output comparison between the old version and the new implementation. It works just fine.</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>kdecore/services/kmimetype.h <span style="color: grey">(bc35bcf)</span></li>
<li>kdecore/services/kmimetype.cpp <span style="color: grey">(d748523)</span></li>
<li>kdecore/services/kmimetyperepository.cpp <span style="color: grey">(f56f48e)</span></li>
<li>kdecore/services/kmimetyperepository_p.h <span style="color: grey">(e1d2389)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111050/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>