Re: Review Request: dataprotocol: make charset recoding work

[Commit Hook]

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


This review has been submitted with commit 
7fce249425be95ce3b4e47e5c2393e64d793c13f by Rolf Eike Beer to branch KDE/4.8.

- Commit Hook


On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104874/
> ---
> 
> (Updated May 6, 2012, 6:14 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> This reworks the code that works with different character sets to actually do 
> the right thing (tm).
> 
> 
> Diffs
> -
> 
>   kio/kio/dataprotocol.cpp e614476 
>   kio/tests/dataprotocoltest.cpp c8df132 
> 
> Diff: http://git.reviewboard.kde.org/r/104874/diff/
> 
> 
> Testing
> ---
> 
> -build whole kdelibs
> -added more testcases from http://greenbytes.de/tech/tc/datauri
> -all dataprotocol tests pass
> 
> 
> Thanks,
> 
> Rolf Eike Beer
> 
>

Re: Review Request: dataprotocol: make charset recoding work

[Rolf Eike Beer]

> On May 9, 2012, 12:04 p.m., David Faure wrote:
> > kio/kio/dataprotocol.cpp, line 71
> > 
> >
> > Not sure the comment is still correct. "decoded"? It's not, anymore, 
> > right? It's just extracted? Or is it even the full initial URL?

Yes, it is still correct. This is after the percent encoding has been decoded.


> On May 9, 2012, 12:04 p.m., David Faure wrote:
> > kio/kio/dataprotocol.cpp, line 277
> > 
> >
> > Why .toUtf8()? Are we sure that this is what the receiver of the data 
> > will use, for decoding?
> > (I mean in the real case of an application getting the data, not in the 
> > unittest)

I have used the new testcases on the old code and compared with the online 
tests. The only cases where old code was right and the string afterwards was 
displayed correctly was when it returned UTF8 strings. So returning UTF8 is the 
right thing here IMHO.


> On May 9, 2012, 12:04 p.m., David Faure wrote:
> > kio/kio/dataprotocol.cpp, line 279
> > 
> >
> > This comment isn't applicable anymore, it was the justification for 
> > toLocal8Bit().

Correct, will delete that.


- Rolf Eike


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


On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104874/
> ---
> 
> (Updated May 6, 2012, 6:14 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> This reworks the code that works with different character sets to actually do 
> the right thing (tm).
> 
> 
> Diffs
> -
> 
>   kio/kio/dataprotocol.cpp e614476 
>   kio/tests/dataprotocoltest.cpp c8df132 
> 
> Diff: http://git.reviewboard.kde.org/r/104874/diff/
> 
> 
> Testing
> ---
> 
> -build whole kdelibs
> -added more testcases from http://greenbytes.de/tech/tc/datauri
> -all dataprotocol tests pass
> 
> 
> Thanks,
> 
> Rolf Eike Beer
> 
>

Re: Review Request: dataprotocol: make charset recoding work

[David Faure]

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



kio/kio/dataprotocol.cpp


Not sure the comment is still correct. "decoded"? It's not, anymore, right? 
It's just extracted? Or is it even the full initial URL?



kio/kio/dataprotocol.cpp


Why .toUtf8()? Are we sure that this is what the receiver of the data will 
use, for decoding?
(I mean in the real case of an application getting the data, not in the 
unittest)



kio/kio/dataprotocol.cpp


This comment isn't applicable anymore, it was the justification for 
toLocal8Bit().


- David Faure


On May 6, 2012, 6:14 p.m., Rolf Eike Beer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104874/
> ---
> 
> (Updated May 6, 2012, 6:14 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> This reworks the code that works with different character sets to actually do 
> the right thing (tm).
> 
> 
> Diffs
> -
> 
>   kio/kio/dataprotocol.cpp e614476 
>   kio/tests/dataprotocoltest.cpp c8df132 
> 
> Diff: http://git.reviewboard.kde.org/r/104874/diff/
> 
> 
> Testing
> ---
> 
> -build whole kdelibs
> -added more testcases from http://greenbytes.de/tech/tc/datauri
> -all dataprotocol tests pass
> 
> 
> Thanks,
> 
> Rolf Eike Beer
> 
>