[webkit-reviews] review granted: [Bug 104214] [WTR] Move text output accumulation to the UIProcess : [Attachment 178360] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 104214: [WTR] Move text output accumulation to the UIProcess
https://bugs.webkit.org/show_bug.cgi?id=104214

Attachment 178360: Patch
https://bugs.webkit.org/attachment.cgi?id=178360&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178360&action=review


What effect did this have on the speed of test running? Slower? Faster? No
noticeable effect?

Looks like the patch did not apply.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:220
> +	   outputText(String::format("Boolean value for key \"%s\" not found in
dictionary\n", key));

More efficient to use makeString instead of String::format.

    outputText(makeString("Boolean value for key \", key, \" not found in
dictionary\n"));

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:40
> +#include <wtf/text/WTFString.h>

Normally wtf/Forward.h would be sufficient in a header like this one, since
there’s no actual use of WTF::String, just a declaration of an argument of
const String& type.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:70
> +    void dumpBackForwardListsForAllPages(WTF::StringBuilder&);

Should not need the WTF:: prefix. StringBuilder.h should be taking care of
that.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:84
> +    void outputText(const WTF::String&);

Should not need the WTF:: prefix. WTFString.h should be taking care of that.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:64
> +using WTF::StringBuilder;

Should not need this. StringBuilder.h should do this.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:487
> +    stringBuilder.appendLiteral("\n");

Can use character append here.

> Tools/WebKitTestRunner/TestInvocation.cpp:109
> +    , m_textOutput(WTF::adoptPtr(new WTF::StringBuilder()))

Should not need these WTF:: prefixes, because WTF headers should do the using
thing. No need for () after StringBuilder either. Looks like you moved this
from elsewhere and it had the same problem.

> Tools/WebKitTestRunner/TestInvocation.h:72
> +    OwnPtr<WTF::StringBuilder> m_textOutput;

Why the OwnPtr? Why not just StringBuilder?