Eliminate one copy of replace_char() function.

Eliminate one copy of replace_char() function.

Instead of duplicating it in skimage_main.cpp, reuse the utility
function we have already in picture_utils.h.

BUG=skia:2651
TEST=./gyp_skia && ninja -C out/Debug dm
$ out/Debug/dm --tests --gms=false -m String_Replace
R=reed@google.com

[tfarina, 2014-10-11 21:11:23]

ptal.

[tfarina, 2014-10-14 14:05:05]

Ping?

[tfarina, 2014-10-15 14:01:21]

tfarina@chromium.org changed reviewers:
+ robertphillips@google.com
- mtklein@google.com

[tfarina, 2014-10-15 14:01:21]

Robert, could you review this to me?

Thanks,

[reed1, 2014-10-15 14:13:07]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-15 14:13:07]

https://codereview.chromium.org/646213002/diff/1/tools/picture_utils.h
File tools/picture_utils.h (left):

https://codereview.chromium.org/646213002/diff/1/tools/picture_utils.h#oldcode30
tools/picture_utils.h:30: void replace_char(SkString* str, const char oldChar,
const char newChar);
Hey, this looks semantically dangerous. SkString is UTF8 encoded, so we can't
safely replace (e.g. 0x85) with (e.g. 0x6F). I suggest if we want a "replace"
function, it should operate on SkUnichar types.

[robertphillips, 2014-10-15 14:13:23]

robertphillips@google.com changed reviewers:
- reed@google.com

[robertphillips, 2014-10-15 14:13:24]

I think we should skip this CL an move right to having this as a helper in
SkString.h.

[tfarina, 2014-10-15 14:19:59]

On 2014/10/15 14:13:24, robertphillips wrote:
> I think we should skip this CL an move right to having this as a helper in
> SkString.h.
Right. I will change it to add replace_char() to SkString, as:

void replaceChar(const char oldChar, const char newChar);

Robert, does that sounds good?

Note: I do not plan to make functional changes to it.

[reed1, 2014-10-15 14:41:53]

On 2014/10/15 14:19:59, tfarina wrote:
> On 2014/10/15 14:13:24, robertphillips wrote:
> > I think we should skip this CL an move right to having this as a helper in
> > SkString.h.
> Right. I will change it to add replace_char() to SkString, as:
> 
> void replaceChar(const char oldChar, const char newChar);
> 
> Robert, does that sounds good?
> 
> Note: I do not plan to make functional changes to it.

I do not want to add replace(char, char) to SkString, as I noted above, unless
we make it more inline with SkString's UTF8 semantics.

[tfarina, 2014-10-15 14:45:16]

On 2014/10/15 14:41:53, reed1 wrote:
> On 2014/10/15 14:19:59, tfarina wrote:
> > On 2014/10/15 14:13:24, robertphillips wrote:
> > > I think we should skip this CL an move right to having this as a helper in
> > > SkString.h.
> > Right. I will change it to add replace_char() to SkString, as:
> > 
> > void replaceChar(const char oldChar, const char newChar);
> > 
> > Robert, does that sounds good?
> > 
> > Note: I do not plan to make functional changes to it.
> 
> I do not want to add replace(char, char) to SkString, as I noted above, unless
> we make it more inline with SkString's UTF8 semantics.

For you to accept it, it needs to be:

void replace(SkUnichar oldChar, SkUnichar newChar);

?

[reed1, 2014-10-15 14:46:44]

On 2014/10/15 14:45:16, tfarina wrote:
> On 2014/10/15 14:41:53, reed1 wrote:
> > On 2014/10/15 14:19:59, tfarina wrote:
> > > On 2014/10/15 14:13:24, robertphillips wrote:
> > > > I think we should skip this CL an move right to having this as a helper
in
> > > > SkString.h.
> > > Right. I will change it to add replace_char() to SkString, as:
> > > 
> > > void replaceChar(const char oldChar, const char newChar);
> > > 
> > > Robert, does that sounds good?
> > > 
> > > Note: I do not plan to make functional changes to it.
> > 
> > I do not want to add replace(char, char) to SkString, as I noted above,
unless
> > we make it more inline with SkString's UTF8 semantics.
> 
> For you to accept it, it needs to be:
> 
> void replace(SkUnichar oldChar, SkUnichar newChar);
> 
> ?

yes.

[tfarina, 2014-10-15 20:49:28]

Robert, Mike, could you please do another review?

I will update the CL description soon.

Thanks,

[reed1, 2014-10-15 20:55:18]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-15 20:55:18]

Can you add a test that exercises this new public API?
I suggest you test with some ascii values, as well as values beyond ascii range.

[tfarina, 2014-10-15 21:29:09]

On 2014/10/15 20:55:18, reed1 wrote:
> Can you add a test that exercises this new public API?
Done.

> I suggest you test with some ascii values, as well as values beyond ascii
range.
Could you help me with the values beyond ascii range?

I don't know how to make strings with them in vim, thus I haven't found a way to
test it.

I looked at http://www.fileformat.info/info/charset/UTF-8/list.htm and
http://unicode-table.com/en.

[tfarina, 2014-10-18 03:42:27]

ptal!

https://codereview.chromium.org/646213002/diff/120001/tests/StringTest.cpp
File tests/StringTest.cpp (right):

https://codereview.chromium.org/646213002/diff/120001/tests/StringTest.cpp#ne...
tests/StringTest.cpp:197: SkString
hangul("\xed\x95\x9c\xea\xb5\xad\xec\x96\xb4");
Mike, could you take another look? Let me know if this is OK.

[reed1, 2014-10-20 12:34:49]

unichars are single code-points, and can be up to 24bits (or so).

