Factor out StringAppendF into spdy/platform/.

Factor out StringAppendF into spdy/platform/.

This is a follow-up to https://crrev.com/2774673002.

This CL lands server change 151348843 by bnc.

BUG=488484
Review-Url: https://codereview.chromium.org/2778043004
Cr-Commit-Position: refs/heads/master@{#460487}
Committed: https://chromium.googlesource.com/chromium/src/+/50cdd3373b5fd13e9f4e1354d14e3f1c782e4b35

[Bence, 2017-03-28 19:16:17]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 19:16:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 20:25:04]

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

[commit-bot: I haz the power, 2017-03-28 20:25:05]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-03-29 13:16:09]

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

[Bence, 2017-03-29 13:16:10]

diannahu: PTAL.  Thank you.

[diannahu, 2017-03-29 15:42:57]

lgtm

LGTM with minor question.

https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
net/spdy/spdy_alt_svc_wire_format.cc:12: #include "base/strings/string_util.h"
Can this be removed?

[Bence, 2017-03-29 16:56:18]

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

[Bence, 2017-03-29 16:56:19]

xunjieli: Please rubber stamp.
diannahu: Thank you very much for the review.

https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
net/spdy/spdy_alt_svc_wire_format.cc:12: #include "base/strings/string_util.h"
On 2017/03/29 15:42:57, diannahu wrote:
> Can this be removed?

Unfortunately not: it is needed for base::IsHexDigit() and base:HexDigitToInt().

[diannahu, 2017-03-29 17:25:52]

lgtm

[xunjieli, 2017-03-29 17:43:49]

https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
File net/spdy/spdy_alt_svc_wire_format.cc (right):

https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
net/spdy/spdy_alt_svc_wire_format.cc:12: #include "base/strings/string_util.h"
On 2017/03/29 16:56:18, Bence wrote:
> On 2017/03/29 15:42:57, diannahu wrote:
> > Can this be removed?
> 
> Unfortunately not: it is needed for base::IsHexDigit() and
base:HexDigitToInt().

I am still concerned about this approach. Does having the two repos exactly in
sync justify this code churn? Here, we aren't able to get rid of the direct
dependency of base/strings/string_util.h because there are two more methods need
to be migrated. I am concerned that we might introduce subtle bugs this way and
might cause more headaches down the road.

A good enough approach that works for me is to have the merge script to do a
find-and-replace. 

I know QUIC team is doing the same. Is there a discussion thread somewhere?

[Bence, 2017-03-29 18:28:22]

On 2017/03/29 17:43:49, xunjieli wrote:
>
https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
> File net/spdy/spdy_alt_svc_wire_format.cc (right):
> 
>
https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
> net/spdy/spdy_alt_svc_wire_format.cc:12: #include "base/strings/string_util.h"
> On 2017/03/29 16:56:18, Bence wrote:
> > On 2017/03/29 15:42:57, diannahu wrote:
> > > Can this be removed?
> > 
> > Unfortunately not: it is needed for base::IsHexDigit() and
> base:HexDigitToInt().
> 
> I am still concerned about this approach. Does having the two repos exactly in
> sync justify this code churn? Here, we aren't able to get rid of the direct
> dependency of base/strings/string_util.h because there are two more methods
need
> to be migrated. I am concerned that we might introduce subtle bugs this way
and
> might cause more headaches down the road.
> 
> A good enough approach that works for me is to have the merge script to do a
> find-and-replace. 
> 
> I know QUIC team is doing the same. Is there a discussion thread somewhere?

I personally consider the divergence in the SpdyAltSvcWireFormat class a bug:
why use base::IsHexDigit() in Chromium but hack around with std::isdigit() and
std::isxdigit() in the internal codebase?  I think the way forward is to
refactor this, defer to the base:: implementation in Chromium, and if the
internal codebase does not have equivalent functions, then implement them in
spdy/platform/impl, covered by unittests.

[xunjieli, 2017-03-29 18:37:44]

On 2017/03/29 18:28:22, Bence wrote:
> On 2017/03/29 17:43:49, xunjieli wrote:
> >
>
https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
> > File net/spdy/spdy_alt_svc_wire_format.cc (right):
> > 
> >
>
https://codereview.chromium.org/2778043004/diff/1/net/spdy/spdy_alt_svc_wire_...
> > net/spdy/spdy_alt_svc_wire_format.cc:12: #include
"base/strings/string_util.h"
> > On 2017/03/29 16:56:18, Bence wrote:
> > > On 2017/03/29 15:42:57, diannahu wrote:
> > > > Can this be removed?
> > > 
> > > Unfortunately not: it is needed for base::IsHexDigit() and
> > base:HexDigitToInt().
> > 
> > I am still concerned about this approach. Does having the two repos exactly
in
> > sync justify this code churn? Here, we aren't able to get rid of the direct
> > dependency of base/strings/string_util.h because there are two more methods
> need
> > to be migrated. I am concerned that we might introduce subtle bugs this way
> and
> > might cause more headaches down the road.
> > 
> > A good enough approach that works for me is to have the merge script to do a
> > find-and-replace. 
> > 
> > I know QUIC team is doing the same. Is there a discussion thread somewhere?
> 
> I personally consider the divergence in the SpdyAltSvcWireFormat class a bug:
> why use base::IsHexDigit() in Chromium but hack around with std::isdigit() and
> std::isxdigit() in the internal codebase?  I think the way forward is to
> refactor this, defer to the base:: implementation in Chromium, and if the
> internal codebase does not have equivalent functions, then implement them in
> spdy/platform/impl, covered by unittests.

lgtm

[Bence, 2017-03-29 19:07:24]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2017-03-29 19:08:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 19:25:51]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490814444707490, "parent_rev":
"eb232647e62ce3b932af9d22fbe7572401ad3589", "commit_rev":
"50cdd3373b5fd13e9f4e1354d14e3f1c782e4b35"}

[commit-bot: I haz the power, 2017-03-29 19:26:38]

Description was changed from

==========
Factor out StringAppendF into spdy/platform/.

This is a follow-up to https://crrev.com/2774673002.

This CL lands server change 151348843 by bnc.

BUG=488484
==========

to

==========
Factor out StringAppendF into spdy/platform/.

This is a follow-up to https://crrev.com/2774673002.

This CL lands server change 151348843 by bnc.

BUG=488484

Review-Url: https://codereview.chromium.org/2778043004
Cr-Commit-Position: refs/heads/master@{#460487}
Committed:
https://chromium.googlesource.com/chromium/src/+/50cdd3373b5fd13e9f4e1354d14e...
==========

[commit-bot: I haz the power, 2017-03-29 19:26:45]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/50cdd3373b5fd13e9f4e1354d14e...