Bug 49581 – Cleanup UTF8.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 73969 [details]
Patch

[WebKit Review Bot]

Attachment 73969 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/unicode/UTF8.cpp', u'JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h']" exit_code: 1
JavaScriptCore/wtf/unicode/UTF8.cpp:242:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:243:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:244:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:245:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:246:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:41:  Missing spaces around <=  [whitespace/operators] [3]
Total errors found: 7 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 73969 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73969&action=review

Thanks for making these changes. I think these are an improvement!

> JavaScriptCore/wtf/unicode/UTF8.cpp:290
> +                *target++ = static_cast<UChar>(character); // normal case

Is this typecast really needed? Why? Doesn’t narrowing happen automatically without explicit typecasting.

> JavaScriptCore/wtf/unicode/UTF8.cpp:300
> +            character -= 0x0010000UL;
> +            *target++ = U16_LEAD(character);
> +            *target++ = U16_TRAIL(character);

This is wrong. The U16_LEAD macro works on an actual character, not a character that has had subtraction done. So this change will break things! We need to make sure we have testing coverage.

> JavaScriptCore/wtf/unicode/UTF8.cpp:307
> +                *target++ = 0xFFFD;

We should consider moving CharacterNames.h from WebCore into WTF. Then we could use the name replacementCharacter here instead of the constant 0xFFFD.

[Patrick R. Gansterer]

(In reply to comment #3)
> Thanks for making these changes. I think these are an improvement!
I hope so ;-)

> > JavaScriptCore/wtf/unicode/UTF8.cpp:300
> > +            character -= 0x0010000UL;
> > +            *target++ = U16_LEAD(character);
> > +            *target++ = U16_TRAIL(character);
> 
> This is wrong. The U16_LEAD macro works on an actual character, not a character that has had subtraction done. So this change will break things! We need to make sure we have testing coverage.
Uups, missed removal of this line. I had some failures in the LayoutTests, but they seam to be unrelated to UTF8 changes. :-/
I'm not 100% familar with all LayouTests. Can you point me to a good example to test this? There are some xml test with invalid unicode sequenzes, but I'm not sure if that's the best way to to this too.

> > JavaScriptCore/wtf/unicode/UTF8.cpp:307
> > +                *target++ = 0xFFFD;
> 
> We should consider moving CharacterNames.h from WebCore into WTF. Then we could use the name replacementCharacter here instead of the constant 0xFFFD.
Created bug 49618 to move CharacterNames.h into WTF.

[Darin Adler]

(In reply to comment #4)
> I'm not 100% familar with all LayoutTests.

Nobody is. There are too many for any one person to know them all.

> Can you point me to a good example to test this?

I’m not sure there is already one. First you’d have to look at where this function is used, and then figure out a way to make a test that will pass in a UTF-8 sequence for a character that is non-BMP to that code path. I’d be happy to help, but I can’t do it right now.

[Darin Adler]

(In reply to comment #5)
> First you’d have to look at where this function is used

As you know from your AtomicString work, one place this is used is in the XML parser. Knowing that, I think we can easily produce a test using DOMParser.

[Darin Adler]

(In reply to comment #6)
> As you know from your AtomicString work, one place this is used is in the XML parser. Knowing that, I think we can easily produce a test using DOMParser.

That won’t be completely great because in the case of DOMParser, the input is UTF-16, which is then converted to UTF-8, and then back to UTF-16. But it can test if the round trip works, and would have caught the 0x10000 bug.

[Darin Adler]

Just adding more characters to fast/encoding/xml-utf-8-default.xml might work.

[Patrick R. Gansterer]

(In reply to comment #8)
> Just adding more characters to fast/encoding/xml-utf-8-default.xml might work.
Ok, thx! I'll look for some characters in the different unicode ranges add them and post a patch.

[Patrick R. Gansterer]

Created attachment 74381 [details]
test case for unicode plane 1 characters

[Darin Adler]

You should check that this test case will catch that bug in convertUTF8ToUTF16, by leaving the bug in and running the test case.

[Patrick R. Gansterer]

(In reply to comment #11)
> You should check that this test case will catch that bug in convertUTF8ToUTF16, by leaving the bug in and running the test case.
Argh, missed to add the comment :-/.
I already tried it with the new test case, but it didn't catch an error. Then I inserted a CRASH in the convertUTF8ToUTF16 function, but it didn't crashed.
I didn't had the time to look deeper into this issue, but this additonal testcase won't hurt anyway.
Maybe you can review bug 49618 (CharacterNames.h) in the meantime.

[WebKit Commit Bot]

Comment on attachment 74381 [details]
test case for unicode plane 1 characters

Rejecting patch 74381 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 74381]" exit_code: 1
Last 500 characters of output:
 in _process_patch
    self._main_sequence.run_and_handle_errors(tool, options, state)
  File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py", line 79, in run_and_handle_errors
    log(e.message_with_output())
  File "/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/deprecated_logging.py", line 39, in log
    print >> sys.stderr, string
UnicodeEncodeError: 'ascii' codec can't encode character u'\ufeff' in position 292: ordinal not in range(128)

Full output: http://queues.webkit.org/results/6155071

[Patrick R. Gansterer]

(In reply to comment #12)
> (In reply to comment #11)
> > You should check that this test case will catch that bug in convertUTF8ToUTF16, by leaving the bug in and running the test case.
> I already tried it with the new test case, but it didn't catch an error.
Now I found the problem: I missed one toString in bug 45488. With the patch from that bug the test fails as expected.

[Patrick R. Gansterer]

Created attachment 74476 [details]
Patch

[WebKit Review Bot]

Attachment 74476 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'JavaScriptCore/ChangeLog', u'JavaScriptCore/wtf/unicode/UTF8.cpp', u'JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h']" exit_code: 1
JavaScriptCore/wtf/unicode/UTF8.cpp:245:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:245:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:246:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:247:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:248:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UTF8.cpp:249:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/wtf/unicode/UnicodeMacrosFromICU.h:41:  Missing spaces around <=  [whitespace/operators] [3]
Total errors found: 7 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Committed r72497: <http://trac.webkit.org/changeset/72497>

[Patrick R. Gansterer]

webkit-patch land closed bug, so reopen it

[Patrick R. Gansterer]

@darin: ping

[Darin Adler]

Comment on attachment 74476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74476&action=review

> JavaScriptCore/wtf/unicode/UTF8.cpp:35
> +const UChar replacementCharacter = 0xFFFD;

Since this is inside a .cpp file we should use static to give it internal linkage.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74476 [details]:

compositing/iframes/overlapped-nested-iframes.html
inspector/styles-disable-then-delete.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74476 [details]
Patch

Rejecting patch 74476 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
s/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSWebKitAnimationEvent.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/JSWebKitAnimationEvent.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSWebGLShader.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/JSWebGLShader.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(5 failures)


Full output: http://queues.webkit.org/results/6343115

[Darin Adler]

Comment on attachment 74476 [details]
Patch

Try again.

[Eric Seidel (no email)]

One of those flakes was bug 50255.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74476 [details]:

animations/play-state-suspend.html
http/tests/websocket/tests/multiple-connections.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74476 [details]
Patch

Clearing flags on attachment: 74476

Committed r72979: <http://trac.webkit.org/changeset/72979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.