SkString does not store unichars, it stores UTF8 byte sequences that can encode
unichars, but for any unichar value over 127, UTF8 stores a byte-sequence with
its own encoding.

Your implementation of replace() is just looking at bytes, and that means it can
easily corrupt UTF8 encoding.

To do this properly, for values > 127, you will need to first encode the "from"
and "to" unichars into their UTF8 byte sequences, and then perform a
search/replace on those strings.

[tfarina, 2014-10-20 20:49:27]

On 2014/10/20 12:34:49, reed1 wrote:
> unichars are single code-points, and can be up to 24bits (or so).
> 
> SkString does not store unichars, it stores UTF8 byte sequences that can
encode
> unichars, but for any unichar value over 127, UTF8 stores a byte-sequence with
> its own encoding.
> 
> Your implementation of replace() is just looking at bytes, and that means it
can
> easily corrupt UTF8 encoding.
> 
> To do this properly, for values > 127, you will need to first encode the
"from"
> and "to" unichars into their UTF8 byte sequences, and then perform a
> search/replace on those strings.

Mike, could you clarify to me. Is the function to encode the "from" and "to"
unichars, SkUTF8_FromUnichar()? Is that function that I have to use to do that
encoding?

[reed1, 2014-10-20 21:08:27]

On 2014/10/20 20:49:27, tfarina wrote:
> On 2014/10/20 12:34:49, reed1 wrote:
> > unichars are single code-points, and can be up to 24bits (or so).
> > 
> > SkString does not store unichars, it stores UTF8 byte sequences that can
> encode
> > unichars, but for any unichar value over 127, UTF8 stores a byte-sequence
with
> > its own encoding.
> > 
> > Your implementation of replace() is just looking at bytes, and that means it
> can
> > easily corrupt UTF8 encoding.
> > 
> > To do this properly, for values > 127, you will need to first encode the
> "from"
> > and "to" unichars into their UTF8 byte sequences, and then perform a
> > search/replace on those strings.
> 
> Mike, could you clarify to me. Is the function to encode the "from" and "to"
> unichars, SkUTF8_FromUnichar()? Is that function that I have to use to do that
> encoding?

Yes, that function should return a utf8 sequence for a given unichar.

[tfarina, 2014-10-24 01:56:08]

Patchset #6 (id:140001) has been deleted

[tfarina, 2014-11-12 01:41:34]

Mike, please, see my comments below.

Thanks!

https://codereview.chromium.org/646213002/diff/240001/src/core/SkString.cpp
File src/core/SkString.cpp (right):

https://codereview.chromium.org/646213002/diff/240001/src/core/SkString.cpp#n...
src/core/SkString.cpp:633: size_t len = SkUTF8_FromUnichar(oldChar, buffer);
Mike, could you help me here?

I'm getting the following error when I run the tests:

../../src/core/SkUtils.cpp:268: failed assertion "false && "bad unichar""
Segmentation fault (core dumped)

https://codereview.chromium.org/646213002/diff/240001/tests/StringTest.cpp
File tests/StringTest.cpp (right):

https://codereview.chromium.org/646213002/diff/240001/tests/StringTest.cpp#ne...
tests/StringTest.cpp:213: str.set("\xed\x95\x9c\xea\xb5\xad\xec\x96\xb4");
Could you suggest another string to test that SkUTF8_FromUnichar would like?

[mtklein, 2014-11-12 02:37:32]

I am concerned this has fallen into over-engineering a solution to a
non-problem.  Let's not add anything to SkString that's not used by Skia.  It
does no harm for test apps to duplicate replace_char, and they haven't needed to
care about Unicode to date.

If we make any change here, I'd prefer it be we delete the TODO in
skimage_main.cpp.  It is not important that we deduplicate that function, and
testing it falls somewhere between little and negative value.

[tfarina, 2014-11-12 11:16:29]

mtklein, would you then accept my PATCH SET 1 - 
https://codereview.chromium.org/646213002/#ps1?

[reed1, 2014-11-12 14:11:25]

https://codereview.chromium.org/646213002/diff/240001/tests/StringTest.cpp
File tests/StringTest.cpp (right):

https://codereview.chromium.org/646213002/diff/240001/tests/StringTest.cpp#ne...
tests/StringTest.cpp:213: str.set("\xed\x95\x9c\xea\xb5\xad\xec\x96\xb4");
On 2014/11/12 01:41:34, tfarina wrote:
> Could you suggest another string to test that SkUTF8_FromUnichar would like?

Why not build the string with a set of known Unichars (of any size). Internally
SkString will convert those to utf8 if you call str.appendUnichar()

[scroggo, 2014-12-15 20:34:50]

On 2014/11/12 14:11:25, reed1 wrote:
> https://codereview.chromium.org/646213002/diff/240001/tests/StringTest.cpp
> File tests/StringTest.cpp (right):
> 
>
https://codereview.chromium.org/646213002/diff/240001/tests/StringTest.cpp#ne...
> tests/StringTest.cpp:213: str.set("\xed\x95\x9c\xea\xb5\xad\xec\x96\xb4");
> On 2014/11/12 01:41:34, tfarina wrote:
> > Could you suggest another string to test that SkUTF8_FromUnichar would like?
> 
> Why not build the string with a set of known Unichars (of any size).
Internally
> SkString will convert those to utf8 if you call str.appendUnichar()

FYI: skimage is being removed with https://codereview.chromium.org/804133003/ ,
so there is no longer any duplication.

[tfarina, 2014-12-20 21:00:08]

OK. After
https://skia.googlesource.com/skia/+/9a0d6d6c88b87b3d0195e5633403325b6e922b8c
there is no more duplication.

Closing this!