It makes error handling easier
Created attachment 71150 [details] Patch to use GCharsetConverter It fixes these tests when building with glib unicode: fast/encoding/invalid-multi-byte-over-consumption.html fast/encoding/invalid-xml.html fast/encoding/japanese-encoding-mix.html
Comment on attachment 71150 [details] Patch to use GCharsetConverter View in context: https://bugs.webkit.org/attachment.cgi?id=71150&action=review Great patch! Needs a few fixes, but I like where this is headed. > WebCore/platform/text/gtk/TextCodecGtk.cpp:322 > + , m_iconvDecoder(0) > + , m_iconvEncoder(0) No need to initialize these. The default constructor sets the RefPtr to 0. > WebCore/platform/text/gtk/TextCodecGtk.cpp:359 > + gsize bytesRead, bytesWritten; > + const gchar *input = bytes; bytesRead and bytesWritten should be initialized and on separate lines. Should be const gchar* input = bytes. > WebCore/platform/text/gtk/TextCodecGtk.cpp:362 > + int flags = !length ? G_CONVERTER_INPUT_AT_END : G_CONVERTER_NO_FLAGS; There's an extra space after ? > WebCore/platform/text/gtk/TextCodecGtk.cpp:392 > + GOwnPtr<GError> err; > + GConverterResult res; > + > + res = g_converter_convert(G_CONVERTER(m_iconvDecoder.get()), > + input, > + inputLength, > + buffer, > + sizeof(buffer), > + (GConverterFlags)flags, > + &bytesRead, > + &bytesWritten, > + &err.outPtr()); Please change "err" to "error". No need to define and declare res separately. In general, I like this style of indenting, but in this case I think that it makes the method harder to read, because it is so long. Perhaps it could be like this? GConverterResult res = g_converter_convert(G_CONVERTER(m_iconvDecoder.get()), input, inputLength buffer, sizeof(buffer), ... I think you should also use a static_cast for flags. > WebCore/platform/text/gtk/TextCodecGtk.cpp:399 > + if (g_error_matches(err.get(), G_IO_ERROR, G_IO_ERROR_PARTIAL_INPUT)) { > + memcpy(m_bufferedBytes, input, inputLength); > + m_numBufferedBytes = inputLength; You might want to also leave a comment here about why this might happen and what the strategy is for dealing with it, since it's not obvious from an initial inspection. > WebCore/platform/text/gtk/TextCodecGtk.cpp:440 > + gsize bytesRead, bytesWritten; > + const gchar *input = reinterpret_cast<const char*>(characters); bytesRead and bytesWritten should be initialized and on separate lines. Careful with the * again. > WebCore/platform/text/gtk/TextCodecGtk.cpp:444 > GOwnPtr<GError> err; Probably a good idea to get rid of this abbreviation now. > WebCore/platform/text/gtk/TextCodecGtk.cpp:458 > + res = g_converter_convert(G_CONVERTER(m_iconvEncoder.get()), > + input, > + inputLength, > + buffer, > + sizeof(buffer), > + G_CONVERTER_INPUT_AT_END, > + &bytesRead, > + &bytesWritten, > + &err.outPtr()); Same comment here as above.
Created attachment 71308 [details] Updated patch according to review
Comment on attachment 71308 [details] Updated patch according to review View in context: https://bugs.webkit.org/attachment.cgi?id=71308&action=review I have a couple more nits, but it's very close. > WebCore/platform/text/gtk/TextCodecGtk.cpp:348 > + ASSERT(m_iconvDecoder); > + if (!m_iconvDecoder) { Sorry, I missed this before. Either we should remove the ASSERT here or not handle the failure case. > WebCore/platform/text/gtk/TextCodecGtk.cpp:393 > + // there is not enough input to fully determine what the conversion should produce > + // save it to a buffer to prepend it to the next input Nice comment, but it should be a full sentence with punctuation, etc. > WebCore/platform/text/gtk/TextCodecGtk.cpp:399 > + } else if (g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_NO_SPACE)) { > + bufferWasFull = true; > + } else if (g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_INVALID_DATA)) { WebKit style dictates that one-line blocks omit their surrounding curly braces (as much as I dislike it). > WebCore/platform/text/gtk/TextCodecGtk.cpp:410 > + m_numBufferedBytes = 0; // reset state for subsequent calls to decode This comment should also be in sentence form.
Created attachment 71394 [details] Another update according to review
Comment on attachment 71394 [details] Another update according to review Great. Thanks for the fixes. Hopefully we can continue to improve this code as we go along.
Comment on attachment 71394 [details] Another update according to review Rejecting patch 71394 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 71394]" exit_code: 2 Cleaning working directory Updating working directory Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=71394&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=47896&ctype=xml Processing 1 patch from 1 bug. Processing patch 71394 from bug 47896. Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/4705008
Created attachment 71454 [details] Update patch for current git master
Comment on attachment 71454 [details] Update patch for current git master Clearing flags on attachment: 71454 Committed r70257: <http://trac.webkit.org/changeset/70257>
All reviewed patches have been landed. Closing bug.