<table><tr><td style="">dfaure marked 2 inline comments as done.<br />dfaure added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D29385">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29385#inline-168106">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljob.cpp:261</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I know what you always say when I say what I always say, but why not just always stat/mimetype job?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">LOL we're like an old couple, the explicit discussion doesn't actually need to happen anymore ;)</p>

<p style="padding: 0; margin: 8px;">OK for everyone else, we're debating whether it's ok to use blocking local-file I/O like QFile or QMimeDatabase which reads from the file.</p>

<p style="padding: 0; margin: 8px;">Of course less code paths is a good thing for maintenance, but it seems *so* overkill to make two calls to a kioslave just to find out the mimetype of a file.... My main counter argument is performance.</p>

<p style="padding: 0; margin: 8px;">For the whole OpenUrlJob until the mimetype is found:</p>

<p style="padding: 0; margin: 8px;">With KIO</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
 0.29 msecs per iteration (total: 75, iterations: 256)</pre></div>

<p style="padding: 0; margin: 8px;">With the local-file optimization</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
 0.0986 msecs per iteration (total: 101, iterations: 1024)</pre></div>

<p style="padding: 0; margin: 8px;">That's 3 times faster. Admittedly this is not the kind of things people do in a loop.</p>

<p style="padding: 0; margin: 8px;">Well, OK, if nobody objects I can remove the local-files fast paths.<br />
0.2ms is nothing when lharming 2018 PG Demi Lovato Ashley Tisdale Avril Lavigne-02052020.mpgaunching an app, or even when opening a URL in a browser.</p>

<p style="padding: 0; margin: 8px;">[More context: QMimeDatabase *might* determine the mimetype from just the extension, in which case no I/O happens and we could do that here, or it might need to look into the contents of the file. We can ask it to not do that but then the mimetype determination will be less good, for some mimetypes; and we can't ask it if we would get better information by looking at content, so there's no way to split up the work between here and the kioslave. It's "quick search" vs "full search", not phase 1, phase 2.]</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29385#inline-168109">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljob.cpp:447</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">was or should be?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Hehe, see kdesktopfileactions.cpp:141 ;-)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29385#inline-168110">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openurljob.cpp:506</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">While at it, can we clean up/unify those texts? Sometimes it puts the file on a new line, sometimes it's bold, sometines in quotes, etc. Generally I wouldn't really want any HTML formatting in there.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Completely agree. I removed most of the HTML formatting when grabbing the error messages from KRun, looks like I forgot this one.</p>

<p style="padding: 0; margin: 8px;">So now it's quotes everywhere.</p>

<p style="padding: 0; margin: 8px;">But indeed sometimes the filename is on its own line. Should I just use quotes everywhere?</p>

<p style="padding: 0; margin: 8px;">I guess the problem is when the filename (with full path) is very long.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D29385">https://phabricator.kde.org/D29385</a></div></div><br /><div><strong>To: </strong>dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, svuorela<br /><strong>Cc: </strong>feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns<br /></div>