Persist broken and recently-broken alt-svcs to prefs in HttpServerPropertiesManager

Persist broken and recently-broken alt-svcs to prefs in HttpServerPropertiesManager

Modify TickClock dependency injection for BrokenAlternativeServices to use a setter instead of a constructor param.
Add TickClock dependency injection for HttpServerPropertiesImpl and HttpServerPropertiesManager for testing. 

Add BrokenAlternativeService::Clear() and update HttpServerPropertiesImpl::Clear() to call that.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng




BUG=705029

[wangyix1, 2017-06-10 00:14:29]

Description was changed from

==========
git cl format


Finished HttpServerPropertiesManagerTest.UpdateCacheWithPrefs


Fixed compile error from rebasing


Working on HttpServerPropertiesManagerTest.UpdateCacheWithPrefs


Added TickClock* to HttpServerPropertiesManager and use it in expiration
conversion from TimeTicks to Time


Fixed bug in AlternativeService::FromString()


Updated BrokenAlternativeServices to have alt-svc expiration/broken-count from
prefs overwrite those in the cache when loaded. Changed
ScheduleBrokenAlternateProcolMappingsExpiration() to use a when param.


Moved net_test_task_runner_clock_ to HttpServerPropertiesManagerTest class.
Updated UpdatePrefsWithCache test to not verify broken alt svc expiration time
due to use of Time::Now().


Updated HttpServerPropertiesManagerTest.Clear to test clearing of broken
alternative service.


Added BrokenAlternativeServices::Clear()


Updated existing http_server_properties_manager unittests now that broken alt
svcs are persisted


Fixed MRUCache iterator confusion: recently_broken_alternative_services_ will be
written to disk from oldest to newest order. Also added setter for the
BrokenAlternativeServices TickClock for tests.


Removed TickClock* params and instead added a setter-for-test for
BrokenAlternativeServices and HttpServerPropertiesImpl


broken alt svcs in HttpServerPropertiesImpl::alternative_services_map_ are now
persisted.


Fixed UpdateCacheFromPrefsOnPrefThread() code for loading broken alt svcs.
broken alt-svcs that don't show up in recently_broken_alternative_services are
not loaded. Added call to AddBrokenAndRecentlyBrokenAlternativeServices() in
UpdateCahceFromPrefsOnNetworkThread().


Added code in UpdateCacheFromPrefsOnPrefThread() to remove broken alt svcs that
don't apear in recently broken map.


Fixed compile errors; git cl format

ONLY http_server_properties CHANGES!

Copied over work to persist broken_alternative_service_list_ and
recently_broken_alternative_services_. Still need to update
UpdateCacheFromPrefsOnNetworkThread()

ONLY http_server_properties CHANGES!

BUG=
==========

to

==========
Persist broken and recently-broken alt-svcs to prefs in
HttpServerPropertiesManager

BUG=705029
==========

[wangyix1, 2017-06-10 00:28:32]

Description was changed from

==========
Persist broken and recently-broken alt-svcs to prefs in
HttpServerPropertiesManager

BUG=705029
==========

to

==========
Persist broken and recently-broken alt-svcs to prefs in
HttpServerPropertiesManager

Modify TickClock dependency injection for BrokenAlternativeServices to use a
setter instead of a constructor param.
Add TickClock dependency injection for HttpServerPropertiesImpl and
HttpServerPropertiesManager for testing. 

Add BrokenAlternativeService::Clear() and update
HttpServerPropertiesImpl::Clear() to call that.



BUG=705029
==========

[wangyix1, 2017-06-10 01:13:34]

wangyix@chromium.org changed reviewers:
+ rch@chromium.org, zhongyi@chromium.org

[wangyix1, 2017-06-10 01:13:34]

Still updating the CL that this one depends on
(https://codereview.chromium.org/2917913002/), but feel free to take a look at
this CL; it might help with seeing how the other one fits in.

[wangyix1, 2017-06-10 01:13:52]

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

[commit-bot: I haz the power, 2017-06-10 01:14:05]

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-06-10 01:16:28]

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

