Bug 58420 - String operator+ reallocates unnecessarily when concatting > 2 strings
Summary: String operator+ reallocates unnecessarily when concatting > 2 strings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 57840 57843 60700
  Show dependency treegraph
 
Reported: 2011-04-13 02:15 PDT by Nikolas Zimmermann
Modified: 2011-05-12 08:00 PDT (History)
17 users (show)

See Also:


Attachments
Patch (103.19 KB, patch)
2011-04-13 02:51 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (32.56 KB, patch)
2011-04-13 02:53 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (35.02 KB, patch)
2011-04-13 06:40 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v4 (37.92 KB, patch)
2011-04-13 07:56 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v5 (38.66 KB, patch)
2011-04-13 11:32 PDT, Nikolas Zimmermann
mjs: review-
Details | Formatted Diff | Diff
Patch v6 (34.90 KB, patch)
2011-04-27 13:10 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v7 (37.20 KB, patch)
2011-04-27 14:04 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v8 (37.70 KB, patch)
2011-04-27 15:18 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v9 (39.18 KB, patch)
2011-04-27 15:37 PDT, Nikolas Zimmermann
barraclough: review-
Details | Formatted Diff | Diff
Example of recursive implementation suggestion. (1.24 KB, application/octet-stream)
2011-04-27 16:49 PDT, Gavin Barraclough
no flags Details
Patch v10 (36.31 KB, patch)
2011-05-01 06:15 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v11 (37.28 KB, patch)
2011-05-01 07:12 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v12 (37.27 KB, patch)
2011-05-01 07:24 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v13 (37.21 KB, patch)
2011-05-01 09:33 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
workaround for Patch v13 to make Qt build happy (10.64 KB, patch)
2011-05-01 12:16 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch v14 (37.40 KB, patch)
2011-05-04 02:10 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v15 (37.59 KB, patch)
2011-05-04 04:20 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v16 (37.74 KB, patch)
2011-05-04 13:22 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v17 (37.68 KB, patch)
2011-05-05 03:18 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v18 (37.18 KB, patch)
2011-05-10 07:07 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v19 (38.11 KB, patch)
2011-05-10 12:51 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v20 (38.72 KB, patch)
2011-05-11 04:50 PDT, Nikolas Zimmermann
darin: review+
Details | Formatted Diff | Diff
Patch v21 (36.75 KB, patch)
2011-05-12 02:16 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v22 (37.00 KB, patch)
2011-05-12 03:45 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.23 MB, application/zip)
2011-05-12 04:47 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-04-13 02:15:14 PDT
String operator+ reallocates too often when concatting > 2 strings.

void String::append(const String& str)
{
    if (str.isEmpty())
       return;

    // FIXME: This is extremely inefficient. So much so that we might want to take this
    // out of String's API. We can make it better by optimizing the case where exactly
    // one String is pointing at this StringImpl, but even then it's going to require a
    // call to fastMalloc every single time.
    if (str.m_impl) {
        if (m_impl) {
            UChar* data;
            if (str.length() > numeric_limits<unsigned>::max() - m_impl->length())
                CRASH();
            RefPtr<StringImpl> newImpl = StringImpl::createUninitialized(m_impl->length() + str.length(), data);
            memcpy(data, m_impl->characters(), m_impl->length() * sizeof(UChar));
            memcpy(data + m_impl->length(), str.characters(), str.length() * sizeof(UChar));
            m_impl = newImpl.release();
        } else
            m_impl = str.m_impl;
    }
}

Currently String::append is used when invoking operator+:

String operator+(const String& a, const String& b)
{
    if (a.isEmpty())
        return b;
    if (b.isEmpty())
        return a;
    String c = a;
    c += b;
    return c;
}

In bug 57840, 57843 I worked on converting WebCore to use StringBuilder/StringConcatenate, where possible. Though it become apparent that the better approach is to fix operator+ so it makes use of makeString/StringBuilder internally, instead of having to change lots of call-sites currently using operator+.

The idea behind the StringAppend2 template trick, as seen in the patch, is from Maciej, and it's great!
Comment 1 Nikolas Zimmermann 2011-04-13 02:51:17 PDT
Created attachment 89357 [details]
Patch

Uploading for discussion and EWS results. As symbols changed in JSC, I expect breakage on win.
Works just fine on mac. I wish I could measure performance, if and how much impact this is. Maybe apple folks can run PLT?
Comment 2 Nikolas Zimmermann 2011-04-13 02:53:10 PDT
Created attachment 89358 [details]
Patch v2

Oops, I left in some local unrelated WebCore.xcodeproj changes.
Comment 3 WebKit Review Bot 2011-04-13 02:55:12 PDT
Attachment 89358 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/text/StringBuilder.h:148:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Nikolas Zimmermann 2011-04-13 03:02:22 PDT
(In reply to comment #3)
> Attachment 89358 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> 
> Source/JavaScriptCore/wtf/text/StringBuilder.h:148:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 25 files
I'll add // NOLINT to suppress this warning. Uploading next version, after all EWS results are available.
Comment 5 Early Warning System Bot 2011-04-13 03:10:48 PDT
Attachment 89358 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8397199
Comment 6 WebKit Review Bot 2011-04-13 03:17:58 PDT
Attachment 89358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8398215
Comment 7 Build Bot 2011-04-13 04:01:52 PDT
Attachment 89358 [details] did not build on win:
Build output: http://queues.webkit.org/results/8399218
Comment 8 Nikolas Zimmermann 2011-04-13 06:40:26 PDT
Created attachment 89371 [details]
Patch v3

Fix style and hopefully the linux/win builds. Gtk and EFL ews are unrepsonsive.
Comment 9 Gustavo Noronha (kov) 2011-04-13 07:17:24 PDT
Attachment 89371 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8403222
Comment 10 WebKit Review Bot 2011-04-13 07:22:08 PDT
Attachment 89371 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8404242
Comment 11 WebKit Review Bot 2011-04-13 07:36:27 PDT
Attachment 89358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8399258
Comment 12 Build Bot 2011-04-13 07:42:33 PDT
Attachment 89371 [details] did not build on win:
Build output: http://queues.webkit.org/results/8402258
Comment 13 Nikolas Zimmermann 2011-04-13 07:56:43 PDT
Created attachment 89377 [details]
Patch v4

More build fixes...
Comment 14 WebKit Review Bot 2011-04-13 08:39:49 PDT
Attachment 89377 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8397279
Comment 15 Nikolas Zimmermann 2011-04-13 11:32:26 PDT
Created attachment 89409 [details]
Patch v5

Trying to fix cr-linux build, cr-mac still didn't build.
Comment 16 WebKit Review Bot 2011-04-13 11:58:37 PDT
Attachment 89377 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8399368
Comment 17 Adam Roben (:aroben) 2011-04-21 07:43:22 PDT
Why not just use StringConcatenate.h directly?
Comment 18 Nikolas Zimmermann 2011-04-21 07:49:14 PDT
(In reply to comment #17)
> Why not just use StringConcatenate.h directly?

See bugs https://bugs.webkit.org/show_bug.cgi?id=57840 and https://bugs.webkit.org/show_bug.cgi?id=57843, where I started to move WebCore/ towards using StringBuilder/StringConcatenate directly.
Maciej suggested to optimize our operator+ instead, to aviod having to change all call sites that just concat 2 strings, that was the initial motivation.

I wanted to make WebCore use StringBuilder/Concatenate directly, though Maciej convinved me this direction would be better.
Comment 19 Maciej Stachowiak 2011-04-26 16:08:47 PDT
Comment on attachment 89409 [details]
Patch v5

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

In general this approach looks really good, although I am going to say r- for now, because I suggested many changes.

There is also a small risk that we could have made the two-string operator+ append a tiny bit slower if compilers are not smart enough to optimize out the temporary struct. Can you do some performance testing to make sure this is an improvement or at least not a regression?

> Source/JavaScriptCore/wtf/text/AtomicStringConcatenate.h:37
> +    StringTypeAdapter<AtomicString>(const AtomicString& string)
> +        : m_buffer(string.string())
> +    {
> +    }

How about making this hold a StringTypeAdapter<String> and just forward to it? That would let you avoid replicating the implementations of length() and writeTo()

> Source/JavaScriptCore/wtf/text/StringBuilder.h:148
> +#include "StringOperators.h" // NOLINT

What's NOLINT about?

> Source/JavaScriptCore/wtf/text/WTFString.h:366
> +template<typename T1, typename T2>
> +StringAppend2<T1, T2> operator+(const T1& a, const T2& b)
> +{
> +    return StringAppend2<T1, T2>(a, b);
> +}

I think this is a little risky, because it could apply to any two non-POD types. That has two issues:

1) It could give a really mysterious error when it fails, probably something about not being able to instantiate StringAdapter.
2) If you have a class type that represents a numeric value, it might make sense to add those things numerically, but also be able to append them to strings. With this very generic template function, if you forget to implement an operator+ specifically for that type, you'll accidentally get string appends instead.

