Remove support for Alt-Svc/Alternate Protocol Probability

Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}

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

[Ryan Hamilton, 2016-03-03 19:04:02]

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

[Ryan Hamilton, 2016-03-03 19:04:03]

bnc: PTAL
ianswett: FYI

[Bence, 2016-03-04 13:59:22]

On 2016/03/03 19:04:03, Ryan Hamilton wrote:
> bnc: PTAL
> ianswett: FYI

Please change description to include alternative services, otherwise one might
get the impression that the change only affects Alternate-Protocol.

[Ryan Hamilton, 2016-03-04 15:02:00]

Description was changed from

==========
Remove support for Alternate Protocol Probability
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability
==========

[Ryan Hamilton, 2016-03-04 15:02:09]

On 2016/03/04 13:59:22, Bence wrote:
> On 2016/03/03 19:04:03, Ryan Hamilton wrote:
> > bnc: PTAL
> > ianswett: FYI
> 
> Please change description to include alternative services, otherwise one might
> get the impression that the change only affects Alternate-Protocol.

Done.

[Bence, 2016-03-04 15:07:23]

LGTM

[Ryan Hamilton, 2016-03-04 17:46:53]

eroman: OWNERS review for chrome/browser/resources/net_internals/quic_view.html,
please

[Ryan Hamilton, 2016-03-04 18:28:04]

rch@chromium.org changed reviewers:
+ eroman@chromium.org, mef@chromium.org

[Ryan Hamilton, 2016-03-04 18:28:04]

eroman: OWNERS review for chrome/browser/resources/net_internals/quic_view.html,
please

mef: OWNERS review for components/cronet/android/url_request_context_adapter.cc,
please

[eroman, 2016-03-04 18:37:03]

