Add getters and setters for BrokenAlternativeService's list of broken and recently-broken alt svcs

Add getters and setters for BrokenAlternativeService's list of broken and recently-broken alt svcs

This CL is in preparation for modifying HttpServerPropertiesManager to persist broken and recently-broken alt svcs to disk.

BUG=705029
Review-Url: https://codereview.chromium.org/2917913002
Cr-Commit-Position: refs/heads/master@{#480596}
Committed: https://chromium.googlesource.com/chromium/src/+/c10f1e6870cb3253860dffe55ddad9a0ab646801

[wangyix1, 2017-06-01 17:30:06]

Description was changed from

==========
Added comment to AddBrokenAndRecentlyBrokenAlternativeServices()


Changed AddBrokenAndRecentlyBrokenAlternativeServices() to take ptrs instead of
references


Added BrokenAlternativeService unittests AddBrokenAlternativeServices and
"""ToExisting Fixed bug in BrokenAlternativeService's expiration task scheduling
when adding broken alt svcs.


Fixed error: expiration task needs to be scheduled if expiration queue was empty
and becomes non-empty.


Fixed compile errors; git cl format

ONLY broken_alternative_services CHANGES!

Added BrokenAlternativeService::AddBrokenAndRecentlyBrokenAlternativeServices().
Not tested.


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

ONLY broken_alternative_services CHANGES!

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

to

==========
Add getters and setters for BrokenAlternativeService's list of broken and
recently-broken alt svcs

This CL is in preparation for modifying HttpServerPropertiesManager to persist
broken and recently-broken alt svcs to disk.

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

[wangyix1, 2017-06-01 17:30:06]

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

[wangyix1, 2017-06-01 17:30:24]

PTAL

[Ryan Hamilton, 2017-06-01 18:03:43]

rch@chromium.org changed reviewers:
+ rch@chromium.org
- rch@google.com

[Ryan Hamilton, 2017-06-01 18:04:28]

I'll let cherie do the first pass at the review and will take a look once she's
happy.

[Zhongyi Shi, 2017-06-02 18:19:44]

Thanks for the patience while waiting for feedback. I will take another look on
the unittests when those comments get resolved.

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.cc:137: }
|recently_broken_alternative_services| is an MRUCache. Does the access to this
cache for DCHECK modify the data?

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.cc:142: // front of recency list.
I might lose some context but why those existing recently broken alt svc should
be in front of newly added recently broken alt svcs?

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:33: BrokenAlternativeServiceMap;
Hrm, if I remembered this correctly, the BrokenAlternativeServiceList is also
defined in the impl.h. Is there any difference? If not, how about move this to
http_server_properties.h?

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:101: void
AddBrokenAndRecentlyBrokenAlternativeServices(
The return type of this method should be a boolean to indicate whether an update
should be scheduled to persist the data in disk. Do you plan to do that in the
follow-up CL?

[wangyix1, 2017-06-05 19:16:25]

Fixed comments from ps2, PTAL.

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.cc:137: }
On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> |recently_broken_alternative_services| is an MRUCache. Does the access to this
> cache for DCHECK modify the data?

No, Peek() does not modify the recency list, unlike Get().

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.cc:142: // front of recency list.
On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> I might lose some context but why those existing recently broken alt svc
should
> be in front of newly added recently broken alt svcs?

It's not clear from the name, but this method will be used for loading broken
and recently-broken alt svcs from disk and adding it to the cache. The ones
loaded from disk should be less recent than the ones already in the cache. I
wonder if this should be made more clear?

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:33: BrokenAlternativeServiceMap;
On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> Hrm, if I remembered this correctly, the BrokenAlternativeServiceList is also
> defined in the impl.h. Is there any difference? If not, how about move this to
> http_server_properties.h?

Done.

I don't see it defined in http_server_properties_impl.h, but I agree it makes
sense to move the BrokenAlternativeServiceList typedef to
http_server_properties.h

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:101: void
AddBrokenAndRecentlyBrokenAlternativeServices(
On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> The return type of this method should be a boolean to indicate whether an
update
> should be scheduled to persist the data in disk. Do you plan to do that in the
> follow-up CL?

Hmm I didn't think of that. I'm assuming it should return true if any
alternative services were added to |broken_...| or |recently_broken_...|?

[Zhongyi Shi, 2017-06-06 22:17:49]

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.cc:137: }
On 2017/06/05 19:16:25, wangyix1 wrote:
> On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > |recently_broken_alternative_services| is an MRUCache. Does the access to
this
> > cache for DCHECK modify the data?
> 
> No, Peek() does not modify the recency list, unlike Get().

I miss-read the code, yeah, Peek() should be fine.

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:33: BrokenAlternativeServiceMap;
On 2017/06/05 19:16:25, wangyix1 wrote:
> On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > Hrm, if I remembered this correctly, the BrokenAlternativeServiceList is
also
> > defined in the impl.h. Is there any difference? If not, how about move this
to
> > http_server_properties.h?
> 
> Done.
> 
> I don't see it defined in http_server_properties_impl.h, but I agree it makes
> sense to move the BrokenAlternativeServiceList typedef to
> http_server_properties.h

Ah.. I was referring to your previous CL which defines the
AlternativeServiceHash, BrokenAlternativeServiceMap, and
BrokenAlternativeServiceList in net/http/broken_alternative_services.h.

Instead of define in multiple files, how about move them to the
http_server_properties.h?

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:101: void
AddBrokenAndRecentlyBrokenAlternativeServices(
On 2017/06/05 19:16:25, wangyix1 wrote:
> On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > The return type of this method should be a boolean to indicate whether an
> update
> > should be scheduled to persist the data in disk. Do you plan to do that in
the
> > follow-up CL?
> 
> Hmm I didn't think of that. I'm assuming it should return true if any
> alternative services were added to |broken_...| or |recently_broken_...|?

If you go a step further, the return type will be used to determine whether
HttpServerPropertiesManager will need to schedule an update to disk. And that's
why the return type should be a boolean. But you haven't get to the persisting
logic, so it's fine to leave it to that CL.

[wangyix1, 2017-06-09 19:26:41]

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.cc:137: }
On 2017/06/06 22:17:49, Zhongyi Shi wrote:
> On 2017/06/05 19:16:25, wangyix1 wrote:
> > On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > > |recently_broken_alternative_services| is an MRUCache. Does the access to
> this
> > > cache for DCHECK modify the data?
> > 
> > No, Peek() does not modify the recency list, unlike Get().
> 
> I miss-read the code, yeah, Peek() should be fine. 

Acknowledged.

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:33: BrokenAlternativeServiceMap;
On 2017/06/06 22:17:49, Zhongyi Shi wrote:
> On 2017/06/05 19:16:25, wangyix1 wrote:
> > On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > > Hrm, if I remembered this correctly, the BrokenAlternativeServiceList is
> also
> > > defined in the impl.h. Is there any difference? If not, how about move
this
> to
> > > http_server_properties.h?
> > 
> > Done.
> > 
> > I don't see it defined in http_server_properties_impl.h, but I agree it
makes
> > sense to move the BrokenAlternativeServiceList typedef to
> > http_server_properties.h
> 
> Ah.. I was referring to your previous CL which defines the
> AlternativeServiceHash, BrokenAlternativeServiceMap, and
> BrokenAlternativeServiceList in net/http/broken_alternative_services.h.
> 
> Instead of define in multiple files, how about move them to the
> http_server_properties.h?  

Hmm, I figured I would keep AlternativeServiceHash and
BrokenAlternativeServiceMap as private typedefs in BrokenAlternativeServices
since this map type is very specific to the implementation of
BrokenAlternativeServices: it maps alt-svcs to iterators in
broken_alternative_service_list_. I feel that exposing the
BrokenAlternativeServiceMap is unnecessary since it's only used for this very
specific thing. By extension, since the AlternativeServiceHash is only needed
for the BrokenAlternativeServiceMap, I kept that private too. What do you think?

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:101: void
AddBrokenAndRecentlyBrokenAlternativeServices(
On 2017/06/06 22:17:49, Zhongyi Shi wrote:
> On 2017/06/05 19:16:25, wangyix1 wrote:
> > On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > > The return type of this method should be a boolean to indicate whether an
> > update
> > > should be scheduled to persist the data in disk. Do you plan to do that in
> the
> > > follow-up CL?
> > 
> > Hmm I didn't think of that. I'm assuming it should return true if any
> > alternative services were added to |broken_...| or |recently_broken_...|?
> 
> If you go a step further, the return type will be used to determine whether
> HttpServerPropertiesManager will need to schedule an update to disk. And
that's
> why the return type should be a boolean. But you haven't get to the persisting
> logic, so it's fine to leave it to that CL. 

Acknowledged.

[wangyix1, 2017-06-10 01:07:17]

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:101: void
AddBrokenAndRecentlyBrokenAlternativeServices(
On 2017/06/06 22:17:49, Zhongyi Shi wrote:
> On 2017/06/05 19:16:25, wangyix1 wrote:
> > On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > > The return type of this method should be a boolean to indicate whether an
> > update
> > > should be scheduled to persist the data in disk. Do you plan to do that in
> the
> > > follow-up CL?
> > 
> > Hmm I didn't think of that. I'm assuming it should return true if any
> > alternative services were added to |broken_...| or |recently_broken_...|?
> 
> If you go a step further, the return type will be used to determine whether
> HttpServerPropertiesManager will need to schedule an update to disk. And
that's
> why the return type should be a boolean. But you haven't get to the persisting
> logic, so it's fine to leave it to that CL. 

The current code will only schedule an update to disk after reading from disk if
corrupted data was detected while reading from disk. All the setters called in
HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence() do not
return a boolean. I'm not sure why my setter here needs to return a boolean?
This setter here (like those existing setters) will only be used to update the
cache using stuff read from the disk, so I don't see why this setter would cause
HttpServerPropertiesManager to write everything back to disk. The logic for
detecting corrupted prefs is done elsewhere (in
UpdateCacheFromPrefsOnPrefSequence()) and is the only thing that might cause a
disk update to be scheduled after reading from disk.

[wangyix1, 2017-06-12 22:13:23]

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

[commit-bot: I haz the power, 2017-06-12 22:13:50]

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-12 22:28:45]

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

https://codereview.chromium.org/2917913002/diff/10004/net/http/broken_alterna...
net/http/broken_alternative_services.h:101: void
AddBrokenAndRecentlyBrokenAlternativeServices(
On 2017/06/10 01:07:17, wangyix1 wrote:
> On 2017/06/06 22:17:49, Zhongyi Shi wrote:
> > On 2017/06/05 19:16:25, wangyix1 wrote:
> > > On 2017/06/02 18:19:44, Zhongyi Shi wrote:
> > > > The return type of this method should be a boolean to indicate whether
an
> > > update
> > > > should be scheduled to persist the data in disk. Do you plan to do that
in
> > the
> > > > follow-up CL?
> > > 
> > > Hmm I didn't think of that. I'm assuming it should return true if any
> > > alternative services were added to |broken_...| or |recently_broken_...|?
> > 
> > If you go a step further, the return type will be used to determine whether
> > HttpServerPropertiesManager will need to schedule an update to disk. And
> that's
> > why the return type should be a boolean. But you haven't get to the
persisting
> > logic, so it's fine to leave it to that CL. 
> 
> The current code will only schedule an update to disk after reading from disk
if
> corrupted data was detected while reading from disk. All the setters called in
> HttpServerPropertiesManager::UpdateCacheFromPrefsOnNetworkSequence() do not
> return a boolean. I'm not sure why my setter here needs to return a boolean?
> This setter here (like those existing setters) will only be used to update the
> cache using stuff read from the disk, so I don't see why this setter would
cause
> HttpServerPropertiesManager to write everything back to disk. The logic for
> detecting corrupted prefs is done elsewhere (in
> UpdateCacheFromPrefsOnPrefSequence()) and is the only thing that might cause a
> disk update to be scheduled after reading from disk.

Discussed in person. Will rename this function to
SetBrokenAndRecentlyBrokenAlternativeServices() so it's consistent with the
other setters called in UpdateCacheFromPrefsOnNetworkSequence().

[wangyix1, 2017-06-12 22:52:20]

PTAL

[Zhongyi Shi, 2017-06-12 23:20:38]

Almost there, looks great! Just one question.

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

https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:166: // its position in
|broken_alternative_service_list|.
This method has no guarantee on when it will be called. (Although I agree with
you that this possibly will only called when we load data from disk to memory,
i.e., a restart). 

If an entry exists in both memory, i.e., broken_alternative_service_list_, and
the list it was passed in, broken_alternativbe_service_list, and the expiration
is different, which one would you use? 

Consider the callsite of this method, I guess I would use the data in memory or
do an explicit comparison to make the call?

[commit-bot: I haz the power, 2017-06-12 23:49:49]

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

[commit-bot: I haz the power, 2017-06-12 23:49:52]

Dry run: This issue passed the CQ dry run.

[wangyix1, 2017-06-13 01:17:46]

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

https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:166: // its position in
|broken_alternative_service_list|.
On 2017/06/12 23:20:38, Zhongyi Shi wrote:
> This method has no guarantee on when it will be called. (Although I agree with
> you that this possibly will only called when we load data from disk to memory,
> i.e., a restart). 
> 
> If an entry exists in both memory, i.e., broken_alternative_service_list_, and
> the list it was passed in, broken_alternativbe_service_list, and the
expiration
> is different, which one would you use? 
> 
> Consider the callsite of this method, I guess I would use the data in memory
or
> do an explicit comparison to make the call? 

If some data exists in both cache and disk, I currently have it use the data
from disk. My reasoning is this: before the disk load occurs, any data gathered
in the cache would be based on a blank slate: for example, an alt-svc could be
marked broken even though it never should've been tried in the first place
(since we have no broken alt-svc info loaded yet). Because of this, I simply
have the disk data overwrite the cache data for entries that exist in both. This
is true for both expiration time and broken count.
Is this a reasonable assumption? I could see maybe for expiration time, if the
existing expiration time in cache is longer than the one being loaded from disk,
an explicit comparison (i.e. choosing the larger expiration time) might be
better?

[Zhongyi Shi, 2017-06-13 21:45:22]

lgtm! 

Please wait for rch@'s approval on this CL to land.

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

https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:166: // its position in
|broken_alternative_service_list|.
On 2017/06/13 01:17:46, wangyix1 wrote:
> On 2017/06/12 23:20:38, Zhongyi Shi wrote:
> > This method has no guarantee on when it will be called. (Although I agree
with
> > you that this possibly will only called when we load data from disk to
memory,
> > i.e., a restart). 
> > 
> > If an entry exists in both memory, i.e., broken_alternative_service_list_,
and
> > the list it was passed in, broken_alternativbe_service_list, and the
> expiration
> > is different, which one would you use? 
> > 
> > Consider the callsite of this method, I guess I would use the data in memory
> or
> > do an explicit comparison to make the call? 
> 
> If some data exists in both cache and disk, I currently have it use the data
> from disk. My reasoning is this: before the disk load occurs, any data
gathered
> in the cache would be based on a blank slate: for example, an alt-svc could be
> marked broken even though it never should've been tried in the first place
> (since we have no broken alt-svc info loaded yet). Because of this, I simply
> have the disk data overwrite the cache data for entries that exist in both.
This
> is true for both expiration time and broken count.
> Is this a reasonable assumption? I could see maybe for expiration time, if the
> existing expiration time in cache is longer than the one being loaded from
disk,
> an explicit comparison (i.e. choosing the larger expiration time) might be
> better?

Urm, I don't think we could assume before disk load happens, the cache is based
on a blank slate. Think about the case that a previous disk load happens, memory
state is updated based on the disk load. Of course, updates are also written
back to the disk with some delay. Next time when you restart, disk and memory
could be in inconsistent states, and now which one would you trust? Of course,
all those are heuristics to improve user experience, I think both should be
fine.

+rch@: Ryan, WDYT?

[Ryan Hamilton, 2017-06-19 17:05:35]

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

https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:166: // its position in
|broken_alternative_service_list|.
On 2017/06/13 21:45:22, Zhongyi Shi wrote:
> On 2017/06/13 01:17:46, wangyix1 wrote:
> > On 2017/06/12 23:20:38, Zhongyi Shi wrote:
> > > This method has no guarantee on when it will be called. (Although I agree
> with
> > > you that this possibly will only called when we load data from disk to
> memory,
> > > i.e., a restart). 
> > > 
> > > If an entry exists in both memory, i.e., broken_alternative_service_list_,
> and
> > > the list it was passed in, broken_alternativbe_service_list, and the
> > expiration
> > > is different, which one would you use? 
> > > 
> > > Consider the callsite of this method, I guess I would use the data in
memory
> > or
> > > do an explicit comparison to make the call? 
> > 
> > If some data exists in both cache and disk, I currently have it use the data
> > from disk. My reasoning is this: before the disk load occurs, any data
> gathered
> > in the cache would be based on a blank slate: for example, an alt-svc could
be
> > marked broken even though it never should've been tried in the first place
> > (since we have no broken alt-svc info loaded yet). Because of this, I simply
> > have the disk data overwrite the cache data for entries that exist in both.
> This
> > is true for both expiration time and broken count.
> > Is this a reasonable assumption? I could see maybe for expiration time, if
the
> > existing expiration time in cache is longer than the one being loaded from
> disk,
> > an explicit comparison (i.e. choosing the larger expiration time) might be
> > better?
> 
> Urm, I don't think we could assume before disk load happens, the cache is
based
> on a blank slate. Think about the case that a previous disk load happens,
memory
> state is updated based on the disk load. Of course, updates are also written
> back to the disk with some delay. Next time when you restart, disk and memory
> could be in inconsistent states, and now which one would you trust? Of course,
> all those are heuristics to improve user experience, I think both should be
> fine.
> 
> +rch@: Ryan, WDYT?

I think we should be consistent with the behavior of other similar methods in
this class. What do they do?

[wangyix1, 2017-06-19 19:47:35]

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

https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
net/http/broken_alternative_services.cc:166: // its position in
|broken_alternative_service_list|.
On 2017/06/19 17:05:35, Ryan Hamilton wrote:
> On 2017/06/13 21:45:22, Zhongyi Shi wrote:
> > On 2017/06/13 01:17:46, wangyix1 wrote:
> > > On 2017/06/12 23:20:38, Zhongyi Shi wrote:
> > > > This method has no guarantee on when it will be called. (Although I
agree
> > with
> > > > you that this possibly will only called when we load data from disk to
> > memory,
> > > > i.e., a restart). 
> > > > 
> > > > If an entry exists in both memory, i.e.,
broken_alternative_service_list_,
> > and
> > > > the list it was passed in, broken_alternativbe_service_list, and the
> > > expiration
> > > > is different, which one would you use? 
> > > > 
> > > > Consider the callsite of this method, I guess I would use the data in
> memory
> > > or
> > > > do an explicit comparison to make the call? 
> > > 
> > > If some data exists in both cache and disk, I currently have it use the
data
> > > from disk. My reasoning is this: before the disk load occurs, any data
> > gathered
> > > in the cache would be based on a blank slate: for example, an alt-svc
could
> be
> > > marked broken even though it never should've been tried in the first place
> > > (since we have no broken alt-svc info loaded yet). Because of this, I
simply
> > > have the disk data overwrite the cache data for entries that exist in
both.
> > This
> > > is true for both expiration time and broken count.
> > > Is this a reasonable assumption? I could see maybe for expiration time, if
> the
> > > existing expiration time in cache is longer than the one being loaded from
> > disk,
> > > an explicit comparison (i.e. choosing the larger expiration time) might be
> > > better?
> > 
> > Urm, I don't think we could assume before disk load happens, the cache is
> based
> > on a blank slate. Think about the case that a previous disk load happens,
> memory
> > state is updated based on the disk load. Of course, updates are also written
> > back to the disk with some delay. Next time when you restart, disk and
memory
> > could be in inconsistent states, and now which one would you trust? Of
course,
> > all those are heuristics to improve user experience, I think both should be
> > fine.
> > 
> > +rch@: Ryan, WDYT?
> 
> I think we should be consistent with the behavior of other similar methods in
> this class. What do they do?

The other setters in HttpServerPropertiesImpl() also have disk entries overwrite
existing cache entries, which is the current behavior of
SetBrokenAndRecentlyBrokenAlternativeServices().

[Ryan Hamilton, 2017-06-19 20:22:54]

On 2017/06/19 19:47:35, wangyix1 wrote:
>
https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
> File net/http/broken_alternative_services.cc (right):
> 
>
https://codereview.chromium.org/2917913002/diff/90001/net/http/broken_alterna...
> net/http/broken_alternative_services.cc:166: // its position in
> |broken_alternative_service_list|.
> On 2017/06/19 17:05:35, Ryan Hamilton wrote:
> > On 2017/06/13 21:45:22, Zhongyi Shi wrote:
> > > On 2017/06/13 01:17:46, wangyix1 wrote:
> > > > On 2017/06/12 23:20:38, Zhongyi Shi wrote:
> > > > > This method has no guarantee on when it will be called. (Although I
> agree
> > > with
> > > > > you that this possibly will only called when we load data from disk to
> > > memory,
> > > > > i.e., a restart). 
> > > > > 
> > > > > If an entry exists in both memory, i.e.,
> broken_alternative_service_list_,
> > > and
> > > > > the list it was passed in, broken_alternativbe_service_list, and the
> > > > expiration
> > > > > is different, which one would you use? 
> > > > > 
> > > > > Consider the callsite of this method, I guess I would use the data in
> > memory
> > > > or
> > > > > do an explicit comparison to make the call? 
> > > > 
> > > > If some data exists in both cache and disk, I currently have it use the
> data
> > > > from disk. My reasoning is this: before the disk load occurs, any data
> > > gathered
> > > > in the cache would be based on a blank slate: for example, an alt-svc
> could
> > be
> > > > marked broken even though it never should've been tried in the first
place
> > > > (since we have no broken alt-svc info loaded yet). Because of this, I
> simply
> > > > have the disk data overwrite the cache data for entries that exist in
> both.
> > > This
> > > > is true for both expiration time and broken count.
> > > > Is this a reasonable assumption? I could see maybe for expiration time,
if
> > the
> > > > existing expiration time in cache is longer than the one being loaded
from
> > > disk,
> > > > an explicit comparison (i.e. choosing the larger expiration time) might
be
> > > > better?
> > > 
> > > Urm, I don't think we could assume before disk load happens, the cache is
> > based
> > > on a blank slate. Think about the case that a previous disk load happens,
> > memory
> > > state is updated based on the disk load. Of course, updates are also
written
> > > back to the disk with some delay. Next time when you restart, disk and
> memory
> > > could be in inconsistent states, and now which one would you trust? Of
> course,
> > > all those are heuristics to improve user experience, I think both should
be
> > > fine.
> > > 
> > > +rch@: Ryan, WDYT?
> > 
> > I think we should be consistent with the behavior of other similar methods
in
> > this class. What do they do?
> 
> The other setters in HttpServerPropertiesImpl() also have disk entries
overwrite
> existing cache entries, which is the current behavior of
> SetBrokenAndRecentlyBrokenAlternativeServices().

Thanks! LGTM

[wangyix1, 2017-06-19 20:29:38]

The CQ bit was checked by wangyix@chromium.org

[commit-bot: I haz the power, 2017-06-19 20:29:58]

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-19 22:05:25]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1497904178755250,
"parent_rev": "dae11088a2290b0e88b12bbbb1728a95618b5f3b", "commit_rev":
"c10f1e6870cb3253860dffe55ddad9a0ab646801"}

[commit-bot: I haz the power, 2017-06-19 22:05:39]

Description was changed from

==========
Add getters and setters for BrokenAlternativeService's list of broken and
recently-broken alt svcs

This CL is in preparation for modifying HttpServerPropertiesManager to persist
broken and recently-broken alt svcs to disk.

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

to

==========
Add getters and setters for BrokenAlternativeService's list of broken and
recently-broken alt svcs

This CL is in preparation for modifying HttpServerPropertiesManager to persist
broken and recently-broken alt svcs to disk.

BUG=705029

Review-Url: https://codereview.chromium.org/2917913002
Cr-Commit-Position: refs/heads/master@{#480596}
Committed:
https://chromium.googlesource.com/chromium/src/+/c10f1e6870cb3253860dffe55dda...
==========

[commit-bot: I haz the power, 2017-06-19 22:05:40]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/c10f1e6870cb3253860dffe55dda...