Rename WriteString to WriteStringPiece16 in SpdyFrameBuilder

Rename WriteString to WriteStringPiece16 in SpdyFrameBuilder

As the comment in net/spdy/spdy_frame_builder.h by hkhalil@,
rename WriteString to WriteStringPiece16.

BUG=458880
R=bnc@chromium.org

Committed: https://crrev.com/d588d971032596c63dd082b3d5c7438997467ddd
Cr-Commit-Position: refs/heads/master@{#319804}

Committed: https://crrev.com/848a3392144b624965d8e143e1e253380eaf9824
Cr-Commit-Position: refs/heads/master@{#321049}

[zhuoyu.qian, 2015-02-16 00:53:25]

There is a patch, please have a review.
Thank you.

[asanka, 2015-02-17 18:53:06]

not lgtm.

The suggestion isn't to literally give it a new name, but to make it use a
StringPiece as well.

[zhuoyu.qian, 2015-02-26 03:24:27]

Update the CL as the comment above.
PHAL. =)

[asanka, 2015-03-02 21:39:51]

asanka@chromium.org changed reviewers:
+ bnc@chromium.org

[asanka, 2015-03-02 21:39:51]

asanka@chromium.org changed required reviewers:
+ bnc@chromium.org

[asanka, 2015-03-02 21:40:21]

asanka -> bnc

[Bence, 2015-03-04 20:05:35]

bnc@chromium.org changed reviewers:
+ mmenke@chromium.org

[Bence, 2015-03-04 20:05:35]

Matt: please help me out here.  The CL looks okay to me, especially because it
makes the signature of WriteStringPiece16 and WriteStringPiece32 match, but:

1. We never call these methods with StringPieces.  SpdyHeaderBlock::iterator is
a std::string, and we use C style string literals in tests.  This adds an extra
unnecessary implicit conversion to StringPiece.  Should we not change
WriteStringPiece32(const StringPiece&) to WriteString32(const string&) instead?

2. If we prefer StringPiece for whatever reason, as StringPiece is small, do we
not actually want to pass it by value instead of const reference for
performance?  I know there was a lot of discussion about this, what is the
conclusion for net/?

Thanks.

[mmenke, 2015-03-04 20:30:04]

On 2015/03/04 20:05:35, Bence wrote:
> Matt: please help me out here.  The CL looks okay to me, especially because it
> makes the signature of WriteStringPiece16 and WriteStringPiece32 match, but:
> 
> 1. We never call these methods with StringPieces.  SpdyHeaderBlock::iterator
is
> a std::string, and we use C style string literals in tests.  This adds an
extra
> unnecessary implicit conversion to StringPiece.  Should we not change
> WriteStringPiece32(const StringPiece&) to WriteString32(const string&)
instead?

I don't have a strong opinion here, but I'm fine with either approach.  The real
advantage of StringPieces if you need to sometimes call something with a string,
and somethings with a part of a string or part or all of a C string or non-NULL
terminated character buffer, you can do so without copying it into an
std::string.  You could, for instance, imaging making a SpdyHeaderBlock for
writing headers not own its own strings, to reduce overhead.  Of course, the
complexity may well not be worth the marginal benefits in any case where we use
the SpdyFrameBuilder, anyways.

> 2. If we prefer StringPiece for whatever reason, as StringPiece is small, do
we
> not actually want to pass it by value instead of const reference for
> performance?  I know there was a lot of discussion about this, what is the
> conclusion for net/?
> 
> Thanks.

In terms of by reference vs by value:  I don't have a strong opinion.  I guess a
StringPiece takes up either 64 or 128 bits, depending on platform - size_t and a
pointer, I believe?  Sorry I can't give you a definitive answer here.  I think
we do want to provide a consistent interface here, but don't think which way we
go on either of these decisions is really all that important.

[Bence, 2015-03-05 02:19:12]

Matt: thanks for your response.

zhuoyu.qian: LGTM.  Optionally, feel free to change the signature of both
StringPiece{16,32} to take StringPiece by value instead of const reference if
you are so inclined, but I don't have a strong preference either.

[Bence, 2015-03-06 18:52:19]

Asanka, PTAL.  I'm not an owner, so I believe your approval is still required.

[asanka, 2015-03-06 21:47:40]

On 2015/03/06 18:52:19, Bence wrote:
> Asanka, PTAL.  I'm not an owner, so I believe your approval is still required.

Rubberstamp lgtm

[zhuoyu.qian, 2015-03-09 01:37:05]

The CQ bit was checked by zhuoyu.qian@samsung.com

[commit-bot: I haz the power, 2015-03-09 01:38:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393003/40001

[commit-bot: I haz the power, 2015-03-09 02:42:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-09 02:42:46]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 02:42:52]

The CQ bit was unchecked by commit-bot@chromium.org

[zhuoyu.qian, 2015-03-09 03:16:08]

The CQ bit was checked by zhuoyu.qian@samsung.com

[commit-bot: I haz the power, 2015-03-09 03:16:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393003/40001

[commit-bot: I haz the power, 2015-03-09 03:17:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-09 03:17:21]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 03:17:24]

The CQ bit was unchecked by commit-bot@chromium.org

[zhuoyu.qian, 2015-03-09 06:00:17]

The CQ bit was checked by zhuoyu.qian@samsung.com

[commit-bot: I haz the power, 2015-03-09 06:01:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393003/40001

[commit-bot: I haz the power, 2015-03-09 06:02:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-09 06:02:40]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 06:02:48]

The CQ bit was unchecked by commit-bot@chromium.org

[zhuoyu.qian, 2015-03-09 07:08:10]

The CQ bit was checked by zhuoyu.qian@samsung.com

[commit-bot: I haz the power, 2015-03-09 07:09:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393003/40001

[commit-bot: I haz the power, 2015-03-09 07:10:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-09 07:10:04]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-03-09 07:10:10]

The CQ bit was unchecked by commit-bot@chromium.org

[zhuoyu.qian, 2015-03-10 01:07:09]

The CQ bit was checked by zhuoyu.qian@samsung.com

[commit-bot: I haz the power, 2015-03-10 01:08:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393003/40001

[commit-bot: I haz the power, 2015-03-10 01:09:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-10 01:09:39]

Patchset 3 (id:??) landed as
https://crrev.com/d588d971032596c63dd082b3d5c7438997467ddd
Cr-Commit-Position: refs/heads/master@{#319804}

[gab, 2015-03-10 14:05:29]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/991313002/ by gab@chromium.org.

The reason for reverting is: Appears to be causing iOS Device compile errors on
waterfall: https://build.chromium.org/p/chromium.mac/builders/iOS_Device

Last 4 builds all fail with this log:

=== BUILD TARGET net_unittests OF PROJECT net WITH CONFIGURATION Release ===

Check dependencies

Ld
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/net_unittests.app/net_unittests
normal armv7
    cd /Volumes/data/b/build/slave/iOS_Device/build/src/net
    export IPHONEOS_DEPLOYMENT_TARGET=7.0
    export
PATH="/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Applications/Xcode.app/Contents/Developer/usr/bin:/Users/chrome-bot/slavebin:/Volumes/data/b/depot_tools:/usr/local/git/bin:/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin"
   
/Volumes/data/b/build/slave/iOS_Device/build/src/net/../third_party/llvm-build/Release+Asserts/bin/clang++
-arch armv7 -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk
-L/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos
-L/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk/System/Library/Frameworks
-F/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos
-filelist
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/net.build/Release-iphoneos/net_unittests.build/Objects-normal/armv7/net_unittests.LinkFileList
-dead_strip -Wl,-search_paths_first -stdlib=libc++ -miphoneos-version-min=7.0
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libbase.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libbase_i18n.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libbase_prefs_test_support.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libdynamic_annotations.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libcrcrypto.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libgmock.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libgtest.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libchrome_zlib.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/liburl_lib.a
-lbalsa -lnet -lnet_extras -lnet_test_support -lquic_tools
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libcrnspr.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libcrnss.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libcrssl.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libbase_static.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/liballocator_extension_thunks.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libmodp_b64.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libicui18n.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libicuuc.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libbase_prefs.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libnss_static.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libsqlite_regexp.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libcrnssckbi.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libzlib_x86_simd.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libsdch.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libsql.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libtest_support_base.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libxml.a
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/libtld_cleanup_util.a
-framework CoreFoundation -framework CoreGraphics -framework CoreText -framework
Foundation -framework UIKit -lsqlite3 -framework CFNetwork -framework
MobileCoreServices -framework Security -framework SystemConfiguration -lresolv
-lxml2 -Xlinker -dependency_info -Xlinker
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/net.build/Release-iphoneos/net_unittests.build/Objects-normal/armv7/net_unittests_dependency_info.dat
-o
/Volumes/data/b/build/slave/iOS_Device/build/src/xcodebuild/Release-iphoneos/net_unittests.app/net_unittests
Undefined symbols for architecture armv7:
 
"net::SpdyFrameBuilder::WriteStringPiece16(base::BasicStringPiece<std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > > const&)", referenced
from:
      net::SpdyFramerTest_DuplicateHeader_Test::TestBody() in spdy_framer_test.o
      net::SpdyFramerTest_MultiValueHeader_Test::TestBody() in
spdy_framer_test.o
ld: symbol(s) not found for architecture armv7
clang: error: linker command failed with exit code 1 (use -v to see invocation).

[gab, 2015-03-10 14:40:17]

Looks like this revert indeed fixed the build issues on
http://build.chromium.org/p/chromium.mac/builders/iOS_Device.

[Bence, 2015-03-17 19:30:12]

On 2015/03/10 14:40:17, gab wrote:
> Looks like this revert indeed fixed the build issues on
> http://build.chromium.org/p/chromium.mac/builders/iOS_Device.

zhuoyu.qian: I believe you can try landing this CL again.  A couple of other CLs
had been reverted because of a similar error, but then they were relanded:
https://codereview.chromium.org/982883002
https://codereview.chromium.org/986743004
https://codereview.chromium.org/988733003
It is possible that it was just an infrastructure problem.

[zhuoyu.qian, 2015-03-18 00:50:40]

On 2015/03/17 19:30:12, Bence wrote:
> On 2015/03/10 14:40:17, gab wrote:
> > Looks like this revert indeed fixed the build issues on
> > http://build.chromium.org/p/chromium.mac/builders/iOS_Device.
> 
> zhuoyu.qian: I believe you can try landing this CL again.  A couple of other
CLs
> had been reverted because of a similar error, but then they were relanded:
> https://codereview.chromium.org/982883002
> https://codereview.chromium.org/986743004
> https://codereview.chromium.org/988733003
> It is possible that it was just an infrastructure problem.

Thank you. I'm glad to do this, but I don't know how to reland it.
I think I need your advice. 
Thanks for your help.

[Bence, 2015-03-18 00:54:57]

On 2015/03/18 00:50:40, zhuoyu.qian wrote:
> On 2015/03/17 19:30:12, Bence wrote:
> > On 2015/03/10 14:40:17, gab wrote:
> > > Looks like this revert indeed fixed the build issues on
> > > http://build.chromium.org/p/chromium.mac/builders/iOS_Device.
> > 
> > zhuoyu.qian: I believe you can try landing this CL again.  A couple of other
> CLs
> > had been reverted because of a similar error, but then they were relanded:
> > https://codereview.chromium.org/982883002
> > https://codereview.chromium.org/986743004
> > https://codereview.chromium.org/988733003
> > It is possible that it was just an infrastructure problem.
> 
> Thank you. I'm glad to do this, but I don't know how to reland it.
> I think I need your advice. 
> Thanks for your help.

I believe the procedure is the following: Edit issue, uncheck "Closed", Update
issue.  Then check the box next to "Commit:".

[zhuoyu.qian, 2015-03-18 00:56:15]

The CQ bit was checked by zhuoyu.qian@samsung.com

[commit-bot: I haz the power, 2015-03-18 00:56:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910393003/40001

[commit-bot: I haz the power, 2015-03-18 01:58:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-18 01:59:22]

Patchset 3 (id:??) landed as
https://crrev.com/848a3392144b624965d8e143e1e253380eaf9824
Cr-Commit-Position: refs/heads/master@{#321049}