What I would suggest is to instead define template functions where either the first or second argument has to be a String or AtomicString (for four total).

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:713
> +            namespaceQName = String("xmlns:" + toString(namespaces[i].prefix));

Maybe instead of this,  you should give StringAppend[N] and StringBuilder implicit conversions to AtomicString. It seems String already had one so presumably it can do no harm.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1421
> +                String string = existingStyle->cssText() + styleChange.cssStyle();
> +                setNodeAttribute(styleContainer, styleAttr, string);

This seems like the same AtomicString issue - could avoid this by adding an AtomicString implicit conversion.

> Source/WebCore/editing/MarkupAccumulator.cpp:209
> -    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
> +    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : String("xmlns");

I wonder if it would be more efficient to have String or AtomicString constants (as appropriate) defined for strings like "xmlns". Every time through this code, if the prefix is empty, it converts the "xmlns" to String, creating a buffer, and then to AtomicString, throwing away the buffer, which is wasteful. Maybe there already is one? If you did that though, you'd also want the implicit conversion from StringAppend2 to AtomicString.

> Source/WebCore/html/HTMLAnchorElement.cpp:307
> -    return fragmentIdentifier.isEmpty() ? "" : "#" + fragmentIdentifier;
> +    return fragmentIdentifier.isEmpty() ? String("") : "#" + fragmentIdentifier;

Maybe we should add an emptyString global, or String::empty(), just like we have emptyAtom.

> Source/WebCore/html/HTMLAnchorElement.cpp:444
> -    return query.isEmpty() ? "" : "?" + query;
> +    return query.isEmpty() ? String("") : "?" + query;

ditto previous comment about empty string.

> Source/WebCore/html/ImageInputType.cpp:61
> +    encoding.appendData(name.isEmpty() ? String("x") : (name + ".x"), m_clickLocation.x());
> +    encoding.appendData(name.isEmpty() ? String("y") : (name + ".y"), m_clickLocation.y());

Maybe it would be better to have defined String constants for "x" and "y". Probably not a super hot code path though, even when hit (unlike xmlns which could be very common in XML documents).

> Source/WebCore/page/Location.cpp:121
> -    return url.query().isEmpty() ? "" : "?" + url.query();
> +    return url.query().isEmpty() ? String("") : "?" + url.query();

Again, see previous remarks about empty string.

> Source/WebCore/page/Location.cpp:137
> +    return fragmentIdentifier.isEmpty() ? String("") : "#" + fragmentIdentifier;

Again, see previous remarks about empty string.

> Source/WebCore/page/NavigatorBase.cpp:92
> -    DEFINE_STATIC_LOCAL(String, platformName, (uname(&osname) >= 0 ? String(osname.sysname) + String(" ") + String(osname.machine) : ""));
> +    DEFINE_STATIC_LOCAL(String, platformName, (uname(&osname) >= 0 ? String(osname.sysname) + String(" ") + String(osname.machine) : String("")));

Again, see previous remarks about empty string. Also, some of the String() explicit calls on the other side of the ?: are probably unnecessary. I think you only need the first one.

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:260
> -    dataObject->setFileExtension(extension.isEmpty() ? "" : "." + extension);
> +    dataObject->setFileExtension(extension.isEmpty() ? String("") : "." + extension);

Another empty string case.

> Source/WebCore/workers/WorkerLocation.cpp:69
> -    return m_url.query().isEmpty() ? "" : "?" + m_url.query();
> +    return m_url.query().isEmpty() ? String("") : "?" + m_url.query();

Empty string yada yada

> Source/WebCore/workers/WorkerLocation.cpp:74
> -    return m_url.fragmentIdentifier().isEmpty() ? "" : "#" + m_url.fragmentIdentifier();
> +    return m_url.fragmentIdentifier().isEmpty() ? String("") : "#" + m_url.fragmentIdentifier();

Empty string again.

> Source/WebKit/mac/Misc/WebNSURLExtras.mm:58
> -static uint32_t IDNScriptWhiteList[(USCRIPT_CODE_LIMIT + 31) / 32];
> +static uint32_t IDNScriptWhiteList[(unsigned(USCRIPT_CODE_LIMIT) + 31) / 32];

This is probably the consequence of having the super-generic operator+, as I warned. I think it would be better to be a little less generic than to have typecasts like this.
Comment 20 Nikolas Zimmermann 2011-04-27 05:56:40 PDT
(In reply to comment #19)
> (From update of attachment 89409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89409&action=review
> 
> In general this approach looks really good, although I am going to say r- for now, because I suggested many changes.
Sure!
 
> There is also a small risk that we could have made the two-string operator+ append a tiny bit slower if compilers are not smart enough to optimize out the temporary struct. Can you do some performance testing to make sure this is an improvement or at least not a regression?
Hm, do you have suggestions on performance testing? Did you think about sharking? Or a micro benchmark? Not sure what's the best way to measure performance here.
 
> > Source/JavaScriptCore/wtf/text/AtomicStringConcatenate.h:37
> > +    StringTypeAdapter<AtomicString>(const AtomicString& string)
> > +        : m_buffer(string.string())
> > +    {
> > +    }
> 
> How about making this hold a StringTypeAdapter<String> and just forward to it? That would let you avoid replicating the implementations of length() and writeTo()
Good idea, fixed.
 
> > Source/JavaScriptCore/wtf/text/StringBuilder.h:148
> > +#include "StringOperators.h" // NOLINT
> 
> What's NOLINT about?
It suppresses style bot messages, as this include order error is intentional.
 
> > Source/JavaScriptCore/wtf/text/WTFString.h:366
> > +template<typename T1, typename T2>
> > +StringAppend2<T1, T2> operator+(const T1& a, const T2& b)
> > +{
> > +    return StringAppend2<T1, T2>(a, b);
> > +}
> 
> I think this is a little risky, because it could apply to any two non-POD types. That has two issues:
> 
> 1) It could give a really mysterious error when it fails, probably something about not being able to instantiate StringAdapter.
Yeah, i've seen this as well, before making adjustments of the operator+ usage in several places.

> 2) If you have a class type that represents a numeric value, it might make sense to add those things numerically, but also be able to append them to strings. With this very generic template function, if you forget to implement an operator+ specifically for that type, you'll accidentally get string appends instead.
> 
> What I would suggest is to instead define template functions where either the first or second argument has to be a String or AtomicString (for four total).
Great suggestion, fixed.

> 
> > Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:713
> > +            namespaceQName = String("xmlns:" + toString(namespaces[i].prefix));
> 
> Maybe instead of this,  you should give StringAppend[N] and StringBuilder implicit conversions to AtomicString. It seems String already had one so presumably it can do no harm.
Fixed.

> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:1421
> > +                String string = existingStyle->cssText() + styleChange.cssStyle();
> > +                setNodeAttribute(styleContainer, styleAttr, string);
> 
> This seems like the same AtomicString issue - could avoid this by adding an AtomicString implicit conversion.
Fixed.

> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:209
> > -    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
> > +    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : String("xmlns");
> 
> I wonder if it would be more efficient to have String or AtomicString constants (as appropriate) defined for strings like "xmlns". Every time through this code, if the prefix is empty, it converts the "xmlns" to String, creating a buffer, and then to AtomicString, throwing away the buffer, which is wasteful. Maybe there already is one? If you did that though, you'd also want the implicit conversion from StringAppend2 to AtomicString.
We have XMLNSNames::xmlnsAttr as QualifiedName, we can access the "xmlns" string by using localName(), which gives an AtomicString, should be cheaper.
    AtomicString attr = prefix.isEmpty() ? xmlnsAttr.localName() : xmlnsAttr.localName() + ":" + prefix; 

> 
> > Source/WebCore/html/HTMLAnchorElement.cpp:307
> > -    return fragmentIdentifier.isEmpty() ? "" : "#" + fragmentIdentifier;
> > +    return fragmentIdentifier.isEmpty() ? String("") : "#" + fragmentIdentifier;
> 
> Maybe we should add an emptyString global, or String::empty(), just like we have emptyAtom.
How about an emptyString() free function returning a const String&, that's declared using DEFINE_STATIC_LOCAL?


> > Source/WebCore/html/HTMLAnchorElement.cpp:444
> > -    return query.isEmpty() ? "" : "?" + query;
> > +    return query.isEmpty() ? String("") : "?" + query;
> 
> ditto previous comment about empty string.
Now uses emptyString() here.
 
> > Source/WebCore/html/ImageInputType.cpp:61
> > +    encoding.appendData(name.isEmpty() ? String("x") : (name + ".x"), m_clickLocation.x());
> > +    encoding.appendData(name.isEmpty() ? String("y") : (name + ".y"), m_clickLocation.y());
> 
> Maybe it would be better to have defined String constants for "x" and "y". Probably not a super hot code path though, even when hit (unlike xmlns which could be very common in XML documents).
Right, I left it this way.

