Review Request: remove KEncodingDetector from tier1/kcodecs

[David Faure]

> On July 1, 2012, 9:14 a.m., David Faure wrote:
> > 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).
> 
> Hui Ni wrote:
>     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.
>

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.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105392/#review15285
-----------------------------------------------------------


On July 9, 2012, 6:27 a.m., Hui Ni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105392/
> -----------------------------------------------------------
> 
> (Updated July 9, 2012, 6:27 a.m.)
> 
> 
> Review request for kdelibs, Kevin Ottens and David Faure.
> 
> 
> Description
> -------
> 
> 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 ...
> 
> 
> Diffs
> -----
> 
>   tier1/kcodecs/src/kencodingprober.h da4b958 
>   tier1/kcodecs/src/kencodingprober.cpp 42beac0 
>   tier1/kcoreaddons/src/text/kstringhandler.h b9b6c2e 
>   khtml/khtmlpart_p.h d46d254 
>   khtml/test_regression.cpp 8b2fa15 
>   khtml/khtml_part.h 340ece1 
>   khtml/khtml_part.cpp 223721c 
>   kdeui/actions/kcodecaction.cpp 97a8a90 
>   kdeui/actions/kcodecaction.h 627d770 
> 
> Diff: http://git.reviewboard.kde.org/r/105392/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hui Ni
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120710/e3f8599d/attachment.htm>