http://build.webkit.org/results/GTK%20Linux%2032-bit%20Debug/r55007%20%283318%29/plugins/set-status-pretty-diff.html Direct DRT output seems to be fine: UI DELEGATE STATUS CALLBACK: setStatusText:PÐSS But the expected.txt file is wrong nonetheless: UI DELEGATE STATUS CALLBACK: setStatusText:PÃÂSS
This test fails on the Qt bot and the Windows bot seems to have the problem too. The Qt port dumps the UI DELEGATE STATUS CALLBACK: setStatusText: twice and there seems to be an encoding problem too. http://build.webkit.org/results/Qt%20Linux%20Release/r55012%20%287584%29/plugins/set-status-pretty-diff.html http://build.webkit.org/results/Windows%20Release%20%28Tests%29/r55012%20%289239%29/plugins/set-status-pretty-diff.html
Trying to set status for null message may or may not be a bug - I've just followed what Mac in-process code did. The Unicode differences on Qt/Gtk are clearly a bug.
Filed bug 35165 for the Windows failure. I think that Mac behavior should change here.
Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should only be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit http://trac.webkit.org/wiki/QtWebKitBugs#Component - Add the keyword 'Qt' to signal that it's a Qt-related bug http://trac.webkit.org/wiki/QtWebKitBugs#Keywords
Created attachment 54951 [details] Patch This fixes it - the QString is already UTF-8 encoded so it was converting \xD090 to \xC390\xC290. \C390 is the D with a pair of horizontal bars through it. I'm not sure if this is to be expected or if I'm just working around a bug in QString::toUtf8().
The fix doesn't look right to me - the QString should not be encoded, it's supposed to be UTF-16 inside.
(In reply to comment #6) > The fix doesn't look right to me - the QString should not be encoded, it's > supposed to be UTF-16 inside. I'm probably way off, but is the problem in JavasciptCore/wtf/qt/StringQt.cpp then? Should it detect if the String it is casting is already UTF-8 encoded, as in the case of this test?
String code shouldn't look inside strings passed to constructor to determine their encoding. I don't know where exactly the bug is, but some code clearly has a wrong assumption about buffer content, and uses a wrong encoding to construct String or QString. This can be in NPAPI or in DumpRenderTree bindings.
Comment on attachment 54951 [details] Patch Marking r- to get this out of review queue. I hoped that a reviewer more knowledgeable about Qt would chime in, but this hasn't happened yet.
(In reply to comment #6) > The fix doesn't look right to me - the QString should not be encoded, it's > supposed to be UTF-16 inside. This is the call chain as I understand it. The string passed from the test plugin does appear to be UTF-8: static bool testSetStatus(PluginObject* obj, const NPVariant* args, uint32_t argCount, NPVariant* result) { char* message = 0; if (argCount && NPVARIANT_IS_STRING(args[0])) { NPString statusString = NPVARIANT_TO_STRING(args[0]); message = toCString(statusString); browser->status(obj->npp, message); } static char* toCString(const NPString& string) { char* result = static_cast<char*>(malloc(string.UTF8Length + 1)); memcpy(result, string.UTF8Characters, string.UTF8Length); result[string.UTF8Length] = '\0'; return result; } PluginView casts it to a String: void PluginView::status(const char* message) { if (Page* page = m_parentFrame->page()) page->chrome()->setStatusbarText(m_parentFrame.get(), String(message)); } As far as I can tell, displayStringModifiedByEncoding() isn't doing anything noteworthy to the string, unless it contains backslashes. void Chrome::setStatusbarText(Frame* frame, const String& status) { ASSERT(frame); m_client->setStatusbarText(frame->displayStringModifiedByEncoding(status)); } ChromeClientQt casts it to a QString: void ChromeClientQt::setStatusbarText(const String& msg) { QString x = msg; emit m_webPage->statusBarMessage(msg); } So there's no conversion to UTF-16 along the way here unless that it is implicit in one of the casts. That leads me to think that doing : printf("UI DELEGATE STATUS CALLBACK: setStatusText:%s\n", message.toUtf8().constData()); in DumpRenderTreeQt is converting UTF-8 to UTF-8 a second time, which is why we get 'ÃÂ', which as far as I can tell is an attempt to UTF-8 encode 'Ð'.
> void PluginView::status(const char* message) > { > if (Page* page = m_parentFrame->page()) > page->chrome()->setStatusbarText(m_parentFrame.get(), String(message)); >} This is wrong, and should use String::fromUTF8(). The String constructor treats its char* input as latin-1.
(In reply to comment #11) > > void PluginView::status(const char* message) > > { > > if (Page* page = m_parentFrame->page()) > > page->chrome()->setStatusbarText(m_parentFrame.get(), String(message)); > >} > > This is wrong, and should use String::fromUTF8(). The String constructor treats > its char* input as latin-1. Excellent, thanks Alexey!
Created attachment 55145 [details] Patch
Created attachment 55146 [details] Patch Whoops.
Comment on attachment 55146 [details] Patch r=me
You might want to test what happens if a plug-in passes invalid UTF-8. String::fromUTF8() will make a null string, which may or may not be a problem downstream.
Attachment 55146 [details] was posted by a committer and has review+, assigning to Robert Hogan for commit.
Landed in r59016.