> 
> > Source/WebCore/page/Location.cpp:121
> > -    return url.query().isEmpty() ? "" : "?" + url.query();
> > +    return url.query().isEmpty() ? String("") : "?" + url.query();
> 
> Again, see previous remarks about empty string.
Fixed.

> > Source/WebCore/page/Location.cpp:137
> > +    return fragmentIdentifier.isEmpty() ? String("") : "#" + fragmentIdentifier;
> 
> Again, see previous remarks about empty string.
Fixed.
 
> > Source/WebCore/page/NavigatorBase.cpp:92
> > -    DEFINE_STATIC_LOCAL(String, platformName, (uname(&osname) >= 0 ? String(osname.sysname) + String(" ") + String(osname.machine) : ""));
> > +    DEFINE_STATIC_LOCAL(String, platformName, (uname(&osname) >= 0 ? String(osname.sysname) + String(" ") + String(osname.machine) : String("")));
> 
> Again, see previous remarks about empty string. Also, some of the String() explicit calls on the other side of the ?: are probably unnecessary. I think you only need the first one.
Okay, will check.
 
> > Source/WebCore/platform/chromium/ClipboardChromium.cpp:260
> > -    dataObject->setFileExtension(extension.isEmpty() ? "" : "." + extension);
> > +    dataObject->setFileExtension(extension.isEmpty() ? String("") : "." + extension);
> 
> Another empty string case.
> Empty string yada yada
> Empty string again.
Fixed.

> 
> > Source/WebKit/mac/Misc/WebNSURLExtras.mm:58
> > -static uint32_t IDNScriptWhiteList[(USCRIPT_CODE_LIMIT + 31) / 32];
> > +static uint32_t IDNScriptWhiteList[(unsigned(USCRIPT_CODE_LIMIT) + 31) / 32];
> 
> This is probably the consequence of having the super-generic operator+, as I warned. I think it would be better to be a little less generic than to have typecasts like this.
Definately, fixed.

Going to rebuild from scratch, and then upload a new patch. Thanks a lot for the review!
Comment 21 Nikolas Zimmermann 2011-04-27 13:10:45 PDT
Created attachment 91328 [details]
Patch v6

Adressed Maciejs comments.
I've tried a simple microbenchmark, concatting two Strings, several thousands of times, and couldn't measure any difference between the new and the old method. Though that's not really accurate, but I think we won't see any performance regressions for the simple two string concat case, and only wins for n>2 concats.

Could we verify the theory that nothing changed using a post-commit PLT run from Apple?
P.S. I bet we'll need a Patch v7 as I need to adjust win symbol export table after I get the EWS results.
Comment 22 WebKit Review Bot 2011-04-27 13:28:20 PDT
Attachment 91328 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8520059
Comment 23 Early Warning System Bot 2011-04-27 13:29:23 PDT
Attachment 91328 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8520061
Comment 24 Collabora GTK+ EWS bot 2011-04-27 13:55:48 PDT
Attachment 91328 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8516157
Comment 25 Build Bot 2011-04-27 14:02:39 PDT
Attachment 91328 [details] did not build on win:
Build output: http://queues.webkit.org/results/8514317
Comment 26 Nikolas Zimmermann 2011-04-27 14:04:18 PDT
Created attachment 91344 [details]
Patch v7

Forgot to include a change to rendering/RenderCounter.cpp, should fix Qt/Win build.
Adding another AtomicString.h include to StringOperators.h, which hopefully fixes cr-linux builds.
Comment 27 WebKit Review Bot 2011-04-27 14:36:51 PDT
Attachment 91344 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8511473
Comment 28 Early Warning System Bot 2011-04-27 14:52:33 PDT
Attachment 91344 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8518087
Comment 29 Nikolas Zimmermann 2011-04-27 15:18:03 PDT
Created attachment 91358 [details]
Patch v8

More build fixes...
Comment 30 Build Bot 2011-04-27 15:26:38 PDT
Attachment 91344 [details] did not build on win:
Build output: http://queues.webkit.org/results/8520087
Comment 31 WebKit Review Bot 2011-04-27 15:35:04 PDT
Attachment 91358 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8518100
Comment 32 Nikolas Zimmermann 2011-04-27 15:37:07 PDT
Created attachment 91364 [details]
Patch v9

More build fixes...
Comment 33 WebKit Review Bot 2011-04-27 15:47:13 PDT
Attachment 91328 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8519080
Comment 34 Early Warning System Bot 2011-04-27 15:51:00 PDT
Attachment 91358 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8514339
Comment 35 WebKit Review Bot 2011-04-27 16:05:21 PDT
Attachment 91364 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8511503
Comment 36 Gavin Barraclough 2011-04-27 16:47:29 PDT
Comment on attachment 91364 [details]
Patch v9

Hey Niko,

I like the patch! - this seems like a great implementation to keep the nice clean + operator while giving it a fast implementation under the hood.  I have one concern, and one suggestion.

My concern is the use of DEFINE_STATIC_LOCAL with strings.  This is fine for the empty string – this has a special impl that is thread safe, but all other strings are not, and adding static locals with make these methods unsafe to call from other threads.  If you believe this is okay for these methods, then I think we should at least add an "ASSERT(isMainThread());" to guard.

Looking at this, the approach of bailing out after 5 appends doesn't seem terrible, but I think I've seen a couple of hot string adds that append more than 4 times.  Another possibility would be to tweak your code to take a recursive approach - define the LHS of a StringAppend be able to be either a String, or another string append.  The code would effectively recurse summing the sizes of the StringAppends (though this would all be inlined, so it would just be summing the string adaptors sizes), then allocate a buffer, then recurse over the structure writing characters to a buffer.  I've hacked up a quick implementation outside of WebCore to toy with the idea, I'll upload a copy so you can see what you think of the idea.

I'll r- this patch for now, really with respect the the thread safety concern re use of DEFINE_STATIC_LOCAL, but really like this idea in principal.

cheers,
G.
Comment 37 Gavin Barraclough 2011-04-27 16:49:17 PDT
Created attachment 91383 [details]
Example of recursive implementation suggestion.
Comment 38 WebKit Review Bot 2011-04-27 19:42:49 PDT
Attachment 91364 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8517191
Comment 39 WebKit Review Bot 2011-04-27 20:58:22 PDT
Attachment 91364 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8511610
Comment 40 Nikolas Zimmermann 2011-04-30 10:04:32 PDT
Gavin, thanks for the review and the ToyRecursive concept - I think I'll switch to it before submitting another patch, as it's more elegant, and avoids the StringBuilder-fallback.
Comment 41 Nikolas Zimmermann 2011-05-01 06:15:24 PDT
Created attachment 91821 [details]
Patch v10

A new approach, using Gavins recursive template idea, works like a charme, and looks more beautiful than the first approach.
Builds fine on my mac, from scratch, no regressions in the layout tests.
Comment 42 Early Warning System Bot 2011-05-01 06:30:39 PDT
Attachment 91821 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8531342
Comment 43 WebKit Review Bot 2011-05-01 06:47:05 PDT
Attachment 91821 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8526451
Comment 44 Build Bot 2011-05-01 07:06:33 PDT
Attachment 91821 [details] did not build on win:
Build output: http://queues.webkit.org/results/8521908
Comment 45 Nikolas Zimmermann 2011-05-01 07:12:22 PDT
Created attachment 91823 [details]
Patch v11

Oops, forgot to include the changes in Source/WebCore/ChangeLog.
Added an inline variant of the operator+ for two Strings, hopefully fixes Qt build, which sees QString operator+ clashes.
Comment 46 Nikolas Zimmermann 2011-05-01 07:24:39 PDT
Created attachment 91824 [details]
Patch v12

Previous patch had a merge problem.
Comment 47 Early Warning System Bot 2011-05-01 07:26:38 PDT
Attachment 91823 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8526460
Comment 48 WebKit Review Bot 2011-05-01 07:28:32 PDT
Attachment 91823 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8530356
Comment 49 Collabora GTK+ EWS bot 2011-05-01 07:37:36 PDT
Attachment 91821 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8531350
Comment 50 Early Warning System Bot 2011-05-01 07:41:17 PDT
Attachment 91824 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8523745
Comment 51 Build Bot 2011-05-01 08:17:41 PDT
Attachment 91824 [details] did not build on win:
Build output: http://queues.webkit.org/results/8523753
Comment 52 WebKit Review Bot 2011-05-01 08:22:13 PDT
Attachment 91821 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8531358
Comment 53 WebKit Review Bot 2011-05-01 08:37:08 PDT
Attachment 91823 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8528367
Comment 54 WebKit Review Bot 2011-05-01 08:50:40 PDT
Attachment 91821 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8530371
Comment 55 Nikolas Zimmermann 2011-05-01 09:33:07 PDT
Created attachment 91826 [details]
Patch v13

