<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/105392/">http://git.reviewboard.kde.org/r/105392/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 1st, 2012, 9:14 a.m., <b>David Faure</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;">Great work!
I noticed a few things though:
* in your initial analysis, you said KEncodingDetector had code to detect the encoding from the html/xml and http headers. Is this functionality lost during this port, in khtml?
* you deleted the KEncodingDetector unittest, but apparently KEncodingProber has no unittests. Could you port the old unittest to KEncodingProber, in order to at least validate it a bit, and ideally could you extend the unittest for things that only the prober can do?
* rather than deleting KEncodingDetector, please move it to kde4support (in a separate commit, possibly), so that source compatibility is preserved (just in case there is some application out there that uses it).</pre>
</blockquote>
<p>On July 9th, 2012, 6:42 a.m., <b>Hui Ni</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;">hi
* yes, the funtionality was lost during this port. I've then reverted the changes in khtml.
* I will create a set of unittests for KEncodingProber. I will create a new review request for this task once done.
I have supposed we shall move KEncodingDetector into khtml. khtml is the only heavy user of it I have known.
If we want to keep api, I am fine with moving to kde4support.
</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;">Well, there are other users, see http://lxr.kde.org/ident?i=KEncodingDetector (plus potential users outside of the KDE repositories), but indeed they could be ported to KEncodingProber.
This is difficult. We can't both "provide it in kde4support for easier porting" and "having the code in khtml because that one cannot be ported".
OK then, let's move it into khtml.</pre>
<br />
<p>- David</p>
<br />
<p>On July 9th, 2012, 6:27 a.m., Hui Ni wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Kevin Ottens and David Faure.</div>
<div>By Hui Ni.</div>
<p style="color: grey;"><i>Updated July 9, 2012, 6:27 a.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, this is my first contribution to kde frameworks.
This patch removes KEncodingDetector class from tier1/kcodecs and ports all related bits in kdelibs to KEncodingProber class.
proposal raised at http://mail.kde.org/pipermail/kde-frameworks-devel/2012-June/000738.html
KEncodingDetector is actually KEncodingProber + encoding priority list + QTextDecoder, and the later two things are used only in khtml.
things removed:
enum KEncodingDetector::EncodingChoiceSource -> removed
KEncodingDetector::setEncoding -> removed, implement it in khtml code
KEncodingDetector::encodingChoiceSource -> removed
KEncodingProber::encodingName -> removed, avoid potential memory leak
one to one changes:
enum KEncodingDetector::AutoDetectScript -> enum KEncodingProber::ProberType
KEncodingDetector::encoding -> KEncodingProber::encoding
KEncodingDetector::visuallyOrdered -> check KEncodingProber::encoding is hebrew or not
KEncodingDetector::autoDetectLanguage -> KEncodingProber::proberType
KEncodingDetector::setAutoDetectLanguage -> KEncodingProber::setProberType
KEncodingDetector::decode -> KEncodingProber::feed + QTextCodec::codecForName(prober.encoding())->toUnicode
KEncodingDetector::decodeWithBuffering -> KEncodingProber::feed, feed, feed + check KEncodingProber::state + QTextCodec::codecForName(prober.encoding())->makeDecoder
KEncodingDetector::decodedInvalidCharacters -> QTextCodec::codecForName(prober.encoding())->makeDecoder + hasFailure
KEncodingDetector::resetDecoder -> KEncodingProber::reset
KEncodingDetector::flush -> KEncodingProber::feed, feed, feed + QTextCodec::codecForName(prober.encoding())->toUnicode
KEncodingDetector::scriptForName -> KEncodingProber::proberTypeForName
KEncodingDetector::nameForScript -> KEncodingProber::nameForProberType
KEncodingDetector::hasAutoDetectionForScript -> if (proberType == KEncodingProber::None) ... else ...
</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>tier1/kcodecs/src/kencodingprober.h <span style="color: grey">(da4b958)</span></li>
<li>tier1/kcodecs/src/kencodingprober.cpp <span style="color: grey">(42beac0)</span></li>
<li>tier1/kcoreaddons/src/text/kstringhandler.h <span style="color: grey">(b9b6c2e)</span></li>
<li>khtml/khtmlpart_p.h <span style="color: grey">(d46d254)</span></li>
<li>khtml/test_regression.cpp <span style="color: grey">(8b2fa15)</span></li>
<li>khtml/khtml_part.h <span style="color: grey">(340ece1)</span></li>
<li>khtml/khtml_part.cpp <span style="color: grey">(223721c)</span></li>
<li>kdeui/actions/kcodecaction.cpp <span style="color: grey">(97a8a90)</span></li>
<li>kdeui/actions/kcodecaction.h <span style="color: grey">(627d770)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105392/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>