[commit-bot: I haz the power, 2017-06-10 01:16:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Hamilton, 2017-06-12 18:57:33]

Looks promising! I'll wait to review thoroughly until you've landed the CL that
this depends on, but here are a few comments.

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
File net/http/broken_alternative_services.cc (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:125: clock_ = clock;
I don't think I understand the clock-related changes in this file. What's the
motivation? I assume they were necessitated by something that I may have missed.

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
File net/http/broken_alternative_services.h (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.h:48: void Clear();
nit: Comment (though it's probably fairly obvious)

Since this is so self-contained, consider breaking this out into a simple
stand-alone CL.

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.h:104: // |clock| is unowned.
I think you mean to say that |clock| must outlive this?

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.h:136: base::TickClock* clock_;  // Unowned
if not pointing to |default_clock_|.
I'd just say "Unowned". Because you'll never say "delete clock_" right?

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties.cc:108: }
Tests?

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties.h:107: AlternativeService* alternative_service);
Is this being added in order to support the serialization in
HttpServerPropertiesManager?

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl.h:137: void
SetBrokenAlternativeServicesTickClockForTesting(base::TickClock* clock);
The constructor-based method for setting the clock seems more clear to me. Did
that cause problems that the TestOnly setter solves?

[wangyix1, 2017-06-13 21:56:35]

Description was changed from

==========
Persist broken and recently-broken alt-svcs to prefs in
HttpServerPropertiesManager

Modify TickClock dependency injection for BrokenAlternativeServices to use a
setter instead of a constructor param.
Add TickClock dependency injection for HttpServerPropertiesImpl and
HttpServerPropertiesManager for testing. 

Add BrokenAlternativeService::Clear() and update
HttpServerPropertiesImpl::Clear() to call that.



BUG=705029
==========

to

==========
Persist broken and recently-broken alt-svcs to prefs in
HttpServerPropertiesManager

Modify TickClock dependency injection for BrokenAlternativeServices to use a
setter instead of a constructor param.
Add TickClock dependency injection for HttpServerPropertiesImpl and
HttpServerPropertiesManager for testing. 

Add BrokenAlternativeService::Clear() and update
HttpServerPropertiesImpl::Clear() to call that.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_ubsan_rel_ng




BUG=705029
==========

[wangyix1, 2017-06-13 21:56:48]

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

[commit-bot: I haz the power, 2017-06-13 21:57:26]

Dry run: CQ is trying da patch.

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

[wangyix1, 2017-06-14 00:01:26]

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
File net/http/broken_alternative_services.cc (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:125: clock_ = clock;
On 2017/06/12 18:57:32, Ryan Hamilton wrote:
> I don't think I understand the clock-related changes in this file. What's the
> motivation? I assume they were necessitated by something that I may have
missed.

Acknowledged. Will go back to clock being set using a TickClock* param in the
constructor.

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
File net/http/broken_alternative_services.h (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.h:48: void Clear();
On 2017/06/12 18:57:32, Ryan Hamilton wrote:
> nit: Comment (though it's probably fairly obvious)
> 
> Since this is so self-contained, consider breaking this out into a simple
> stand-alone CL.

Done.

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.h:104: // |clock| is unowned.
On 2017/06/12 18:57:32, Ryan Hamilton wrote:
> I think you mean to say that |clock| must outlive this?

Function has been removed

https://codereview.chromium.org/2932953002/diff/20001/net/http/broken_alterna...
net/http/broken_alternative_services.h:136: base::TickClock* clock_;  // Unowned
if not pointing to |default_clock_|.
On 2017/06/12 18:57:32, Ryan Hamilton wrote:
> I'd just say "Unowned". Because you'll never say "delete clock_" right?

Done.

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties.cc:108: }
On 2017/06/12 18:57:32, Ryan Hamilton wrote:
> Tests?

Where should the test for this go? There doesn't seem to be any existing tests
that test the AlternativeService class by itself.

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties.h:107: AlternativeService* alternative_service);
On 2017/06/12 18:57:32, Ryan Hamilton wrote:
> Is this being added in order to support the serialization in
> HttpServerPropertiesManager?

Yes.

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/2932953002/diff/20001/net/http/http_server_pr...
net/http/http_server_properties_impl.h:137: void
SetBrokenAlternativeServicesTickClockForTesting(base::TickClock* clock);
On 2017/06/12 18:57:33, Ryan Hamilton wrote:
> The constructor-based method for setting the clock seems more clear to me. Did
> that cause problems that the TestOnly setter solves?

Acknowledged. Will go back to using a TickClock* constructor param.

[Zhongyi Shi, 2017-06-16 04:17:10]

This is my initial round of review. I haven't get to the unittests. 

In general, I think you are doing the right thing. Some nits on the APIs in
HttpServerProperties and Manager.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties.h:105: 
Is this method meant to parse the alternative service from a string to
AlternativeService? Could you add some comment on the requirements on input?

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.h:42:
HttpServerPropertiesImpl(base::TickClock* clock);
nit: explicit

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.h:72:
recently_broken_alternative_services);
Does the raw pointers work? Usually pass by unique_ptr means ownership transfer.
I didn't quite get why the ownership of the broken_alternative_service_list need
to be transferred.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:53: const int
kMaxBrokenAlternativeServicesToPersist = 200;
We used to decide the number of Alt Svc to persist based on data collected from
histograms. I would suggest adding a histogram as well to log how many
broken/recently broken AlternativeServices and are persisted in the real world
so as to tune the size.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:612: base::TimeTicks*
expiration_time_ticks) {
Can we just change this method to take DictionaryValue as input, and adds a
entry to broken_alternative_service_list if valid?

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.h:230:
recently_broken_alternative_services,
Can we just use raw pointers here? Is there any strong reason leading you to use
the unique_ptrs?

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.h:293: bool
ExtractBrokenAlternativeServiceData(
nit:
%s/ExtractBrokenAlternativeServiceData/AddToBrokenAlternativeServiceDataList

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.h:297: bool
AddRecentlyBrokenAlternativeServiceData(
nit:
%s/AddRecentlyBrokenAlternativeServiceData/AddToRecentlyBrokenAlternativeServices

[wangyix1, 2017-06-16 18:41:16]

PTAL

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties.h:105: 
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> Is this method meant to parse the alternative service from a string to
> AlternativeService? Could you add some comment on the requirements on input?

Yes, that's what it does.
Done.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.h:42:
HttpServerPropertiesImpl(base::TickClock* clock);
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_impl.h:72:
recently_broken_alternative_services);
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> Does the raw pointers work? Usually pass by unique_ptr means ownership
transfer.
> I didn't quite get why the ownership of the broken_alternative_service_list
need
> to be transferred. 

unique_ptrs are used since these params are simply passed through to
BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices().
I chose to use unique_ptr params for
BrokenAlternativeServices::SetBrokenAndRecentlyBrokenAlternativeServices()
because its implementation modifies |broken_alternative_service_list| and
|recently_broken_alternative_services|. I suppose I could use raw pointers, but
I think using unique_ptrs makes it clear that those objects should not be used
after calling this function. Thoughts?

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_manager.cc (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:53: const int
kMaxBrokenAlternativeServicesToPersist = 200;
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> We used to decide the number of Alt Svc to persist based on data collected
from
> histograms. I would suggest adding a histogram as well to log how many
> broken/recently broken AlternativeServices and are persisted in the real world
> so as to tune the size.

Done.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.cc:612: base::TimeTicks*
expiration_time_ticks) {
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> Can we just change this method to take DictionaryValue as input, and adds a
> entry to broken_alternative_service_list if valid? 

The reason for this can be seen in how this function is used in
UpdateCacheFromPrefsOnPrefSequence(). I only want to add an entry to
|broken_alternative_service_list| if the broken alt-svc has an entry in
|recently_broken_alternative_services|. The logic to check for this is currently
not the responsibility of this function. Should I move that logic into this
function? The params would then be:

const DictionaryValue* broken_alternative_service_dict,

const RecentlyBrokenAlternativeServices& recently_broken_alternative_services,

BrokenAlternativeServiceList* broken_alternative_service_list

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.h:230:
recently_broken_alternative_services,
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> Can we just use raw pointers here? Is there any strong reason leading you to
use
> the unique_ptrs?

Acknowledged.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.h:293: bool
ExtractBrokenAlternativeServiceData(
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> nit:
> %s/ExtractBrokenAlternativeServiceData/AddToBrokenAlternativeServiceDataList

Acknowledged.

https://codereview.chromium.org/2932953002/diff/100001/net/http/http_server_p...
net/http/http_server_properties_manager.h:297: bool
AddRecentlyBrokenAlternativeServiceData(
On 2017/06/16 04:17:09, Zhongyi Shi wrote:
> nit:
>
%s/AddRecentlyBrokenAlternativeServiceData/AddToRecentlyBrokenAlternativeServices

Done.

[wangyix1, 2017-06-21 18:38:09]

Ping

[wangyix1, 2017-06-21 18:49:59]

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

[commit-bot: I haz the power, 2017-06-21 18:50:16]

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-06-21 20:56:16]

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

[commit-bot: I haz the power, 2017-06-21 20:56:17]

Dry run: This issue passed the CQ dry run.

[wangyix1, 2017-06-23 18:58:47]

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

[commit-bot: I haz the power, 2017-06-23 18:59:11]

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-06-23 20:08:07]

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

[commit-bot: I haz the power, 2017-06-23 20:08:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Zhongyi Shi, 2017-06-28 21:17:35]

Discussed offline, will take another look when this CL is ready to review.

[wangyix1, 2017-07-05 23:31:18]

PTAL

[Ryan Hamilton, 2017-07-06 16:48:22]

Good progress! I'll defer to zhongyi for a full review.

https://codereview.chromium.org/2932953002/diff/260001/net/http/broken_altern...
File net/http/broken_alternative_services.h (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/broken_altern...
net/http/broken_alternative_services.h:12: #include
"base/time/default_tick_clock.h"
It looks like a fwd-declare of base::TickClock should work, since i don't think
default tick clock is actually used?

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:38: // lifetime would not have begun
yet.
Alternatively, you could pass in nullptr and the other constructor could notice
that the argument is null so use default_clock_ instead.

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
net/http/http_server_properties_manager.h:322: base::DefaultTickClock
default_clock_;
Do we really need a default clock in both the impl and the manager?

[wangyix1, 2017-07-06 18:04:29]

https://codereview.chromium.org/2932953002/diff/260001/net/http/broken_altern...
File net/http/broken_alternative_services.h (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/broken_altern...
net/http/broken_alternative_services.h:12: #include
"base/time/default_tick_clock.h"
On 2017/07/06 16:48:22, Ryan Hamilton wrote:
> It looks like a fwd-declare of base::TickClock should work, since i don't
think
> default tick clock is actually used?

Done.

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
net/http/http_server_properties_impl.cc:38: // lifetime would not have begun
yet.
On 2017/07/06 16:48:22, Ryan Hamilton wrote:
> Alternatively, you could pass in nullptr and the other constructor could
notice
> that the argument is null so use default_clock_ instead.

Done.

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
net/http/http_server_properties_manager.h:322: base::DefaultTickClock
default_clock_;
On 2017/07/06 16:48:22, Ryan Hamilton wrote:
> Do we really need a default clock in both the impl and the manager?

I put a default clock in impl since it's instantiated in many places other than
the manager and didn't want to explicitly add a DefaultTickClock instance in
every place it's used.

The manager needs a TickClock for converting broken alt-svc expiration times
from TimeTicks to Time. Instead of having its own TickClock* and DefaultClock, I
could add a function Impl::GetTickClock() and have the manager use that instead?

[Ryan Hamilton, 2017-07-06 18:36:20]

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
net/http/http_server_properties_manager.h:322: base::DefaultTickClock
default_clock_;
On 2017/07/06 18:04:29, wangyix1 wrote:
> On 2017/07/06 16:48:22, Ryan Hamilton wrote:
> > Do we really need a default clock in both the impl and the manager?
> 
> I put a default clock in impl since it's instantiated in many places other
than
> the manager and didn't want to explicitly add a DefaultTickClock instance in
> every place it's used.

Ah, I see. Yuck :(
 
> The manager needs a TickClock for converting broken alt-svc expiration times
> from TimeTicks to Time. Instead of having its own TickClock* and DefaultClock,
I
> could add a function Impl::GetTickClock() and have the manager use that
instead?

Oh, that's clever. Sure, that sounds like a good idea.

[Zhongyi Shi, 2017-07-06 23:13:53]

Could you rebase the CL before I proceed to the full review? There are some
changes landed recently in HttpServerProperties, especially
AlternativeServiceInfo which might affect this CL. Let me know when it's ready
for review. Sorry about the inconvenience.

https://codereview.chromium.org/2932953002/diff/280001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/280001/net/http/http_server_p...
net/http/http_server_properties_manager.h:99: // expiration of broken
alternative services. It must not be null.
nit: If null, default clock will be used.

[wangyix1, 2017-07-07 00:13:54]

On 2017/07/06 18:36:20, Ryan Hamilton wrote:
>
https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
> File net/http/http_server_properties_manager.h (right):
> 
>
https://codereview.chromium.org/2932953002/diff/260001/net/http/http_server_p...
> net/http/http_server_properties_manager.h:322: base::DefaultTickClock
> default_clock_;
> On 2017/07/06 18:04:29, wangyix1 wrote:
> > On 2017/07/06 16:48:22, Ryan Hamilton wrote:
> > > Do we really need a default clock in both the impl and the manager?
> > 
> > I put a default clock in impl since it's instantiated in many places other
> than
> > the manager and didn't want to explicitly add a DefaultTickClock instance in
> > every place it's used.
> 
> Ah, I see. Yuck :(
>  
> > The manager needs a TickClock for converting broken alt-svc expiration times
> > from TimeTicks to Time. Instead of having its own TickClock* and
DefaultClock,
> I
> > could add a function Impl::GetTickClock() and have the manager use that
> instead?
> 
> Oh, that's clever. Sure, that sounds like a good idea.

Okay I rediscovered the reason for why I didn't do this in the first place:
HttpServerPropertiesImpl lives on the net thread, but the manager needs to use
the clock on the prefs thread. It would be weird for the manger to call
impl_->GetTickClock() on the prefs thread when all other impl functions are only
called from the net thread. What do you think?

[wangyix1, 2017-07-07 00:17:42]

"git cl upload" is no longer letting me patch this codereview. This has been
re-uploaded to Gerrit here:

https://chromium-review.googlesource.com/c/562604/

zhongyi@, your comment nit has been fixed and is part of the initial version of
the code uploaded to Gerrit.

https://codereview.chromium.org/2932953002/diff/280001/net/http/http_server_p...
File net/http/http_server_properties_manager.h (right):

https://codereview.chromium.org/2932953002/diff/280001/net/http/http_server_p...
net/http/http_server_properties_manager.h:99: // expiration of broken
alternative services. It must not be null.
On 2017/07/06 23:13:53, Zhongyi Shi wrote:
> nit: If null, default clock will be used. 

Done.

[Ryan Hamilton, 2017-07-11 18:17:17]

rch@chromium.org changed reviewers:
- rch@chromium.org