More build fixes...
Comment 56 Early Warning System Bot 2011-05-01 09:49:05 PDT
Attachment 91826 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8529377
Comment 57 Nikolas Zimmermann 2011-05-01 11:49:08 PDT
Does anyone have a suggestion how to fix the Qt build for "Patch v13". There are operator+ clashes with QString. I'm not sure why QString is included at all when building css/CSS*.cpp files, but it appears to be and cause clashes with the operator+ defined in StringOperators.h.

I hope ossy or any szeged guys could have a look?
Comment 58 Csaba Osztrogonác 2011-05-01 12:16:11 PDT
Created attachment 91831 [details]
workaround for Patch v13 to make Qt build happy

I have a quick look at the Qt build, and attached a quick workaround.
I know it isn't a good solution for this problem, but it can show
what happened. It seems, that the problem is around operator+ when
the left side is char* and the right side is String. I think the
compiler can't determine which operator+ should be used here:
a) const QString operator+(const char*, const QString&)
b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)

I hope it can help you a little bit. But unfortunately I don't 
have more time on sunday evening to find a better solution.
Comment 59 Build Bot 2011-05-01 12:31:49 PDT
Attachment 91826 [details] did not build on win:
Build output: http://queues.webkit.org/results/8530405
Comment 60 Nikolas Zimmermann 2011-05-01 12:58:10 PDT
(In reply to comment #58)
> Created an attachment (id=91831) [details]
> workaround for Patch v13 to make Qt build happy
> 
> I have a quick look at the Qt build, and attached a quick workaround.
> I know it isn't a good solution for this problem, but it can show
> what happened. It seems, that the problem is around operator+ when
> the left side is char* and the right side is String. I think the
> compiler can't determine which operator+ should be used here:
> a) const QString operator+(const char*, const QString&)
> b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> 
> I hope it can help you a little bit. But unfortunately I don't 
> have more time on sunday evening to find a better solution.

Thanks a lot for your help! Is the QString include really intended when building WebCore (non Qt specific files)? If we'd remove that, we wouldn't need your changes, and I'd really like to avoid hacks like those.

I'm not yet sure why the win build remains broken, any ideas here? CC'ing Adam Roben.
Comment 61 Adam Roben (:aroben) 2011-05-02 10:00:24 PDT
(In reply to comment #59)
> Attachment 91826 [details] did not build on win:
> Build output: http://queues.webkit.org/results/8530405

Here's the first error:

6>c:\cygwin\home\buildbot\webkitbuild\debug\include\private\javascriptcore\StringOperators.h(74) : error C2143: syntax error : missing ';' before '<'

I have no idea what's causing this! Hopefully I'll have some time later today to try building this patch myself.
Comment 62 Nikolas Zimmermann 2011-05-03 00:36:20 PDT
(In reply to comment #61)
> (In reply to comment #59)
> > Attachment 91826 [details] [details] did not build on win:
> > Build output: http://queues.webkit.org/results/8530405
> 
> Here's the first error:
> 
> 6>c:\cygwin\home\buildbot\webkitbuild\debug\include\private\javascriptcore\StringOperators.h(74) : error C2143: syntax error : missing ';' before '<'
> 
> I have no idea what's causing this! Hopefully I'll have some time later today to try building this patch myself.

template<typename StringType1, typename StringType2>
class StringTypeAdapter<StringAppend<StringType1, StringType2> > {

I'm not sure where it exactly fails. StringOperators.h should include StringConcatenate.h which defines StringTypeAdapter. I hope you'll find it out using a local build.
Comment 63 Zoltan Herczeg 2011-05-03 04:35:09 PDT
According to g++ -H, the qstring.h is included:

..... ../../../Source/JavaScriptCore/wtf/Vector.h
...... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/QDataStream
....... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qdatastream.h
........ /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qiodevice.h
......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qobject.h
.......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qstring.h

In short, Vector.h has this define:

#if PLATFORM(QT)
#include <QDataStream>
#endif

which pulls the qstring.h
Comment 64 Nikolas Zimmermann 2011-05-03 04:45:35 PDT
(In reply to comment #63)
> According to g++ -H, the qstring.h is included:
> 
> ..... ../../../Source/JavaScriptCore/wtf/Vector.h
> ...... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/QDataStream
> ....... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qdatastream.h
> ........ /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qiodevice.h
> ......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qobject.h
> .......... /nfs_root_dir/usr/local/Trolltech/Qt-4.7.2/include/QtCore/qstring.h
> 
> In short, Vector.h has this define:
> 
> #if PLATFORM(QT)
> #include <QDataStream>
> #endif
> 
> which pulls the qstring.h

Possible workaround for the Qt port that doesn't involve changing WebCore files (as Ossys workaround does).
I've noticed that qstring.h has a conditon around operator+ (#ifndef QT_USE_FAST_OPERATOR_PLUS), so we might workaround this, by disabling the QString operator+ completly when building within WebCore.

How about defining QT_USE_FAST_OPERATOR_PLUS then including QDataStream?
Can someone with a Qt build try?

Thanks in advance!
Comment 65 Darin Adler 2011-05-03 11:08:09 PDT
I’d like our operator+ to be compatible with the QString operator+. I don’t understand why they end up conflicting. All the discussion here has been about avoiding including the QString header; I don’t see any discussion or details about the actual conflict. As long as one of the two arguments to operator+ is a String or something else from WebCore I’d be surprised if it conflicted with the QString operator.
Comment 66 Nikolas Zimmermann 2011-05-03 13:44:46 PDT
 (In reply to comment #65)
> I’d like our operator+ to be compatible with the QString operator+. I don’t understand why they end up conflicting. All the discussion here has been about avoiding including the QString header; I don’t see any discussion or details about the actual conflict. As long as one of the two arguments to operator+ is a String or something else from WebCore I’d be surprised if it conflicted with the QString operator.

Hey Darin,

I think comment 58 explains it well:

> I have a quick look at the Qt build, and attached a quick workaround.
> I know it isn't a good solution for this problem, but it can show
> what happened. It seems, that the problem is around operator+ when
> the left side is char* and the right side is String. I think the
> compiler can't determine which operator+ should be used here:
> a) const QString operator+(const char*, const QString&)
> b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)

I guess you missed that comment.
Comment 67 Adam Roben (:aroben) 2011-05-03 13:46:48 PDT
(In reply to comment #58)
> Created an attachment (id=91831) [details]
> workaround for Patch v13 to make Qt build happy
> 
> I have a quick look at the Qt build, and attached a quick workaround.
> I know it isn't a good solution for this problem, but it can show
> what happened. It seems, that the problem is around operator+ when
> the left side is char* and the right side is String. I think the
> compiler can't determine which operator+ should be used here:
> a) const QString operator+(const char*, const QString&)
> b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)

Can't we fix this by adding this operator?

WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);
Comment 68 Darin Adler 2011-05-03 13:48:05 PDT
(In reply to comment #66)
> Hey Darin,
> 
> I think comment 58 explains it well:
> 
> > I have a quick look at the Qt build, and attached a quick workaround.
> > I know it isn't a good solution for this problem, but it can show
> > what happened. It seems, that the problem is around operator+ when
> > the left side is char* and the right side is String. I think the
> > compiler can't determine which operator+ should be used here:
> > a) const QString operator+(const char*, const QString&)
> > b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> 
> I guess you missed that comment.

I think the simplest solution to that is to explicitly overload operator+ for the case where the left side is const char* and the right side is const String&. While it’s inelegant to have to have that extra function, it would do no harm.

On a separate note, I also think we want that case optimized so that the char* doesn’t get converted to a string before we create the concatenated string. Is that optimized or not?
Comment 69 Nikolas Zimmermann 2011-05-04 01:44:50 PDT
(In reply to comment #67)
> (In reply to comment #58)
> > Created an attachment (id=91831) [details] [details]
> > workaround for Patch v13 to make Qt build happy
> > 
> > I have a quick look at the Qt build, and attached a quick workaround.
> > I know it isn't a good solution for this problem, but it can show
> > what happened. It seems, that the problem is around operator+ when
> > the left side is char* and the right side is String. I think the
> > compiler can't determine which operator+ should be used here:
> > a) const QString operator+(const char*, const QString&)
> > b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> 
> Can't we fix this by adding this operator?
> 
> WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);

Hm, I had this in earlier patches, not sure why I removed it, will test...

(In reply to comment #68)
> (In reply to comment #66)
> > Hey Darin,
> > 
> > I think comment 58 explains it well:
> > 
> > > I have a quick look at the Qt build, and attached a quick workaround.
> > > I know it isn't a good solution for this problem, but it can show
> > > what happened. It seems, that the problem is around operator+ when
> > > the left side is char* and the right side is String. I think the
> > > compiler can't determine which operator+ should be used here:
> > > a) const QString operator+(const char*, const QString&)
> > > b) WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T)
> > 
> > I guess you missed that comment.
> 
> I think the simplest solution to that is to explicitly overload operator+ for the case where the left side is const char* and the right side is const String&. While it’s inelegant to have to have that extra function, it would do no harm.
I'll try Adams suggestion first.

> 
> On a separate note, I also think we want that case optimized so that the char* doesn’t get converted to a string before we create the concatenated string. Is that optimized or not?
That's the purpose of the templatified operator+:
template<typename T>
StringAppend<String, T> operator+(const String& string1, T string2)
To optimize for the case, where you concat a String with eg. a char*.

Going to try to fix the Qt build by supllying the <T, String> operator now...
Comment 70 Nikolas Zimmermann 2011-05-04 01:52:45 PDT
(In reply to comment #67)
> Can't we fix this by adding this operator?
> 
> WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);

I remember why I removed it initially, if we provide both operator+(T, const String&) and operator+(const String&, T) it clashes if T=String.

/Users/nzimmermann/Coding/WebKit/Source/WebCore/storage/AbstractDatabase.cpp:272: error: ambiguous overload for 'operator+' in 'WTF::operator+(T, const WTF::String&) [with T = const char*](((const WTF::String&)((const WTF::String*)WebCore::AbstractDatabase::databaseInfoTableName()))) + " (key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value TEXT NOT NULL ON CONFLICT FAIL);"'
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:95: note: candidates are: WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&) [with T = WTF::StringAppend<const char*, WTF::String>]
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:89: note:                 WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T) [with T = const char*]

