Fix ODR violation that can break WTF::String pretty printing in tests.

Fix ODR violation that can break WTF::String pretty printing in tests.

WTF::String's operator<< was declared in a separate header, presumably
to prevent non-test code from depending on it. Unfortunately, this
resulted in ODR violations, since tests that forgot to include this
header would have a different template instantiation of gtest's
DefaultPrintNonContainerTo() vs tests that did remember to include it.

Rather than forcing tests to remember a non-obvious dependency that can
implicitly introduced with no warning, operator<< has been moved to the
same header where WTF::String is declared. In order to prevent it from
being used outside tests, operator<< is still only defined in the test
support library.

BUG=514330
TBR=yhirano@chromium.org

Committed: https://crrev.com/37e7fd401a4ef48a6ce0e0342675e3629a177309
git-svn-id: svn://svn.chromium.org/blink/trunk@199609 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[dcheng, 2015-07-27 19:57:35]

dcheng@chromium.org changed reviewers:
+ jchaffraix@chromium.org

[dcheng, 2015-07-27 19:59:04]

https://codereview.chromium.org/1257203002/diff/20001/Source/wtf/text/WTFStri...
File Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1257203002/diff/20001/Source/wtf/text/WTFStri...
Source/wtf/text/WTFString.h:638: WTF_EXPORT std::ostream&
operator<<(std::ostream&, const String&);
It feels a little sketchy to declare an exported function in one component and
define it in another, even if it does compile.

An alternative I explored was using PrintTo(), but unfortunately, that means
things like:

EXPECT_EQ(a, b) << "more info about b: " << b->fieldThatReturnsString();

need to be rewritten to:
EXPECT_EQ(a, b) << "more info about b: " <<
::testing::PrintToString(b->fieldThatReturnsString());

Ultimately, this felt like the least bad solution.

[dcheng, 2015-07-27 20:28:07]

It seems I spoke to soon, since this breaks on Windows. Let me try to figure out
alternatives...

[dcheng, 2015-07-27 22:10:57]

OK, fixed the build. It's not the nicest thing in the world, but given the
constraints I'm working with:
- Don't cause an ODR violation
- Make it easy for tests to do the Right Thing

Keeping it in a separate header means tests will needlessly (even if harmlessly)
#include something they don't need to. Given that, I think this is the
nicest-looking solution.

[Julien - ping for review, 2015-07-28 17:35:31]

jchaffraix@chromium.org changed reviewers:
+ thakis@chromium.org

[Julien - ping for review, 2015-07-28 17:35:32]

lgtm, that looks reasonable to me.

+Nico as a C++ master in case I missed something.

https://codereview.chromium.org/1257203002/diff/60001/Source/wtf/text/WTFStri...
File Source/wtf/text/WTFString.h (right):

https://codereview.chromium.org/1257203002/diff/60001/Source/wtf/text/WTFStri...
Source/wtf/text/WTFString.h:639: WTF_UNITTEST_HELPERS_EXPORT std::ostream&
operator<<(std::ostream&, const String&);
It seems to me that we would benefit from a printer for WTFString so this should
be a member function instead of a free function.

That's something that wasn't done in WebKit land but would be good IMO.

[Nico, 2015-07-28 17:45:57]

Right now, no non-test files in blink include iosfwd, ostream, sstream,
strstream, iostream (that's all the stream headers I can think of). Having to
remember to include this header is unfortunate, but making our non-test code
depend more on stream headers is maybe worse?

[dcheng, 2015-07-28 17:49:15]

On 2015/07/28 at 17:45:57, thakis wrote:
> Right now, no non-test files in blink include iosfwd, ostream, sstream,
strstream, iostream (that's all the stream headers I can think of). Having to
remember to include this header is unfortunate, but making our non-test code
depend more on stream headers is maybe worse?

Is your concern that non-test code will start using streams by mistake?

[Nico, 2015-07-28 18:22:16]

On 2015/07/28 17:49:15, dcheng wrote:
> On 2015/07/28 at 17:45:57, thakis wrote:
> > Right now, no non-test files in blink include iosfwd, ostream, sstream,
> strstream, iostream (that's all the stream headers I can think of). Having to
> remember to include this header is unfortunate, but making our non-test code
> depend more on stream headers is maybe worse?
> 
> Is your concern that non-test code will start using streams by mistake?

Yes. Currently, it looks like an effort has been made to never include stream
headers in non-test code in blink.

[Julien - ping for review, 2015-07-28 18:29:58]

On 2015/07/28 at 18:22:16, thakis wrote:
> On 2015/07/28 17:49:15, dcheng wrote:
> > On 2015/07/28 at 17:45:57, thakis wrote:
> > > Right now, no non-test files in blink include iosfwd, ostream, sstream,
> > strstream, iostream (that's all the stream headers I can think of). Having
to
> > remember to include this header is unfortunate, but making our non-test code
> > depend more on stream headers is maybe worse?
> > 
> > Is your concern that non-test code will start using streams by mistake?
> 
> Yes. Currently, it looks like an effort has been made to never include stream
headers in non-test code in blink.

This was definitely the case in WebKit land (see the thread "Easing printf based
debugging in WebKit with an helper." from webkit-dev [for some reason the
archive doesn't include the beginning of the thread so no clicky linky]). As far
as Blink is involved, this is news to me.

There is probably a good reason for the rule but I have found the absence of
easy dumping (at least in Debug) super annoying over the years and I know some
engineers who shares this feeling.

[dcheng, 2015-07-28 19:01:49]

On 2015/07/28 at 18:29:58, jchaffraix wrote:
> On 2015/07/28 at 18:22:16, thakis wrote:
> > On 2015/07/28 17:49:15, dcheng wrote:
> > > On 2015/07/28 at 17:45:57, thakis wrote:
> > > > Right now, no non-test files in blink include iosfwd, ostream, sstream,
> > > strstream, iostream (that's all the stream headers I can think of). Having
to
> > > remember to include this header is unfortunate, but making our non-test
code
> > > depend more on stream headers is maybe worse?
> > > 
> > > Is your concern that non-test code will start using streams by mistake?
> > 
> > Yes. Currently, it looks like an effort has been made to never include
stream headers in non-test code in blink.
> 
> This was definitely the case in WebKit land (see the thread "Easing printf
based debugging in WebKit with an helper." from webkit-dev [for some reason the
archive doesn't include the beginning of the thread so no clicky linky]). As far
as Blink is involved, this is news to me.
> 
> There is probably a good reason for the rule but I have found the absence of
easy dumping (at least in Debug) super annoying over the years and I know some
engineers who shares this feeling.

I don't think this is a major issue: since it's just iosfwd in the header,
actually trying to use it would give you an error, right?

The alternative seems strictly worse, and in violation of IWYU.

[Nico, 2015-07-28 19:05:41]

a'ight, lgtm I guess.

/me wishes gtest wouldn't rely on ostream, but it does, and blink now uses gtest
for better or worse.

[dcheng, 2015-07-28 21:40:01]

dcheng@chromium.org changed reviewers:
+ yhirano@chromium.org

[dcheng, 2015-07-28 21:40:17]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-07-28 21:40:17]

TBR yhirano@ for Source/modules/websockets changes.

[commit-bot: I haz the power, 2015-07-28 21:40:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257203002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257203002/60001

[commit-bot: I haz the power, 2015-07-28 21:43:05]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199609

[commit-bot: I haz the power, 2015-09-23 11:51:49]

Patchset 4 (id:??) landed as
https://crrev.com/37e7fd401a4ef48a6ce0e0342675e3629a177309