LGTM for chrome/browser/resources/net_internals/*

[mef, 2016-03-04 18:40:40]

https://codereview.chromium.org/1699653002/diff/60001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/1699653002/diff/60001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:167: // Currently
(circa M39) enabling QUIC requires setting probability threshold.
I guess this comment is now outdated.

https://codereview.chromium.org/1699653002/diff/60001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:168: if
(config_->enable_quic) {
This is legacy api, you also need to update current
api:https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/android/cronet_url_request_context_adapter.cc&l=490

[Ryan Hamilton, 2016-03-04 19:17:57]

https://codereview.chromium.org/1699653002/diff/60001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/1699653002/diff/60001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:167: // Currently
(circa M39) enabling QUIC requires setting probability threshold.
On 2016/03/04 18:40:40, mef wrote:
> I guess this comment is now outdated.

Done.

https://codereview.chromium.org/1699653002/diff/60001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:168: if
(config_->enable_quic) {
On 2016/03/04 18:40:40, mef wrote:
> This is legacy api, you also need to update current
>
api:https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/android/cronet_url_request_context_adapter.cc&l=490

Heh, yeah, I just noticed that :> Done.

[mef, 2016-03-07 19:49:36]

Sorry for the delay, lgtm.

[Ryan Hamilton, 2016-03-07 20:17:22]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-03-07 20:17:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1699653002/#ps120001
(title: "more ios fix")

[commit-bot: I haz the power, 2016-03-07 20:17:43]

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

[commit-bot: I haz the power, 2016-03-07 21:51:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-07 21:52:27]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

Committed: https://crrev.com/dced4c771d25317d26a0f6e258c7b7f64d4e8eea
Cr-Commit-Position: refs/heads/master@{#379652}
==========

[commit-bot: I haz the power, 2016-03-07 21:52:28]

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

[anthonyvd, 2016-03-07 22:02:44]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1773853003/ by anthonyvd@chromium.org.

The reason for reverting is: This appears to have broken the following bot:
https://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/bui....

[mef, 2016-03-07 22:08:44]

On 2016/03/07 22:02:44, anthonyvd wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/1773853003/ by mailto:anthonyvd@chromium.org.
> 
> The reason for reverting is: This appears to have broken the following bot:
>
https://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/bui....

G'Ah, I should've noticed that. Let me try it locally.

[mef, 2016-03-07 22:18:45]

On 2016/03/07 22:08:44, mef wrote:
> On 2016/03/07 22:02:44, anthonyvd wrote:
> > A revert of this CL (patchset #7 id:120001) has been created in
> > https://codereview.chromium.org/1773853003/ by
mailto:anthonyvd@chromium.org.
> > 
> > The reason for reverting is: This appears to have broken the following bot:
> >
>
https://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/bui....
> 
> G'Ah, I should've noticed that. Let me try it locally.

Just removing  the offending line does the trick:

diff --git a/ios/crnet/crnet_consumer/crnet_consumer_app_delegate.mm
b/ios/crnet/crnet_consumer/crnet_consumer_app_delegate.mm
index ca44848..b224eda 100644
--- a/ios/crnet/crnet_consumer/crnet_consumer_app_delegate.mm
+++ b/ios/crnet/crnet_consumer/crnet_consumer_app_delegate.mm
@@ -45,8 +45,6 @@
     didFinishLaunchingWithOptions:(NSDictionary*)launchOptions {
   [CrNet setPartialUserAgent:@"Dummy/1.0"];
   [CrNet setQuicEnabled:YES];
-  // Always use QUIC if able.
-  [CrNet setAlternateProtocolThreshold:0.0];
   [CrNet setSDCHEnabled:YES withPrefStore:[self SDCHPrefStoreFileName]];
   [CrNet install];
   [CrNet startNetLogToFile:[self currentNetLogFileName] logBytes:NO];

[Ryan Hamilton, 2016-03-07 22:49:58]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability

Committed: https://crrev.com/dced4c771d25317d26a0f6e258c7b7f64d4e8eea
Cr-Commit-Position: refs/heads/master@{#379652}
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

Committed: https://crrev.com/dced4c771d25317d26a0f6e258c7b7f64d4e8eea
Cr-Commit-Position: refs/heads/master@{#379652}
==========

[Ryan Hamilton, 2016-03-07 22:59:29]

mef: PTAL

[mef, 2016-03-07 23:00:29]

still lgtm

[Ryan Hamilton, 2016-03-07 23:01:23]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-03-07 23:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1699653002/#ps140001
(title: "Fix iOS more")

[commit-bot: I haz the power, 2016-03-07 23:01:47]

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

[commit-bot: I haz the power, 2016-03-08 00:36:51]

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

[commit-bot: I haz the power, 2016-03-08 00:36:52]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Hamilton, 2016-03-14 23:11:56]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2016-03-14 23:12:19]

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

[commit-bot: I haz the power, 2016-03-15 00:33:35]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability

Committed: https://crrev.com/dced4c771d25317d26a0f6e258c7b7f64d4e8eea
Cr-Commit-Position: refs/heads/master@{#379652}
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

Committed: https://crrev.com/dced4c771d25317d26a0f6e258c7b7f64d4e8eea
Cr-Commit-Position: refs/heads/master@{#379652}
==========

[commit-bot: I haz the power, 2016-03-15 00:33:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-15 00:35:23]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability

Committed: https://crrev.com/dced4c771d25317d26a0f6e258c7b7f64d4e8eea
Cr-Commit-Position: refs/heads/master@{#379652}
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}
==========

[commit-bot: I haz the power, 2016-03-15 00:35:25]

Patchset 8 (id:??) landed as
https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}

[PEConn, 2016-03-15 09:21:46]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1802893002/ by peconn@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=594921

Basically parts of http_stream_factory_impl_unittest.cc are only compiled if
ENABLE_BIDIRECTIONAL_STREAM is a build flag and you haven't updated these parts
of the file..

[Ryan Hamilton, 2016-03-17 00:41:06]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}
==========

[Ryan Hamilton, 2016-03-17 00:41:06]

rch@chromium.org changed reviewers:
+ mmenke@chromium.org, xunjieli@chromium.org

[Ryan Hamilton, 2016-03-17 00:43:47]

mef, et. al.: I thought we had confirmed that all various cronet configurations
were part of the CQ. This CL was reverted because of a post-CQ build failure.

In any case, how can I get a bot to build this with the right options to ensure
I did this correctly.

Also, I *Really* think we should reconsider the decision to sprinkle #ifdefs
throughout the stack because of this new BIDI streaming code. When that CL
landed, I explained that I was concerned about EXACTLY this problem.

[xunjieli, 2016-03-17 00:52:40]

Nope. Cronet configurations aren't part of the CQ. We have a trybot, but it
is in an incorrect state right now (
https://codereview.chromium.org/1750743002/), so you can't really use
it.  I don't really like #ifdefs too (since the compile and unittests
aren't being run on the CQ bots, and code search's cross reference doesn't
work either), but #ifdefs does save some binary footprint on chrome proper,
so I am not sure. Matt, do you have any thought on removing the #ifdefs
from net::BidirectionalStream?

On Wed, Mar 16, 2016 at 8:43 PM, <rch@chromium.org> wrote:

> mef, et. al.: I thought we had confirmed that all various cronet
> configurations
> were part of the CQ. This CL was reverted because of a post-CQ build
> failure.
>
> In any case, how can I get a bot to build this with the right options to
> ensure
> I did this correctly.
>
> Also, I *Really* think we should reconsider the decision to sprinkle
> #ifdefs
> throughout the stack because of this new BIDI streaming code. When that CL
> landed, I explained that I was concerned about EXACTLY this problem.
>
> https://codereview.chromium.org/1699653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2016-03-17 04:09:39]

I suspect the binary savings aren't huge, and it's a reasonable concern.
On the other hand, remove the ifdefs and we may discover a bunch of code is
unexpectedly depending on the C++ APIs, which may also be fine, assuming
we're fairly happy with the C++ APIs, and they don't tend to lead to broken
code (Unlike, say, URLRequest and URLFetcher).

That having been said...If we aren't building Cronet at all on the bots,
removing the #ifdefs doesn't really address the underlying problem.

On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <xunjieli@chromium.org> wrote:

> Nope. Cronet configurations aren't part of the CQ. We have a trybot, but
> it is in an incorrect state right now (
> https://codereview.chromium.org/1750743002/), so you can't really use
> it.  I don't really like #ifdefs too (since the compile and unittests
> aren't being run on the CQ bots, and code search's cross reference doesn't
> work either), but #ifdefs does save some binary footprint on chrome proper,
> so I am not sure. Matt, do you have any thought on removing the #ifdefs
> from net::BidirectionalStream?
>
> On Wed, Mar 16, 2016 at 8:43 PM, <rch@chromium.org> wrote:
>
>> mef, et. al.: I thought we had confirmed that all various cronet
>> configurations
>> were part of the CQ. This CL was reverted because of a post-CQ build
>> failure.
>>
>> In any case, how can I get a bot to build this with the right options to
>> ensure
>> I did this correctly.
>>
>> Also, I *Really* think we should reconsider the decision to sprinkle
>> #ifdefs
>> throughout the stack because of this new BIDI streaming code. When that CL
>> landed, I explained that I was concerned about EXACTLY this problem.
>>
>> https://codereview.chromium.org/1699653002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xunjieli, 2016-03-17 13:51:50]

The #ifdefs are for the build flag, enable_bidirectional_stream. We have
quite a number of C++ unittests in net/. If the #ifdefs (i.e. the build
flag) is removed, net_unittests will build and run net::BidirectionalStream
tests. Given how fast H2 and QUIC code changes, I am voting for removing
the #ifdefs.

On Thu, Mar 17, 2016 at 12:09 AM, Matt Menke <mmenke@chromium.org> wrote:

> I suspect the binary savings aren't huge, and it's a reasonable concern.
> On the other hand, remove the ifdefs and we may discover a bunch of code is
> unexpectedly depending on the C++ APIs, which may also be fine, assuming
> we're fairly happy with the C++ APIs, and they don't tend to lead to broken
> code (Unlike, say, URLRequest and URLFetcher).
>
> That having been said...If we aren't building Cronet at all on the bots,
> removing the #ifdefs doesn't really address the underlying problem.
>
> On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <xunjieli@chromium.org> wrote:
>
>> Nope. Cronet configurations aren't part of the CQ. We have a trybot, but
>> it is in an incorrect state right now (
>> https://codereview.chromium.org/1750743002/), so you can't really use
>> it.  I don't really like #ifdefs too (since the compile and unittests
>> aren't being run on the CQ bots, and code search's cross reference doesn't
>> work either), but #ifdefs does save some binary footprint on chrome proper,
>> so I am not sure. Matt, do you have any thought on removing the #ifdefs
>> from net::BidirectionalStream?
>>
>> On Wed, Mar 16, 2016 at 8:43 PM, <rch@chromium.org> wrote:
>>
>>> mef, et. al.: I thought we had confirmed that all various cronet
>>> configurations
>>> were part of the CQ. This CL was reverted because of a post-CQ build
>>> failure.
>>>
>>> In any case, how can I get a bot to build this with the right options to
>>> ensure
>>> I did this correctly.
>>>
>>> Also, I *Really* think we should reconsider the decision to sprinkle
>>> #ifdefs
>>> throughout the stack because of this new BIDI streaming code. When that
>>> CL
>>> landed, I explained that I was concerned about EXACTLY this problem.
>>>
>>> https://codereview.chromium.org/1699653002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Hamilton, 2016-03-17 14:14:54]

On Wed, Mar 16, 2016 at 9:09 PM, Matt Menke <mmenke@chromium.org> wrote:

> I suspect the binary savings aren't huge, and it's a reasonable concern.
> On the other hand, remove the ifdefs and we may discover a bunch of code is
> unexpectedly depending on the C++ APIs, which may also be fine, assuming
> we're fairly happy with the C++ APIs, and they don't tend to lead to broken
> code (Unlike, say, URLRequest and URLFetcher).
>
> That having been said...If we aren't building Cronet at all on the bots,
> removing the #ifdefs doesn't really address the underlying problem.
>

​Heh, yes, that's very true. I think there are two issues at the moment:

1) The CQ does not seem to build Cronet and there is not trybot to do so
either. This means that if someone breaks the Cronet build, they only find
out when their CL​ is reverted. I think in the past, the Cronet main bot
was made an FYI bot when we were in this state. CLs which broke that bot
were not reverted, but patches were cooked up to make the bot green again.
I think we *definitely* want the CQ running Cronet if the Cronet bots
trigger reverts.

2) The #ifdefs are sprinkled throughout a number of files in net (which
change regularly). These #ifdefs act like little landmines, just waiting
for a CL to trigger them (as in my case). This slows down development. They
also mean that code search does not work for new BIDI code which makes it
harder to reason about.

If, as you say, the binary savings are not huge, I would strongly support
removing the ifdefs.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Hamilton, 2016-03-17 18:32:56]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2016-03-17 18:32:56]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
eroman@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1699653002/#ps160001
(title: "Fix BIDI")

[commit-bot: I haz the power, 2016-03-17 18:33:23]

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

[mef, 2016-03-17 20:25:07]

On 2016/03/17 04:09:39, mmenke wrote:
> I suspect the binary savings aren't huge, and it's a reasonable concern.
> On the other hand, remove the ifdefs and we may discover a bunch of code is
> unexpectedly depending on the C++ APIs, which may also be fine, assuming
> we're fairly happy with the C++ APIs, and they don't tend to lead to broken
> code (Unlike, say, URLRequest and URLFetcher).
Do you mean the danger of somebody using net::BidirectionalStream C++ API
outside of Cronet, like in Chrome proper or something?

> 
> That having been said...If we aren't building Cronet at all on the bots,
> removing the #ifdefs doesn't really address the underlying problem.
FWIW we currently ARE building Cronet on regular Android bots, however we are
NOT testing it there,
and we don't have 'enable_bidirectional_stream' feature flag set.

Also, given that we have a separate Cronet waterfall, we could consider NOT
building Cronet on regular Android bots,
as this takes time and clutters gyp/gn files (specifically net_small and
url_use_icu_alternatives_on_android targets).

> 
> On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <mailto:xunjieli@chromium.org>
wrote:
> 
> > Nope. Cronet configurations aren't part of the CQ. We have a trybot, but
> > it is in an incorrect state right now (
> > https://codereview.chromium.org/1750743002/), so you can't really use
> > it.  I don't really like #ifdefs too (since the compile and unittests
> > aren't being run on the CQ bots, and code search's cross reference doesn't
> > work either), but #ifdefs does save some binary footprint on chrome proper,
> > so I am not sure. Matt, do you have any thought on removing the #ifdefs
> > from net::BidirectionalStream?
I'm +1 for removal of #ifdefs.
> >
> > On Wed, Mar 16, 2016 at 8:43 PM, <mailto:rch@chromium.org> wrote:
> >
> >> mef, et. al.: I thought we had confirmed that all various cronet
> >> configurations
> >> were part of the CQ. This CL was reverted because of a post-CQ build
> >> failure.
> >>
> >> In any case, how can I get a bot to build this with the right options to
> >> ensure I did this correctly.
> >>
> >> Also, I *Really* think we should reconsider the decision to sprinkle
> >> #ifdefs
> >> throughout the stack because of this new BIDI streaming code. When that CL
> >> landed, I explained that I was concerned about EXACTLY this problem.
I hear you, and I agree that for this particular issue removal of
'enable_bidirectional_stream' build flag seems like a reasonable solution.

> >>
> >> https://codereview.chromium.org/1699653002/
> >>
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ryan Hamilton, 2016-03-17 20:40:36]

On 2016/03/17 20:25:07, mef wrote:
> On 2016/03/17 04:09:39, mmenke wrote:
> > 
> > That having been said...If we aren't building Cronet at all on the bots,
> > removing the #ifdefs doesn't really address the underlying problem.
> FWIW we currently ARE building Cronet on regular Android bots, however we are
> NOT testing it there,
> and we don't have 'enable_bidirectional_stream' feature flag set.

Oh, I see. cronet does not necessarily imply enable_bidirectional_stream?

> > > Matt, do you have any thought on removing the #ifdefs
> > > from net::BidirectionalStream?
> I'm +1 for removal of #ifdefs.

Woot :>

> I hear you, and I agree that for this particular issue removal of
> 'enable_bidirectional_stream' build flag seems like a reasonable solution.

Cool, thanks!

[commit-bot: I haz the power, 2016-03-17 20:51:58]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}
==========

[commit-bot: I haz the power, 2016-03-17 20:52:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-17 20:53:01]

Description was changed from

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}
==========

to

==========
Remove support forAlt-Svc/Alternate Protocol Probability

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

Committed: https://crrev.com/740688bec5c66af00debe117b6d375ecd212e570
Cr-Commit-Position: refs/heads/master@{#381134}

Committed: https://crrev.com/dc7b9058657530c2df23e4ab7b4b3b03f54cde5f
Cr-Commit-Position: refs/heads/master@{#381791}
==========

[commit-bot: I haz the power, 2016-03-17 20:53:02]

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

[mmenke, 2016-03-18 14:57:23]

On Thu, Mar 17, 2016 at 4:25 PM, <mef@chromium.org> wrote:

> On 2016/03/17 04:09:39, mmenke wrote:
> > I suspect the binary savings aren't huge, and it's a reasonable concern.
> > On the other hand, remove the ifdefs and we may discover a bunch of code
> is
> > unexpectedly depending on the C++ APIs, which may also be fine, assuming
> > we're fairly happy with the C++ APIs, and they don't tend to lead to
> broken
> > code (Unlike, say, URLRequest and URLFetcher).
> Do you mean the danger of somebody using net::BidirectionalStream C++ API
> outside of Cronet, like in Chrome proper or something?
>

That's exactly what I mean.


> >
> > That having been said...If we aren't building Cronet at all on the bots,
> > removing the #ifdefs doesn't really address the underlying problem.
> FWIW we currently ARE building Cronet on regular Android bots, however we
> are
> NOT testing it there,
> and we don't have 'enable_bidirectional_stream' feature flag set.
>

Ahh...And not with the icu alternatives magic, either, I guess?


> Also, given that we have a separate Cronet waterfall, we could consider NOT
> building Cronet on regular Android bots,
> as this takes time and clutters gyp/gn files (specifically net_small and
> url_use_icu_alternatives_on_android targets).
>
> >
> > On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <mailto:xunjieli@chromium.org>
> wrote:
> >
> > > Nope. Cronet configurations aren't part of the CQ. We have a trybot,
> but
> > > it is in an incorrect state right now (
> > > https://codereview.chromium.org/1750743002/), so you can't really use
> > > it. I don't really like #ifdefs too (since the compile and unittests
> > > aren't being run on the CQ bots, and code search's cross reference
> doesn't
> > > work either), but #ifdefs does save some binary footprint on chrome
> proper,
> > > so I am not sure. Matt, do you have any thought on removing the #ifdefs
> > > from net::BidirectionalStream?
> I'm +1 for removal of #ifdefs.
> > >
> > > On Wed, Mar 16, 2016 at 8:43 PM, <mailto:rch@chromium.org> wrote:
> > >
> > >> mef, et. al.: I thought we had confirmed that all various cronet
> > >> configurations
> > >> were part of the CQ. This CL was reverted because of a post-CQ build
> > >> failure.
> > >>
> > >> In any case, how can I get a bot to build this with the right options
> to
> > >> ensure I did this correctly.
> > >>
> > >> Also, I *Really* think we should reconsider the decision to sprinkle
> > >> #ifdefs
> > >> throughout the stack because of this new BIDI streaming code. When
> that CL
> > >> landed, I explained that I was concerned about EXACTLY this problem.
> I hear you, and I agree that for this particular issue removal of
> 'enable_bidirectional_stream' build flag seems like a reasonable solution.
>
> > >>
> > >> https://codereview.chromium.org/1699653002/
> > >>
> > >
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> https://codereview.chromium.org/1699653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mef, 2016-03-18 15:09:03]

On 2016/03/18 14:57:23, mmenke wrote:
> On Thu, Mar 17, 2016 at 4:25 PM, <mailto:mef@chromium.org> wrote:
> 
> > On 2016/03/17 04:09:39, mmenke wrote:
> > > I suspect the binary savings aren't huge, and it's a reasonable concern.
> > > On the other hand, remove the ifdefs and we may discover a bunch of code
> > is
> > > unexpectedly depending on the C++ APIs, which may also be fine, assuming
> > > we're fairly happy with the C++ APIs, and they don't tend to lead to
> > broken
> > > code (Unlike, say, URLRequest and URLFetcher).
> > Do you mean the danger of somebody using net::BidirectionalStream C++ API
> > outside of Cronet, like in Chrome proper or something?
> >
> 
> That's exactly what I mean.
Can we use NET_EXPORT_PRIVATE for net::BidirectionalStream to avoid that?
> 
> 
> > >
> > > That having been said...If we aren't building Cronet at all on the bots,
> > > removing the #ifdefs doesn't really address the underlying problem.
> > FWIW we currently ARE building Cronet on regular Android bots, however we
> > are
> > NOT testing it there,
> > and we don't have 'enable_bidirectional_stream' feature flag set.
> >
> 
> Ahh...And not with the icu alternatives magic, either, I guess?
Regular Android builds do build 3 cronet-specific targets -
url_use_icu_alternatives_on_android, net_small and cronet, but don't execute any
tests on them.

> 
> 
> > Also, given that we have a separate Cronet waterfall, we could consider NOT
> > building Cronet on regular Android bots,
> > as this takes time and clutters gyp/gn files (specifically net_small and
> > url_use_icu_alternatives_on_android targets).
> >
> > >
> > > On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <mailto:xunjieli@chromium.org>
> > wrote:
> > >
> > > > Nope. Cronet configurations aren't part of the CQ. We have a trybot,
> > but
> > > > it is in an incorrect state right now (
> > > > https://codereview.chromium.org/1750743002/), so you can't really use
> > > > it. I don't really like #ifdefs too (since the compile and unittests
> > > > aren't being run on the CQ bots, and code search's cross reference
> > doesn't
> > > > work either), but #ifdefs does save some binary footprint on chrome
> > proper,
> > > > so I am not sure. Matt, do you have any thought on removing the #ifdefs
> > > > from net::BidirectionalStream?
> > I'm +1 for removal of #ifdefs.
> > > >
> > > > On Wed, Mar 16, 2016 at 8:43 PM, <mailto:rch@chromium.org> wrote:
> > > >
> > > >> mef, et. al.: I thought we had confirmed that all various cronet
> > > >> configurations
> > > >> were part of the CQ. This CL was reverted because of a post-CQ build
> > > >> failure.
> > > >>
> > > >> In any case, how can I get a bot to build this with the right options
> > to
> > > >> ensure I did this correctly.
> > > >>
> > > >> Also, I *Really* think we should reconsider the decision to sprinkle
> > > >> #ifdefs
> > > >> throughout the stack because of this new BIDI streaming code. When
> > that CL
> > > >> landed, I explained that I was concerned about EXACTLY this problem.
> > I hear you, and I agree that for this particular issue removal of
> > 'enable_bidirectional_stream' build flag seems like a reasonable solution.
> >
> > > >>
> > > >> https://codereview.chromium.org/1699653002/
> > > >>
> > > >
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> > https://codereview.chromium.org/1699653002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[xunjieli, 2016-03-18 15:10:13]

On Fri, Mar 18, 2016 at 11:09 AM, <mef@chromium.org> wrote:

> On 2016/03/18 14:57:23, mmenke wrote:
> > On Thu, Mar 17, 2016 at 4:25 PM, <mailto:mef@chromium.org> wrote:
> >
> > > On 2016/03/17 04:09:39, mmenke wrote:
> > > > I suspect the binary savings aren't huge, and it's a reasonable
> concern.
> > > > On the other hand, remove the ifdefs and we may discover a bunch of
> code
> > > is
> > > > unexpectedly depending on the C++ APIs, which may also be fine,
> assuming
> > > > we're fairly happy with the C++ APIs, and they don't tend to lead to
> > > broken
> > > > code (Unlike, say, URLRequest and URLFetcher).
> > > Do you mean the danger of somebody using net::BidirectionalStream C++
> API
> > > outside of Cronet, like in Chrome proper or something?
> > >
> >
> > That's exactly what I mean.
> Can we use NET_EXPORT_PRIVATE for net::BidirectionalStream to avoid that?
>
>
Cronet wrappers are in components/, so we can't use NET_EXPORT_PRIVATE,
right?


> >
> > > >
> > > > That having been said...If we aren't building Cronet at all on the
> bots,
> > > > removing the #ifdefs doesn't really address the underlying problem.
> > > FWIW we currently ARE building Cronet on regular Android bots, however
> we
> > > are
> > > NOT testing it there,
> > > and we don't have 'enable_bidirectional_stream' feature flag set.
> > >
> >
> > Ahh...And not with the icu alternatives magic, either, I guess?
> Regular Android builds do build 3 cronet-specific targets -
> url_use_icu_alternatives_on_android, net_small and cronet, but don't
> execute any
> tests on them.
>
>
> >
> >
> > > Also, given that we have a separate Cronet waterfall, we could
> consider NOT
> > > building Cronet on regular Android bots,
> > > as this takes time and clutters gyp/gn files (specifically net_small
> and
> > > url_use_icu_alternatives_on_android targets).
> > >
> > > >
> > > > On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <mailto:
> xunjieli@chromium.org>
> > > wrote:
> > > >
> > > > > Nope. Cronet configurations aren't part of the CQ. We have a
> trybot,
> > > but
> > > > > it is in an incorrect state right now (
> > > > > https://codereview.chromium.org/1750743002/), so you can't really
> use
> > > > > it. I don't really like #ifdefs too (since the compile and
> unittests
> > > > > aren't being run on the CQ bots, and code search's cross reference
> > > doesn't
> > > > > work either), but #ifdefs does save some binary footprint on chrome
> > > proper,
> > > > > so I am not sure. Matt, do you have any thought on removing the
> #ifdefs
> > > > > from net::BidirectionalStream?
> > > I'm +1 for removal of #ifdefs.
> > > > >
> > > > > On Wed, Mar 16, 2016 at 8:43 PM, <mailto:rch@chromium.org> wrote:
> > > > >
> > > > >> mef, et. al.: I thought we had confirmed that all various cronet
> > > > >> configurations
> > > > >> were part of the CQ. This CL was reverted because of a post-CQ
> build
> > > > >> failure.
> > > > >>
> > > > >> In any case, how can I get a bot to build this with the right
> options
> > > to
> > > > >> ensure I did this correctly.
> > > > >>
> > > > >> Also, I *Really* think we should reconsider the decision to
> sprinkle
> > > > >> #ifdefs
> > > > >> throughout the stack because of this new BIDI streaming code. When
> > > that CL
> > > > >> landed, I explained that I was concerned about EXACTLY this
> problem.
> > > I hear you, and I agree that for this particular issue removal of
> > > 'enable_bidirectional_stream' build flag seems like a reasonable
> solution.
> > >
> > > > >>
> > > > >> https://codereview.chromium.org/1699653002/
> > > > >>
> > > > >
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > > https://codereview.chromium.org/1699653002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/1699653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2016-03-18 15:14:24]

On Fri, Mar 18, 2016 at 11:10 AM, Helen Li <xunjieli@chromium.org> wrote:

>
>
> On Fri, Mar 18, 2016 at 11:09 AM, <mef@chromium.org> wrote:
>
>> On 2016/03/18 14:57:23, mmenke wrote:
>> > On Thu, Mar 17, 2016 at 4:25 PM, <mailto:mef@chromium.org> wrote:
>> >
>> > > On 2016/03/17 04:09:39, mmenke wrote:
>> > > > I suspect the binary savings aren't huge, and it's a reasonable
>> concern.
>> > > > On the other hand, remove the ifdefs and we may discover a bunch of
>> code
>> > > is
>> > > > unexpectedly depending on the C++ APIs, which may also be fine,
>> assuming
>> > > > we're fairly happy with the C++ APIs, and they don't tend to lead to
>> > > broken
>> > > > code (Unlike, say, URLRequest and URLFetcher).
>> > > Do you mean the danger of somebody using net::BidirectionalStream C++
>> API
>> > > outside of Cronet, like in Chrome proper or something?
>> > >
>> >
>> > That's exactly what I mean.
>> Can we use NET_EXPORT_PRIVATE for net::BidirectionalStream to avoid that?
>>
>>
> Cronet wrappers are in components/, so we can't use NET_EXPORT_PRIVATE,
> right?
>

NET_EXPORT_PRIVATE isn't actually enforced...But yea, using it on something
Cronet uses from outside of net/ doesn't seem good.


>
>
>> >
>> > > >
>> > > > That having been said...If we aren't building Cronet at all on the
>> bots,
>> > > > removing the #ifdefs doesn't really address the underlying problem.
>> > > FWIW we currently ARE building Cronet on regular Android bots,
>> however we
>> > > are
>> > > NOT testing it there,
>> > > and we don't have 'enable_bidirectional_stream' feature flag set.
>> > >
>> >
>> > Ahh...And not with the icu alternatives magic, either, I guess?
>> Regular Android builds do build 3 cronet-specific targets -
>> url_use_icu_alternatives_on_android, net_small and cronet, but don't
>> execute any
>> tests on them.
>>
>
So why can't they build with bidirectional streams?  I'm suddenly very
confused on what's going on here.


> >
>> >
>> > > Also, given that we have a separate Cronet waterfall, we could
>> consider NOT
>> > > building Cronet on regular Android bots,
>> > > as this takes time and clutters gyp/gn files (specifically net_small
>> and
>> > > url_use_icu_alternatives_on_android targets).
>> > >
>> > > >
>> > > > On Wed, Mar 16, 2016 at 8:52 PM, Helen Li <mailto:
>> xunjieli@chromium.org>
>> > > wrote:
>> > > >
>> > > > > Nope. Cronet configurations aren't part of the CQ. We have a
>> trybot,
>> > > but
>> > > > > it is in an incorrect state right now (
>> > > > > https://codereview.chromium.org/1750743002/), so you can't
>> really use
>> > > > > it. I don't really like #ifdefs too (since the compile and
>> unittests
>> > > > > aren't being run on the CQ bots, and code search's cross reference
>> > > doesn't
>> > > > > work either), but #ifdefs does save some binary footprint on
>> chrome
>> > > proper,
>> > > > > so I am not sure. Matt, do you have any thought on removing the
>> #ifdefs
>> > > > > from net::BidirectionalStream?
>> > > I'm +1 for removal of #ifdefs.
>> > > > >
>> > > > > On Wed, Mar 16, 2016 at 8:43 PM, <mailto:rch@chromium.org> wrote:
>> > > > >
>> > > > >> mef, et. al.: I thought we had confirmed that all various cronet
>> > > > >> configurations
>> > > > >> were part of the CQ. This CL was reverted because of a post-CQ
>> build
>> > > > >> failure.
>> > > > >>
>> > > > >> In any case, how can I get a bot to build this with the right
>> options
>> > > to
>> > > > >> ensure I did this correctly.
>> > > > >>
>> > > > >> Also, I *Really* think we should reconsider the decision to
>> sprinkle
>> > > > >> #ifdefs
>> > > > >> throughout the stack because of this new BIDI streaming code.
>> When
>> > > that CL
>> > > > >> landed, I explained that I was concerned about EXACTLY this
>> problem.
>> > > I hear you, and I agree that for this particular issue removal of
>> > > 'enable_bidirectional_stream' build flag seems like a reasonable
>> solution.
>> > >
>> > > > >>
>> > > > >> https://codereview.chromium.org/1699653002/
>> > > > >>
>> > > > >
>> > > > >
>> > > >
>> > > > --
>> > > > You received this message because you are subscribed to the Google
>> Groups
>> > > > "Chromium-reviews" group.
>> > > > To unsubscribe from this group and stop receiving emails from it,
>> send an
>> > > email
>> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > >
>> > >
>> > > https://codereview.chromium.org/1699653002/
>> > >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>> Groups
>> > "Chromium-reviews" group.
>> > To unsubscribe from this group and stop receiving emails from it, send
>> an
>> email
>> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>>
>>
>> https://codereview.chromium.org/1699653002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.