Let's see if I can solve that ambiguity.
Comment 71 Nikolas Zimmermann 2011-05-04 02:10:19 PDT
Created attachment 92201 [details]
Patch v14

Experimenting with Darins suggestion, to see whether Qt builds fine now.
Comment 72 Early Warning System Bot 2011-05-04 02:56:54 PDT
Attachment 92201 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8552473
Comment 73 Build Bot 2011-05-04 03:22:35 PDT
Attachment 92201 [details] did not build on win:
Build output: http://queues.webkit.org/results/8556475
Comment 74 Nikolas Zimmermann 2011-05-04 04:20:39 PDT
Created attachment 92215 [details]
Patch v15

Additionally supply "StringAppend<const char*, AtomicString> operator+(const char* string1, const AtomicString& string2)", exposed by editing/markup.cpp (Qt build failure).
Even with that fix win won't build properly, there's still a cyclic include problem, to be investigated.
Comment 75 WebKit Review Bot 2011-05-04 04:44:04 PDT
Attachment 92201 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8556492
Comment 76 WebKit Review Bot 2011-05-04 05:10:41 PDT
Attachment 92201 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8552501
Comment 77 Build Bot 2011-05-04 05:25:05 PDT
Attachment 92215 [details] did not build on win:
Build output: http://queues.webkit.org/results/8552502
Comment 78 WebKit Review Bot 2011-05-04 05:38:49 PDT
Attachment 92201 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8560004
Comment 79 WebKit Review Bot 2011-05-04 06:08:52 PDT
Attachment 92201 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8558494
Comment 80 Adam Roben (:aroben) 2011-05-04 07:02:49 PDT
(In reply to comment #70)
> (In reply to comment #67)
> > Can't we fix this by adding this operator?
> > 
> > WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&);
> 
> I remember why I removed it initially, if we provide both operator+(T, const String&) and operator+(const String&, T) it clashes if T=String.
> 
> /Users/nzimmermann/Coding/WebKit/Source/WebCore/storage/AbstractDatabase.cpp:272: error: ambiguous overload for 'operator+' in 'WTF::operator+(T, const WTF::String&) [with T = const char*](((const WTF::String&)((const WTF::String*)WebCore::AbstractDatabase::databaseInfoTableName()))) + " (key TEXT NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE,value TEXT NOT NULL ON CONFLICT FAIL);"'
> /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:95: note: candidates are: WTF::StringAppend<T, WTF::String> WTF::operator+(T, const WTF::String&) [with T = WTF::StringAppend<const char*, WTF::String>]
> /Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/StringOperators.h:89: note:                 WTF::StringAppend<WTF::String, T> WTF::operator+(const WTF::String&, T) [with T = const char*]
> 
> Let's see if I can solve that ambiguity.

Maybe adding an operator that explicitly takes two Strings would do it.
Comment 81 Nikolas Zimmermann 2011-05-04 11:38:26 PDT
(In reply to comment #80)
> > Let's see if I can solve that ambiguity.
> 
> Maybe adding an operator that explicitly takes two Strings would do it.
I already tried that :-) Adding an explicit "StringAppend<String, String> operator+(const String&, const String&)" just results in a third case that's ambiguous, according to gcc.

See Patch v15 for the solution that actually works with Qt - that problem is resolved now.
Patch v15 should build everywhere except win, unfortunately cr-linux bot is behind, gtk results will come soon.

The win build failure is an include issue, I'm trying to reorganize the StringOperators.h/StringConcatenate.h inclusion to avoid the problem, still waiting for my local mac build to finish.

I hope that solution will work on win as well, tired of 1.5h build-test-build cycles... :-) Stay tuned...
Comment 82 Darin Adler 2011-05-04 11:54:32 PDT
Once we get this compiling I want to make sure that we get the optimal case for various combinations of types that are being concatenated. In theory, as long as either the first or second item is a String, the entire concatenation should work with the same performance as a makeString call.
Comment 83 Nikolas Zimmermann 2011-05-04 12:07:37 PDT
(In reply to comment #82)
> Once we get this compiling I want to make sure that we get the optimal case for various combinations of types that are being concatenated. In theory, as long as either the first or second item is a String, the entire concatenation should work with the same performance as a makeString call.

Note, the first approach (https://bugs.webkit.org/attachment.cgi?id=91364&action=prettypatch) directly used makeString() calls for 2/3/4 arguments that are added, and fell back to StringBuilder for > 4 arguments.

The current approach generates the same code as using makeString() for N arguments, where N is arbitary, as the template is defined recursive. The special StringTypeAdapter<> for StringAppend<T1,T2> types take care of that. The required allocations, etc. should be equal to using makeString - if it's not it's a bug. I'll look again closely, once the build problems are fixed.

Please verify that my comments are correct. We should get consensus that this approach is fine.
Comment 84 Darin Adler 2011-05-04 12:10:17 PDT
(In reply to comment #83)
> We should get consensus that this approach is fine.

The approach is great. I just want to be sure that for cases like this:

    const char* a;
    String b;
    const char* c;
    String d = a + b + c;

We get the single makeString call. I didn’t study the template well enough to be sure that would be the case.
Comment 85 Nikolas Zimmermann 2011-05-04 13:22:46 PDT
Created attachment 92308 [details]
Patch v16

New patch, new luck.
Comment 86 Build Bot 2011-05-04 14:38:40 PDT
Attachment 92308 [details] did not build on win:
Build output: http://queues.webkit.org/results/8553635
Comment 87 Nikolas Zimmermann 2011-05-05 03:18:56 PDT
Created attachment 92399 [details]
Patch v17

New day, new patch. Another attempt to fix the win build, by moving StringAppend right into StringConcatenate.h
Comment 88 Build Bot 2011-05-05 03:54:03 PDT
Attachment 92399 [details] did not build on win:
Build output: http://queues.webkit.org/results/8554869
Comment 89 WebKit Review Bot 2011-05-05 04:37:57 PDT
Attachment 92399 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8558857
Comment 90 WebKit Review Bot 2011-05-05 05:24:24 PDT
Attachment 92399 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8571417
Comment 91 WebKit Review Bot 2011-05-05 06:32:55 PDT
Attachment 92399 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8557918
Comment 92 Adam Roben (:aroben) 2011-05-05 09:08:59 PDT
I think this is the problem on Windows.

PathWalker.cpp (e.g.) has these #includes:

#include "config.h"
#include "PathWalker.h"

#include <wtf/text/StringConcatenate.h>

(It uses makeString() in its implementation.)

StringConcatenate.h then leads to these recursive #includes:

AtomicString.h
   WTFString.h

But note this code near the end of WTFString.h:

#ifndef AtomicString_h
#include "StringOperators.h"
#endif

So StringOperators.h never gets pulled in. Then StringConcatenate.h tries to use StringAppend (which is defined in StringOperators.h) and fails.
Comment 93 Nikolas Zimmermann 2011-05-10 07:07:26 PDT
Created attachment 92950 [details]
Patch v18

New attempt - including StringConcatenate.h directly hopefully works now... (Adams hint)
Comment 94 WebKit Review Bot 2011-05-10 07:52:10 PDT
Comment on attachment 92950 [details]
Patch v18

Attachment 92950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8653969
Comment 95 WebKit Review Bot 2011-05-10 09:15:22 PDT
Comment on attachment 92950 [details]
Patch v18

Attachment 92950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8655948
Comment 96 WebKit Review Bot 2011-05-10 10:22:26 PDT
Comment on attachment 92950 [details]
Patch v18

Attachment 92950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8685026
Comment 97 Nikolas Zimmermann 2011-05-10 12:51:48 PDT
Created attachment 92995 [details]
Patch v19

Adapt to chromium gtest change, try to fix cr-linux/cr-mac builds.
Comment 98 Nikolas Zimmermann 2011-05-11 04:50:47 PDT
Created attachment 93105 [details]
Patch v20

Another attempt at fixing the win cyclic include problem. If this still won't work I'm lost....
Note: This is likely to break other ports again :( If so, Patch v19 is the reference patch that builds everywhere except on win.
Comment 99 Adam Roben (:aroben) 2011-05-11 06:10:03 PDT
Comment on attachment 93105 [details]
Patch v20

I built this patch without trouble on Windows.
Comment 100 Nikolas Zimmermann 2011-05-11 06:14:47 PDT
(In reply to comment #99)
> (From update of attachment 93105 [details])
> I built this patch without trouble on Windows.
Patch v20?
Comment 101 Adam Roben (:aroben) 2011-05-11 06:15:36 PDT
(In reply to comment #100)
> (In reply to comment #99)
> > (From update of attachment 93105 [details] [details])
> > I built this patch without trouble on Windows.
> Patch v20?

Yes. (That's what attachment 93105 [details] is.)
Comment 102 Darin Adler 2011-05-11 09:03:15 PDT
Comment on attachment 93105 [details]
Patch v20

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

Congratulations on getting this to compile!

I’m going to say review+ but I think that some of the changes here are not good and we need some additional refinement after landing this.

> Source/JavaScriptCore/wtf/text/AtomicString.h:205
>  
> +#include "StringConcatenate.h"
>  #endif // AtomicString_h

I’m glad you finally got this working, but the way all these headers include other headers remains a mess. For example, there’s no difference between including AtomicString.h and StringConcatenate.h now; including either always includes both. I’m OK landing like this, but I think we need to work on improving this over time.

> Source/JavaScriptCore/wtf/text/StringOperators.h:4
> + * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> + * Copyright (C) Research In Motion Limited 2011. All rights reserved.

Given what code was moved to this file, I think you moved too many operators. I’m pretty sure none of this code was written by Lars.

> Source/JavaScriptCore/wtf/text/WTFString.cpp:961
> +StringAppend<const char*, String> operator+(const char* string1, const String& string2)
> +{
> +    return StringAppend<const char*, String>(string1, string2);
> +}
> +
> +StringAppend<const char*, AtomicString> operator+(const char* string1, const AtomicString& string2)
> +{
> +    return StringAppend<const char*, AtomicString>(string1, string2);
> +}

These should be inlines in the header. The StringAppend optimization works better if the entire thing can be evaluated at compile time and ends compiling down to a single function call. Having these functions non-inline will create unwanted function calls.

> Source/JavaScriptCore/wtf/text/WTFString.cpp:967
> +const String& emptyString()
> +{
> +    DEFINE_STATIC_LOCAL(String, emptyString, (""));
> +    return emptyString;
> +}

I think this should return the same value as emptyAtom, not a distinct empty string.

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:743
> -        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : AtomicString(attrPrefix + ":" + toString(attributes[i].localname));
> +        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : attrPrefix + colonString + toString(attributes[i].localname);

This is a step in the wrong direction. The string + operator should support appending a single character or a char* without first converting it to a String, so it should not have been necessary to make any code change here at all.

> Source/WebCore/editing/MarkupAccumulator.cpp:213
> -    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
> +    AtomicString attr = prefix.isEmpty() ? xmlnsAtom : xmlnsAtom + ":" + prefix;

This seems worse than before. Why take a string with "xmlns" and then at runtime, append a second string with ":" in it, when the xmlns and the ":" could be concatenated at compile time? We end up concatenating three strings when we could just concatenate two. It's an improvement to use xmlnsAtom when we are not concatenating, though.

> Source/WebCore/html/ImageInputType.cpp:63
> +    encoding.appendData(name.isEmpty() ? xString : (name + "." + xString), m_clickLocation.x());
> +    encoding.appendData(name.isEmpty() ? yString : (name + "." + yString), m_clickLocation.y());

Here it is not sensible to separately concatenate the "." and then the "x". That's extra work at runtime for no reason. We should just concatenate ".x" as before. It's fine to use xString for the other side of the ternary operator, though.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:673
> +        AtomicString prefixColonLocalName = prefix + colonString + localName;

Same comment as above about the colon. With makeString we would have been able to use a single character ':' and avoid having to even call strlen, much less having to look at the length of a preallocated String object. Why can’t our operator+ do the same?

> Source/WebCore/loader/CrossOriginAccessControl.cpp:113
> -        errorDescription = (accessControlOriginString == "*") ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true."
> +        errorDescription = (accessControlOriginString == "*") ? String("Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true.")
>              : "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";

Why exactly is this needed now if it wasn’t needed before? The relationship to the operator+ change is not clear, nor is it explained by the change log. Maybe the issue is that we have to have one side of the ? : be an explicit String to make sure that the other side gets converted from StringAdapter to String? If so, then I suggest getting these changes to explicitly make a String landed separately before we add the StringAdapter, just to make the StringAdapter change smaller.

> Source/WebCore/page/NavigatorBase.cpp:90
> +    if (WEBCORE_NAVIGATOR_PLATFORM != emptyString())
>          return WEBCORE_NAVIGATOR_PLATFORM;

There is an isEmpty function that should be used instead of comparing with an empty string.

> Source/WebKit/chromium/src/WebHTTPLoadInfo.cpp:108
> -        result.first->second += String("\n") + value;
> +        result.first->second += String("\n") + String(value);

This is unnecessarily inefficient. Why turn the "\n" into a String before concatenating it?

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:308
> -    result += "<" + element->nodeName().lower();
> +    result += String("<") + element->nodeName().lower();

Seems like a step in the wrong direction. Explicitly making a String out of the "<" before concatenating.

> Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:337
> -                                result += "./" + param->directoryName + "/";
> +                                result += String("./") + param->directoryName + "/";

Seems like a step in the wrong direction. Explicitly making a String out of the "./" before concatenating.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:186
> -    EXPECT_EQ(testStringA, decodeString(v.data(), v.data() + v.size()));
> +    EXPECT_EQ(String(testStringA), decodeString(v.data(), v.data() + v.size()));
>  
>      v = encodeString(String(testStringB));
> -    EXPECT_EQ(testStringB, decodeString(v.data(), v.data() + v.size()));
> +    EXPECT_EQ(String(testStringB), decodeString(v.data(), v.data() + v.size()));

Why is this change needed? How is it related to the operator+ change?

> Source/WebKit/mac/WebView/WebFrame.mm:495
> -    return _private->coreFrame->documentTypeString() + markupString;
> +    return String(_private->coreFrame->documentTypeString() + String(markupString));

I wish we could keep this simpler, but I guess I can live with it like this.
Comment 103 Nikolas Zimmermann 2011-05-12 00:44:26 PDT
(In reply to comment #102)
> (From update of attachment 93105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93105&action=review
> 
> Congratulations on getting this to compile!
Thanks :-)

> I’m going to say review+ but I think that some of the changes here are not good and we need some additional refinement after landing this.
Great, I'd still upload my new patch with r? to give it a EWS run.

> 
> > Source/JavaScriptCore/wtf/text/AtomicString.h:205
> >  
> > +#include "StringConcatenate.h"
> >  #endif // AtomicString_h
> 
> I’m glad you finally got this working, but the way all these headers include other headers remains a mess. For example, there’s no difference between including AtomicString.h and StringConcatenate.h now; including either always includes both. I’m OK landing like this, but I think we need to work on improving this over time.
Agreed. I think AtomicString.h / StringConcatenate.h includes should all be replaced by a single WTFString.h include, if there aren't any convincing arguments against this.
 
> > Source/JavaScriptCore/wtf/text/StringOperators.h:4
> > + * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
> > + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> > + * Copyright (C) Research In Motion Limited 2011. All rights reserved.
> 
> Given what code was moved to this file, I think you moved too many operators. I’m pretty sure none of this code was written by Lars.
Right, fixed.

> 
> > Source/JavaScriptCore/wtf/text/WTFString.cpp:961
> > +StringAppend<const char*, String> operator+(const char* string1, const String& string2)
> > +{
> > +    return StringAppend<const char*, String>(string1, string2);
> > +}
> > +
> > +StringAppend<const char*, AtomicString> operator+(const char* string1, const AtomicString& string2)
> > +{
> > +    return StringAppend<const char*, AtomicString>(string1, string2);
> > +}
> 
> These should be inlines in the header. The StringAppend optimization works better if the entire thing can be evaluated at compile time and ends compiling down to a single function call. Having these functions non-inline will create unwanted function calls.
Trying.

> 
> > Source/JavaScriptCore/wtf/text/WTFString.cpp:967
> > +const String& emptyString()
> > +{
> > +    DEFINE_STATIC_LOCAL(String, emptyString, (""));
> > +    return emptyString;
> > +}
> 
> I think this should return the same value as emptyAtom, not a distinct empty string.
Or using DEFINE_STATIC_LOCAL(String, emptyString, (StringImpl::empty()) ?

> 
> > Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:743
> > -        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : AtomicString(attrPrefix + ":" + toString(attributes[i].localname));
> > +        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : attrPrefix + colonString + toString(attributes[i].localname);
> 
> This is a step in the wrong direction. The string + operator should support appending a single character or a char* without first converting it to a String, so it should not have been necessary to make any code change here at all.
The problem is the ternary operator usage, both arguments of the operator have to be of the same type, otherwhise the operator AtomicString() can't be invoked on the StringAppend<...> object. This workaround is needed in each place using the ternary operator with concatted Strings and either a String/AtomicString operand.

> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:213
> > -    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
> > +    AtomicString attr = prefix.isEmpty() ? xmlnsAtom : xmlnsAtom + ":" + prefix;
> 
> This seems worse than before. Why take a string with "xmlns" and then at runtime, append a second string with ":" in it, when the xmlns and the ":" could be concatenated at compile time? We end up concatenating three strings when we could just concatenate two. It's an improvement to use xmlnsAtom when we are not concatenating, though.
Fixed by introducing a static local "xmlns:" xmlnsWithColon String.

> 
> > Source/WebCore/html/ImageInputType.cpp:63
> > +    encoding.appendData(name.isEmpty() ? xString : (name + "." + xString), m_clickLocation.x());
> > +    encoding.appendData(name.isEmpty() ? yString : (name + "." + yString), m_clickLocation.y());
> 
> Here it is not sensible to separately concatenate the "." and then the "x". That's extra work at runtime for no reason. We should just concatenate ".x" as before. It's fine to use xString for the other side of the ternary operator, though.
Ok, I'll introduce two other xDotString/yDotString statics.

> 
> > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:673
> > +        AtomicString prefixColonLocalName = prefix + colonString + localName;
> 
> Same comment as above about the colon. With makeString we would have been able to use a single character ':' and avoid having to even call strlen, much less having to look at the length of a preallocated String object. Why can’t our operator+ do the same?
Right -- most of the things you found are relicts from the time, where I didn't have the specialied const char* operator+ variants, it should work now without these hacks.

> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:113
> > -        errorDescription = (accessControlOriginString == "*") ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true."
> > +        errorDescription = (accessControlOriginString == "*") ? String("Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true.")
> >              : "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";
> 
> Why exactly is this needed now if it wasn’t needed before? The relationship to the operator+ change is not clear, nor is it explained by the change log. Maybe the issue is that we have to have one side of the ? : be an explicit String to make sure that the other side gets converted from StringAdapter to String? If so, then I suggest getting these changes to explicitly make a String landed separately before we add the StringAdapter, just to make the StringAdapter change smaller.
Previously, we implicitly converted the first argument of the ? operator to String(), because the String operator+ always returned a String object. Both arguments of operator? have to be of the same type, so the conversion worked fine before. Now with StringAppend, we explicitely have to convert the first side or the second side to a string, otherwhise the operator String() can't be invoked -- this just won't build.

> 
> > Source/WebCore/page/NavigatorBase.cpp:90
> > +    if (WEBCORE_NAVIGATOR_PLATFORM != emptyString())
> >          return WEBCORE_NAVIGATOR_PLATFORM;
> 
> There is an isEmpty function that should be used instead of comparing with an empty string.
Fixed.
e
> 
> > Source/WebKit/chromium/src/WebHTTPLoadInfo.cpp:108
> > -        result.first->second += String("\n") + value;
> > +        result.first->second += String("\n") + String(value);
> 
> This is unnecessarily inefficient. Why turn the "\n" into a String before concatenating it?
Fixed.

> 
> > Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:308
> > -    result += "<" + element->nodeName().lower();
> > +    result += String("<") + element->nodeName().lower();
> 
> Seems like a step in the wrong direction. Explicitly making a String out of the "<" before concatenating.
Fixed.

> 
> > Source/WebKit/chromium/src/WebPageSerializerImpl.cpp:337
> > -                                result += "./" + param->directoryName + "/";
> > +                                result += String("./") + param->directoryName + "/";
> 
> Seems like a step in the wrong direction. Explicitly making a String out of the "./" before concatenating.
Fixed.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:186
> > -    EXPECT_EQ(testStringA, decodeString(v.data(), v.data() + v.size()));
> > +    EXPECT_EQ(String(testStringA), decodeString(v.data(), v.data() + v.size()));
> >  
> >      v = encodeString(String(testStringB));
> > -    EXPECT_EQ(testStringB, decodeString(v.data(), v.data() + v.size()));
> > +    EXPECT_EQ(String(testStringB), decodeString(v.data(), v.data() + v.size()));
> 
> Why is this change needed? How is it related to the operator+ change?
GTest defines a template<typename T1, Typename T2> .. operator+(T1, T2), we conflict with it, I need to cast to WebCore types first, unfortunately. But this is the only place which exposes this problem, so I think it's fine to change.

> 
> > Source/WebKit/mac/WebView/WebFrame.mm:495
> > -    return _private->coreFrame->documentTypeString() + markupString;
> > +    return String(_private->coreFrame->documentTypeString() + String(markupString));
> 
> I wish we could keep this simpler, but I guess I can live with it like this.
:( Yeah...

Okay, I'll try a local build first, let's see how many of the changes work :-)
Note: I'm not landing the patch as-is, but I rather want to fix Darins comments first.
So I'll upload the patch with r?, to let EWS run, if everything still builds I'm landing it.
Comment 104 Nikolas Zimmermann 2011-05-12 01:12:03 PDT
(In reply to comment #103)

I'm replying again with detailed compiler logs, for each of the changes Darin commented, that I've fixed now:

> > > Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:743
> > > -        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : AtomicString(attrPrefix + ":" + toString(attributes[i].localname));
> > > +        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : attrPrefix + colonString + toString(attributes[i].localname);
> > 
> > This is a step in the wrong direction. The string + operator should support appending a single character or a char* without first converting it to a String, so it should not have been necessary to make any code change here at all.
> The problem is the ternary operator usage, both arguments of the operator have to be of the same type, otherwhise the operator AtomicString() can't be invoked on the StringAppend<...> object. This workaround is needed in each place using the ternary operator with concatted Strings and either a String/AtomicString operand.

Reverting the XMLDocumentParserLibxml2.cpp changes results in:
/Users/nzimmermann/Coding/WebKit/Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:742: error: call of overloaded 'AtomicString(WTF::StringAppend<WTF::String, WTF::String>)' is ambiguous
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:50: note: candidates are: WTF::AtomicString::AtomicString(const WTF::String&)
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:39: note:                 WTF::AtomicString::AtomicString(const WTF::AtomicString&)

StringAppend offers both operator String() and operator AtomicString(), so it's not clear which AtomicString constructor should be invoked.

Just removing the AtomicString(...) makes it work fine, as the first argument of the ternary operator is already an AtomicString, so for the second argument, operator AtomicString() of StringAppend<..> is automatically invoked:
-        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : AtomicString(attrPrefix + ":" + toString(attributes[i].localname));
+        AtomicString attrQName = attrPrefix.isEmpty() ? toAtomicString(attributes[i].localname) : attrPrefix + ":" + toString(attributes[i].localname);

This resolves the problem.

> 
> > 
> > > Source/WebCore/editing/MarkupAccumulator.cpp:213
> > > -    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
> > > +    AtomicString attr = prefix.isEmpty() ? xmlnsAtom : xmlnsAtom + ":" + prefix;
> > 
> > This seems worse than before. Why take a string with "xmlns" and then at runtime, append a second string with ":" in it, when the xmlns and the ":" could be concatenated at compile time? We end up concatenating three strings when we could just concatenate two. It's an improvement to use xmlnsAtom when we are not concatenating, though.
> Fixed by introducing a static local "xmlns:" xmlnsWithColon String.

-    AtomicString attr = !prefix.isEmpty() ? "xmlns:" + prefix : "xmlns";
-    return !element->hasAttribute(attr);
+    if (prefix.isEmpty())
+        return !element->hasAttribute(xmlnsAtom);
+
+    DEFINE_STATIC_LOCAL(String, xmlnsWithColon, ("xmlns:"));
+    return !element->hasAttribute(xmlnsWithColon + prefix);

This is the new solution, which should be cleaner and faster.

> 
> > 
> > > Source/WebCore/html/ImageInputType.cpp:63
> > > +    encoding.appendData(name.isEmpty() ? xString : (name + "." + xString), m_clickLocation.x());
> > > +    encoding.appendData(name.isEmpty() ? yString : (name + "." + yString), m_clickLocation.y());
> > 
> > Here it is not sensible to separately concatenate the "." and then the "x". That's extra work at runtime for no reason. We should just concatenate ".x" as before. It's fine to use xString for the other side of the ternary operator, though.
> Ok, I'll introduce two other xDotString/yDotString statics.
Solved this way:
     const AtomicString& name = element()->name();
-    encoding.appendData(name.isEmpty() ? "x" : (name + ".x"), m_clickLocation.x());
-    encoding.appendData(name.isEmpty() ? "y" : (name + ".y"), m_clickLocation.y());
-    if (!name.isEmpty() && !element()->value().isEmpty())
+    if (name.isEmpty()) {
+        encoding.appendData("x", m_clickLocation.x());
+        encoding.appendData("y", m_clickLocation.y());
+        return true;
+    }
+
+    DEFINE_STATIC_LOCAL(String, dotXString, (".x"));
+    DEFINE_STATIC_LOCAL(String, dotYString, (".y"));
+    encoding.appendData(name + dotXString, m_clickLocation.x());
+    encoding.appendData(name + dotYString, m_clickLocation.y());
+
+    if (!!element()->value().isEmpty())
         encoding.appendData(name, element()->value());
     return true;

Arguable more verbose, but also more efficient.

> 
> > 
> > > Source/WebCore/html/parser/HTMLTreeBuilder.cpp:673
> > > +        AtomicString prefixColonLocalName = prefix + colonString + localName;
> > 
> > Same comment as above about the colon. With makeString we would have been able to use a single character ':' and avoid having to even call strlen, much less having to look at the length of a preallocated String object. Why can’t our operator+ do the same?
> Right -- most of the things you found are relicts from the time, where I didn't have the specialied const char* operator+ variants, it should work now without these hacks.
When reverting all changes to HTMLTreeBuilder, I see:

/Users/nzimmermann/Coding/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:672: error: call of overloaded 'AtomicString(WTF::StringAppend<WTF::String, WTF::AtomicString>)' is ambiguous
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:50: note: candidates are: WTF::AtomicString::AtomicString(const WTF::String&)
/Users/nzimmermann/Coding/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/AtomicString.h:39: note:                 WTF::AtomicString::AtomicString(const WTF::AtomicString&)

Fix is very simple:
-        AtomicString prefixColonLocalName(prefix + ":" + localName);
+        AtomicString prefixColonLocalName = prefix + ':' + localName;

> 
> > 
> > > Source/WebCore/loader/CrossOriginAccessControl.cpp:113
> > > -        errorDescription = (accessControlOriginString == "*") ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true."
> > > +        errorDescription = (accessControlOriginString == "*") ? String("Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true.")
> > >              : "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";
> > 
> > Why exactly is this needed now if it wasn’t needed before? The relationship to the operator+ change is not clear, nor is it explained by the change log. Maybe the issue is that we have to have one side of the ? : be an explicit String to make sure that the other side gets converted from StringAdapter to String? If so, then I suggest getting these changes to explicitly make a String landed separately before we add the StringAdapter, just to make the StringAdapter change smaller.
> Previously, we implicitly converted the first argument of the ? operator to String(), because the String operator+ always returned a String object. Both arguments of operator? have to be of the same type, so the conversion worked fine before. Now with StringAppend, we explicitely have to convert the first side or the second side to a string, otherwhise the operator String() can't be invoked -- this just won't build.

I reverted the change to test:
/Users/nzimmermann/Coding/WebKit/Source/WebCore/loader/CrossOriginAccessControl.cpp:113: error: no match for ternary 'operator?:' in 'WTF::operator==(((const WTF::String&)((const WTF::String*)accessControlOriginString)), ((const char*)"*")) ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true." : WTF::operator+(const WTF::String&, T) [with T = const char*](((const char*)" is not allowed by Access-Control-Allow-Origin."))'

A possible fix is to unwrap the ternary operator:
-        errorDescription = (accessControlOriginString == "*") ? "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true."
-            : "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";
+        if (accessControlOriginString == "*")
+            errorDescription = "Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true.";
+        else
+            errorDescription =  "Origin " + securityOrigin->toString() + " is not allowed by Access-Control-Allow-Origin.";

This will build just fine now, I changed my patch.

> 
> > 
> > > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:186
> > > -    EXPECT_EQ(testStringA, decodeString(v.data(), v.data() + v.size()));
> > > +    EXPECT_EQ(String(testStringA), decodeString(v.data(), v.data() + v.size()));
> > >  
> > >      v = encodeString(String(testStringB));
> > > -    EXPECT_EQ(testStringB, decodeString(v.data(), v.data() + v.size()));
> > > +    EXPECT_EQ(String(testStringB), decodeString(v.data(), v.data() + v.size()));
> > 
> > Why is this change needed? How is it related to the operator+ change?
> GTest defines a template<typename T1, Typename T2> .. operator+(T1, T2), we conflict with it, I need to cast to WebCore types first, unfortunately. But this is the only place which exposes this problem, so I think it's fine to change.

From patch v18, cr-linux build output:

Source/WebKit/chromium/testing/gtest/include/gtest/gtest.h:1353:   instantiated from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = UChar [4], T2 = WTF::String, bool lhs_is_null_literal = false]'
Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:183:   instantiated from here
Source/WebKit/chromium/testing/gtest/include/gtest/gtest.h:1316: error: ambiguous overload for 'operator==' in 'expected == actual'
Source/JavaScriptCore/wtf/text/AtomicString.h:138: note: candidates are: bool WTF::operator==(const WTF::String&, const WTF::AtomicString&)
Source/JavaScriptCore/wtf/text/AtomicString.h:136: note:                 bool WTF::operator==(const WTF::AtomicString&, const WTF::String&)
Source/JavaScriptCore/wtf/text/AtomicString.h:133: note:                 bool WTF::operator==(const WTF::AtomicString&, const WTF::AtomicString&)
Source/JavaScriptCore/wtf/text/WTFString.h:361: note:                 bool WTF::operator==(const WTF::String&, const WTF::String&)

Nothing I can do to fix that, from our side...
Comment 105 Nikolas Zimmermann 2011-05-12 02:16:57 PDT
Created attachment 93271 [details]
Patch v21

Testing Darins suggestions with EWS.
Comment 106 Early Warning System Bot 2011-05-12 02:41:46 PDT
Comment on attachment 93271 [details]
Patch v21

Attachment 93271 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8691537
Comment 107 Nikolas Zimmermann 2011-05-12 03:45:20 PDT
Created attachment 93274 [details]
Patch v22
Comment 108 Nikolas Zimmermann 2011-05-12 04:15:53 PDT
Comment on attachment 93274 [details]
Patch v22

Oops patch v22, won't link on mac as I accidentally removed "__ZNK3JSC10JSFunction23isHostFunctionNonInlineEv" from the JavaScriptCore.exp file -- the String append operator changes themselves build/link just fine.
Comment 109 WebKit Review Bot 2011-05-12 04:28:03 PDT
Comment on attachment 93271 [details]
Patch v21

Attachment 93271 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8689677
Comment 110 WebKit Review Bot 2011-05-12 04:47:14 PDT
Comment on attachment 93274 [details]
Patch v22

Attachment 93274 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8691555

New failing tests:
fast/forms/input-image-submit.html
Comment 111 WebKit Review Bot 2011-05-12 04:47:24 PDT
Created attachment 93276 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 112 WebKit Review Bot 2011-05-12 04:50:07 PDT
Comment on attachment 93271 [details]
Patch v21

Attachment 93271 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8685720
Comment 113 Nikolas Zimmermann 2011-05-12 05:18:26 PDT
Landed in r86330.
Comment 114 WebKit Review Bot 2011-05-12 05:52:48 PDT
http://trac.webkit.org/changeset/86330 might have broken Qt Linux Release
The following tests are not passing:
fast/forms/input-image-submit.html
Comment 115 Nikolas Zimmermann 2011-05-12 06:30:26 PDT
Fixed typo leading to regression in fast/forms/input-image-submit.html in r86332.
Comment 116 Nikolas Zimmermann 2011-05-12 06:52:05 PDT
WinCE/WinCairo still have problems - no EWS no way to catch these errors before landing.
Anyhow, landed a possible wince build fix in r86333.
Trying to fix WinCE/WinCairo linking by exporting three symbols in the JSC.def file in r86334.
Comment 117 Nikolas Zimmermann 2011-05-12 07:01:06 PDT
(In reply to comment #116)
> Trying to fix WinCE/WinCairo linking by exporting three symbols in the JSC.def file in r86334.
Reverted again in r86335 - it broke the win build. WinCE build is fixed even w/o this problem.
To summarize: WinCairo is the only platform which remains broken, it doesn't link.

5>WebCore_debug.lib(InspectorDOMAgent.obj) : error LNK2001: unresolved external symbol "class WTF::String __cdecl WTF::operator+(class WTF::String const &,class WTF::String const &)" (??HWTF@@YA?AVString@0@ABV10@0@Z)
5>WebCore_debug.lib(ResourceHandleManager.obj) : error LNK2001: unresolved external symbol "class WTF::String __cdecl WTF::operator+(class WTF::String const &,class WTF::String const &)" (??HWTF@@YA?AVString@0@ABV10@0@Z)

This operator+ variant is declared inline in StringOperators.h, not sure why this happens. I guess I'll have to wait for Brent Fulgham...