Sdch view for net-internals

Sdch view for net-internals

BUG=

R=mmenke, rdsmith, jwd

TEST=NetInternalsTest.netInternalsTourTabs
     NetInternalsTest.netInternalsExport*/Import*
     NetInternalsTest.netInternalsSdchView*

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

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

[baranovich, 2014-07-28 10:04:22]

Hi Matt,
Not sure about the best reviewers for this CL.

Following
https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/iKlYMSt0MAA

This CL introduces net-internals#sdch tab more or less in a way it was done a
year ago in Yandex, although with some improvements
The tab displays list of loaded dictionaries, blacklisted domains and two lists
of errors: dictionary load/save errors and dictionary use errors.

My today's concerns are all about |problems_| collection, which holds 'url:
error' pairs. If it is so problematic, why not add it at all? It is the most
useful thing in all that page, to my mind:) it allows the developer to see which
errors actually occur.
1) unlimited |problems_| size. But this is easily fixable although.
2) mutable |problems_|. Since adding to problems collection is hooked to former
static SdchErrorRecovery which was called from const method
SdchManager::CanFetchDictionary. Another options are to make it non-const or
refactor the way it works, so that SdchErrorRecovery was called from outside.
3) The |problems_| collection itself and hooking adding into it to
SdchErrorRecovery seems a hack a little bit (but it was the cheapest thing to
do). Ideally I would like to add use-related errors into corresponding
URLRequest netlog, instead of keeping some separate collection. But I don't see
how to cope with dictionary fetch errors. Use errors are local and if the
developer wants to see what's wrong, she just opens net-internals, loads the
resource, and sees whats going on. But dictionary fetches are done in the
background, and I'm not sure if it is good to hook that stuff to net-log
existence. Also, a little bit unclear which event source to use. Maybe add some
long-living source called SDCH_FETCHES. Another thing is that introducing
net-log events will cause serious Sdch API changes. All sdch-error related
methods will have to take BoundNetLog argument, or return SdchProblems values
instead of just bools, so that the client could fill the netlog.
4) Sdch error names. Now they're just numbers, names would be better. To avoid
writing a lot of boilerpalte, I would suggest moving SdchProblems enum into
separate macro-filled file (like net_errors.h).

[mmenke, 2014-07-28 13:24:18]

I'm a good reviewer for the net-internals portion of this CL.  I'll see if I can
find someone for the sdch portion (Which, admittedly, looks pretty simple).

I'm taking today off, so won't get back to you until tomorrow, but I'll send out
an email asking about SDCH reviewers today.

On 2014/07/28 10:04:22, baranovich wrote:
> Hi Matt,
> Not sure about the best reviewers for this CL.
> 
> Following
> https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/iKlYMSt0MAA
> 
> This CL introduces net-internals#sdch tab more or less in a way it was done a
> year ago in Yandex, although with some improvements
> The tab displays list of loaded dictionaries, blacklisted domains and two
lists
> of errors: dictionary load/save errors and dictionary use errors.
> 
> My today's concerns are all about |problems_| collection, which holds 'url:
> error' pairs. If it is so problematic, why not add it at all? It is the most
> useful thing in all that page, to my mind:) it allows the developer to see
which
> errors actually occur.
> 1) unlimited |problems_| size. But this is easily fixable although.
> 2) mutable |problems_|. Since adding to problems collection is hooked to
former
> static SdchErrorRecovery which was called from const method
> SdchManager::CanFetchDictionary. Another options are to make it non-const or
> refactor the way it works, so that SdchErrorRecovery was called from outside.
> 3) The |problems_| collection itself and hooking adding into it to
> SdchErrorRecovery seems a hack a little bit (but it was the cheapest thing to
> do). Ideally I would like to add use-related errors into corresponding
> URLRequest netlog, instead of keeping some separate collection. But I don't
see
> how to cope with dictionary fetch errors. Use errors are local and if the
> developer wants to see what's wrong, she just opens net-internals, loads the
> resource, and sees whats going on. But dictionary fetches are done in the
> background, and I'm not sure if it is good to hook that stuff to net-log
> existence. Also, a little bit unclear which event source to use. Maybe add
some
> long-living source called SDCH_FETCHES. Another thing is that introducing
> net-log events will cause serious Sdch API changes. All sdch-error related
> methods will have to take BoundNetLog argument, or return SdchProblems values
> instead of just bools, so that the client could fill the netlog.
> 4) Sdch error names. Now they're just numbers, names would be better. To avoid
> writing a lot of boilerpalte, I would suggest moving SdchProblems enum into
> separate macro-filled file (like net_errors.h).

[Randy Smith (Not in Mondays), 2014-07-28 14:36:26]

On 2014/07/28 13:24:18, mmenke wrote:
> I'm a good reviewer for the net-internals portion of this CL.  I'll see if I
can
> find someone for the sdch portion (Which, admittedly, looks pretty simple).
> 
> I'm taking today off, so won't get back to you until tomorrow, but I'll send
out
> an email asking about SDCH reviewers today.

I've added myself for the SDCH review; I intend to look at it and give initial
feedback today.

> 
> On 2014/07/28 10:04:22, baranovich wrote:
> > Hi Matt,
> > Not sure about the best reviewers for this CL.
> > 
> > Following
> > https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/iKlYMSt0MAA
> > 
> > This CL introduces net-internals#sdch tab more or less in a way it was done
a
> > year ago in Yandex, although with some improvements
> > The tab displays list of loaded dictionaries, blacklisted domains and two
> lists
> > of errors: dictionary load/save errors and dictionary use errors.
> > 
> > My today's concerns are all about |problems_| collection, which holds 'url:
> > error' pairs. If it is so problematic, why not add it at all? It is the most
> > useful thing in all that page, to my mind:) it allows the developer to see
> which
> > errors actually occur.
> > 1) unlimited |problems_| size. But this is easily fixable although.
> > 2) mutable |problems_|. Since adding to problems collection is hooked to
> former
> > static SdchErrorRecovery which was called from const method
> > SdchManager::CanFetchDictionary. Another options are to make it non-const or
> > refactor the way it works, so that SdchErrorRecovery was called from
outside.
> > 3) The |problems_| collection itself and hooking adding into it to
> > SdchErrorRecovery seems a hack a little bit (but it was the cheapest thing
to
> > do). Ideally I would like to add use-related errors into corresponding
> > URLRequest netlog, instead of keeping some separate collection. But I don't
> see
> > how to cope with dictionary fetch errors. Use errors are local and if the
> > developer wants to see what's wrong, she just opens net-internals, loads the
> > resource, and sees whats going on. But dictionary fetches are done in the
> > background, and I'm not sure if it is good to hook that stuff to net-log
> > existence. Also, a little bit unclear which event source to use. Maybe add
> some
> > long-living source called SDCH_FETCHES. Another thing is that introducing
> > net-log events will cause serious Sdch API changes. All sdch-error related
> > methods will have to take BoundNetLog argument, or return SdchProblems
values
> > instead of just bools, so that the client could fill the netlog.
> > 4) Sdch error names. Now they're just numbers, names would be better. To
avoid
> > writing a lot of boilerpalte, I would suggest moving SdchProblems enum into
> > separate macro-filled file (like net_errors.h).

[Randy Smith (Not in Mondays), 2014-07-28 19:52:43]

[Matt's perspective specifically requested.]

My main concern is about the problem list being stored on the sdch manager. 
There are a couple of pieces of this concern:
* That list will grow without limit and is only used for debugging; it makes
more sense to me for it to be stored by the net-internals subsystem for the
period of time that subsystem is active (this is what we do with most of the
other net-internals targeted information).  
* Sdch information is per-profile, and my impression had been that net-internals
showed browser global information.  However, I note that 
NetInternalsMessageHandler is storing a per-profile variable in
main_context_getter_, which goes against my impression :-} (Matt?)  I'm pretty
sure we even if net-internals isn't global that we have to deal with both
incognito and non-incognito sources of information for that profile.
* For net-internals events, we call logging functions inside the main code and
let net-internals decide what to do with that info.  This CL does the reverse,
storing information in the net subsystem and retrieving it from net-internals
when wanted.  I'm not certain that this is wrong; there's clearly value in
displaying the problem list separately from the events list, just to make it
more accessible, and storage within SDCHManager and retrieval by the subsystem
makes a lot of sense for data that's continuing state that basically needs to be
stored anyway (e.g. the dictionary list).

My inclination as to how to solve this problem would be that if something
happens at a specific point in time (not a continuing state) it should be logged
either through the events system or some similar method (i.e. handed off to
net-internals to be stored or dropped on the floor as appropriate).  Problem
codes and related URLs would fall into this category.  Data that's a continuing
state of the subsystem (e.g. dictionary lists and contents, blacklists) can be
stored in the subsystem and queried for as needed.  The per-profile issue
mentioned above will still have to be dealt with for this data. 

The question of how the problem information should be displayed is trickier (and
completely up to Matt; I'm just sharing my thoughts).  I'd be inclined to say
that, if we can integrate it with the event system in a way that still makes the
information reasonably accessible to someone scanning for problems, we should,
as that doesn't drop the timestamp information.  I'd sorta like that information
to be integrated with the event source (URL_REQUEST?) it is related to, but
that'll require a fair ways more plumbing than just hacking SdchErrorRecovery.

https://codereview.chromium.org/423813002/diff/1/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/1/chrome/browser/resources/net...
chrome/browser/resources/net_internals/main.js:204: addTab(SdchView);
(Not sure this is the right place in the code for this comment.)  My inclination
is that, in the drop down list of tabs in net-internals, SDCH should be with the
other protocols (QUIC, SPDY, maybe HSTS).  Matt's the authority, though.

https://codereview.chromium.org/423813002/diff/1/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/sdch_view.html (right):

https://codereview.chromium.org/423813002/diff/1/chrome/browser/resources/net...
chrome/browser/resources/net_internals/sdch_view.html:44: <tr
jsselect="blacklisted">
You might want to be aware of https://codereview.chromium.org/414563002; I'm
adding recording of the root causes for blacklisting.

https://codereview.chromium.org/423813002/diff/1/chrome/browser/resources/net...
chrome/browser/resources/net_internals/sdch_view.html:62: <td
jscontent="error"></td>
The errors are currently shown in the view by number.  It'd be a lot more useful
for debugging if they were shown by symbol.  That can be done a la
net/base/net_error_list.h, but you want to make sure that exported versions of
the net internals file show up correctly even if they are shown on versions of
chrome different from the one they were saved on.  Matt can probably guide you
better than I as to how to do that.

[baranovich, 2014-07-28 22:11:27]

Hi Randy,

As for me, I totally agree, that this is a debug info and should be stored in
some better place, initially I thought that dictionaries may be fetched in the
loong background, but the are fetched only with 100ms delay after source
request.

Could you please provide some additional info on where I can find examples of
storing (and updating) some info in the net-internals subsystem? I was under
impression, that net-internals is almost stateless and just asks info from
different components in a loop. SdchManager is unaware of net-internals
existence, unless we want to introduce some delegates...

>You might want to be aware of https://codereview.chromium.org/414563002; I'm
>adding recording of the root causes for blacklisting.
Yep, thanks for pointing this out! I'll integrate reasons into webui.

We may try to come up here with the way SdchManager can be refactored to show
errors in event list. There are 4 places which produce sdch problems:
1) static Filter::FixupEncodingTypes
2) SdchFilter
Both of them might get URL_REQUEST BoundNetLog via FilterContext and pass it to
some sort of SdchErrorRecovery(SdchProblem, GURL, BoundNetLog) method or add
Event to netlog by themselves.
3) SdchDictionaryFetcher can use netlog of fetch request.
4) SdchManager itself
I think it would be plausible to pass BoundNetLog argument to all SdchManager
methods producing errors.
Another way may be to return ProblemCodes instead of bool from most SdchManager
methods, so that callers (which are essentially URLrequestHttpJob, SdchFilter
and SdchDictionaryFetcher) may report errors by themselves.
Other ways seem too exotic and complicated to me:)

[mmenke, 2014-07-29 19:48:41]

On 2014/07/28 22:11:27, baranovich wrote:
> Hi Randy,
> 
> As for me, I totally agree, that this is a debug info and should be stored in
> some better place, initially I thought that dictionaries may be fetched in the
> loong background, but the are fetched only with 100ms delay after source
> request.
> 
> Could you please provide some additional info on where I can find examples of
> storing (and updating) some info in the net-internals subsystem? I was under
> impression, that net-internals is almost stateless and just asks info from
> different components in a loop. SdchManager is unaware of net-internals
> existence, unless we want to introduce some delegates...
> 
> >You might want to be aware of https://codereview.chromium.org/414563002; I'm
> >adding recording of the root causes for blacklisting.
> Yep, thanks for pointing this out! I'll integrate reasons into webui.
> 
> We may try to come up here with the way SdchManager can be refactored to show
> errors in event list. There are 4 places which produce sdch problems:
> 1) static Filter::FixupEncodingTypes
> 2) SdchFilter
> Both of them might get URL_REQUEST BoundNetLog via FilterContext and pass it
to
> some sort of SdchErrorRecovery(SdchProblem, GURL, BoundNetLog) method or add
> Event to netlog by themselves.
> 3) SdchDictionaryFetcher can use netlog of fetch request.
> 4) SdchManager itself
> I think it would be plausible to pass BoundNetLog argument to all SdchManager
> methods producing errors.
> Another way may be to return ProblemCodes instead of bool from most
SdchManager
> methods, so that callers (which are essentially URLrequestHttpJob, SdchFilter
> and SdchDictionaryFetcher) may report errors by themselves.
> Other ways seem too exotic and complicated to me:)

Randy:  NetLog events are globals, but other net-internals tabs are are just for
the current profile's main URLRequestContext (Listing socket pools for all
contexts for all profiles would get pretty crazy, and we don'y have all the
pointers, anyways).


I think passing in the request's BoundNetLog makes the most sense - could just
return more data to the URLRequestHttpJob, but putting the logging in the filter
allows for more data for certain errors, if warranted, and means the
URLRequestHttpJob doesn't need to know all the possible error types.  That
having been said, I think we may want to pass a network error along as well.  If
we could pass them all the way to devtools, and add strings for the new error
codes, that would help developers more generally than just adding stuff to
net-internals.

Also, when there's an error, we should make sure a net_error parameter is also
logged, if there isn't already, so that the event is red in the events list.

For getting error strings in net-internals, see net/base/net_log_logger's
GetConstants function.  Using the new int->string mapping also has to be added
to the Javascript files.

We can also make the SDCH tab scrape SDCH error from the NetLog.

[Randy Smith (Not in Mondays), 2014-07-30 14:34:39]

On 2014/07/29 19:48:41, mmenke wrote:
> On 2014/07/28 22:11:27, baranovich wrote:
> > Hi Randy,
> > 
> > As for me, I totally agree, that this is a debug info and should be stored
in
> > some better place, initially I thought that dictionaries may be fetched in
the
> > loong background, but the are fetched only with 100ms delay after source
> > request.
> > 
> > Could you please provide some additional info on where I can find examples
of
> > storing (and updating) some info in the net-internals subsystem? I was under
> > impression, that net-internals is almost stateless and just asks info from
> > different components in a loop. SdchManager is unaware of net-internals
> > existence, unless we want to introduce some delegates...
> > 
> > >You might want to be aware of https://codereview.chromium.org/414563002;
I'm
> > >adding recording of the root causes for blacklisting.
> > Yep, thanks for pointing this out! I'll integrate reasons into webui.
> > 
> > We may try to come up here with the way SdchManager can be refactored to
show
> > errors in event list. There are 4 places which produce sdch problems:
> > 1) static Filter::FixupEncodingTypes
> > 2) SdchFilter
> > Both of them might get URL_REQUEST BoundNetLog via FilterContext and pass it
> to
> > some sort of SdchErrorRecovery(SdchProblem, GURL, BoundNetLog) method or add
> > Event to netlog by themselves.
> > 3) SdchDictionaryFetcher can use netlog of fetch request.
> > 4) SdchManager itself
> > I think it would be plausible to pass BoundNetLog argument to all
SdchManager
> > methods producing errors.
> > Another way may be to return ProblemCodes instead of bool from most
> SdchManager
> > methods, so that callers (which are essentially URLrequestHttpJob,
SdchFilter
> > and SdchDictionaryFetcher) may report errors by themselves.
> > Other ways seem too exotic and complicated to me:)
> 
> Randy:  NetLog events are globals, but other net-internals tabs are are just
for
> the current profile's main URLRequestContext (Listing socket pools for all
> contexts for all profiles would get pretty crazy, and we don'y have all the
> pointers, anyways).
> 
> 
> I think passing in the request's BoundNetLog makes the most sense - could just
> return more data to the URLRequestHttpJob, but putting the logging in the
filter
> allows for more data for certain errors, if warranted, and means the
> URLRequestHttpJob doesn't need to know all the possible error types.  That
> having been said, I think we may want to pass a network error along as well. 
If
> we could pass them all the way to devtools, and add strings for the new error
> codes, that would help developers more generally than just adding stuff to
> net-internals.
> 
> Also, when there's an error, we should make sure a net_error parameter is also
> logged, if there isn't already, so that the event is red in the events list.
> 
> For getting error strings in net-internals, see net/base/net_log_logger's
> GetConstants function.  Using the new int->string mapping also has to be added
> to the Javascript files.
> 
> We can also make the SDCH tab scrape SDCH error from the NetLog.

All this SGTM.  baranovich@: I have some preference for making the SdchManager
<-> everything else interface not include a netlog (i.e. return information from
it as needed for logging, and do the logging in the calling function), but if
that's too messy, feel free to pass in netlogs.  Everything else (SdchFilter,
SdchDictionaryFetcher) i'm fine with taking netlogs.  Let us know if you need
anything else for the next rewrite; thanks!

[baranovich, 2014-08-02 21:17:16]

Matt, Randy, I'm done with rewrite.
I've added 2 types of netlog events: for resource handling and dictionary
fetches. Also SdchDictionsryFetcher uses its own netlog source to track
fetch-related errors. But not all, some fetch errors (which come from
SdchManager::FetchDictionary) are logged via URL_REQUEST netlog to be able to
track both dictionary url and URL_REQUEST url.

I deleted problem tables from the sdch_view in favor of links to event filters.

Not sure whether should I add new net internals tests. Such simple tabs (like
quic or spdy) are not tested.

[baranovich, 2014-08-02 21:17:25]

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:324: if (!IsSdchEnabledForUrl(url))
I've split this method into two for 2 reasons:
1) to distinguish between disabled SDCH and blacklisted domain.
2) do not call SdchErrorRecovery via IsInSupportedDomain from AddSdchDictionary
and GetVcdiffDictionary. It is done from the outside now.

[Randy Smith (Not in Mondays), 2014-08-04 15:37:35]

On 2014/08/02 21:17:25, baranovich wrote:
> https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (right):
> 
>
https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.cc...
> net/base/sdch_manager.cc:324: if (!IsSdchEnabledForUrl(url))
> I've split this method into two for 2 reasons:
> 1) to distinguish between disabled SDCH and blacklisted domain.
> 2) do not call SdchErrorRecovery via IsInSupportedDomain from
AddSdchDictionary
> and GetVcdiffDictionary. It is done from the outside now.

I'm racing to land something by feature freeze, so I may not get to this today. 
I will try very hard to get to it by tomorrow.

[Randy Smith (Not in Mondays), 2014-08-05 17:50:34]

baranovich@: Thanks very much for this!  I think we're heading in the right
direction; hopefully we can satisfy my architectural persnicketyness (sic?) with
another round or two.  High level comments (with a couple of questions for
Matt):

+ In the absence of Matt's opinion, I'd still like to see a different
  positioning of SDCH in the dropdown menu list.
+ It looks like the SOURCE_SDCH_DICTIONARY_FETCHER only shows up in
  the events list in the event of a problem.  I don't like that; if we
  do a dictionary fetch, I'd like to see an event for it.  Willing to
  to do additional instrumentation on ScheduleDelayedRun(),
  StartFetching, and OnURLFetchComplete for the success case?  
+ Matt: Given that the SDCH dictionary fetcher is responsible for
  starting the URL request, it seems like we should have a source
  linkage back and forth between the two.  But it doesn't look like
  URLRequest does this for anything else that spawns it.  Do you agree
  there should be a source linkage?
+ Matt: Currently all SDCH dictionary fetches will be bundled under a
  single event source.  My inclination would be that we should have a
  separate event source for each fetch (losely, each time
  ScheduleDelayedRun() is called we should create a new event
  source).  Opinion?
+ Rather than putting SDCH related interfaces in the fairly abstract
  Filter and FilterContext interfaces, I'd be inclined instead to
  expore a BoundNetLog from FilterContext to which filters that wish
  to can log whatever events they want.  More likely to be useful to
  general FilterContext consumers.

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:91: // no error occurs (SDCH or SDCH-over-https may be
disabled).
I would think we'd want to add errors in this case.  The reason for returning an
AddResult structure is that we get context for why a failure occurred, so there
being cases where we don't get that context is unfortunate.

This opens a (relatively small) can of worms in that ProblemCodes is a list of
values that go into a histogram, and if you change that it's messy (you need to
rev the histograms that use it).  Possibly we should create a new enum for the
possible return values from this routine.

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:92: struct AddResult {
I don't think there's any need to make this a structure, at least if you go with
my comment above--just return the problem code and make it ..._OK if the
dictionary was added.  If you don't end up going that route, I think it's more
in line with chromium coding conventions to return a boolean and take a pointer
to a ProblemCode to give more detail.

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:148: base::DictionaryValue* DictionaryInfoToValue()
const;
I think it's reasonable to make all of the information needed by this function
part of Dictionary's public interface, and I'd rather have it there than make
Dictionary construct a json value purely for net-internals (see my note below to
Matt about SdchInfoToValue()).

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:277: base::Value* SdchInfoToValue() const;
I keep wincing at the existence of this function; to my mind, the SdchManager
shouldn't know about consing up its internal state for the net-internals
debugging functions, and the interface shouldn't have methods purely for that
purpose.  I'd rather have the net internals system reach in for the data and
construct the json itself.  

When I look at what data isn't public, I get:
* The dictionary list.  I'd be good with GetAvailDictionaryList returning a list
of dictionary pointers (with strong constraints on how long they're valid for).
* blacklisted domains.  I don't see a way around breaking the interface for
this, so I'd suggest putting an interface to get the blacklisted domains, with
details (maybe with a ForDebugging suffix to make it clear this isn't intended
for ordinary use) into the SdchManager interface.

https://codereview.chromium.org/423813002/diff/40001/net/filter/sdch_filter.h
File net/filter/sdch_filter.h (right):

https://codereview.chromium.org/423813002/diff/40001/net/filter/sdch_filter.h...
net/filter/sdch_filter.h:68: SdchManager* GetSdchManager() const;
As best I can tell, this is only used once, and the definition is simple enough
that I'm inclined to inline it.

https://codereview.chromium.org/423813002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/423813002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:352: if (request_)
We've indirected request_ several lines above; either there's no need for this
check, or there's a bug in that line.

[mmenke, 2014-08-05 18:55:17]

Talked a bit to rdsmith about this.

What we're thinking is more along the lines of passing in the
UrlRequestHttpJob's BoundNetLog to the filter object, and have it log events to
that NetLog as needed related to the status of that filter.

For dictionary  requests, once the URLRequest is created, but before it's
started, the SdchManager can log a special event to the request's BoundNetLog to
indicate it's an SDCH dictionary request.  This behavior isn't lovely, but it
seems better than having a single source for all SDCH dictionary requests, and
less spammy than having a top level event for each individual dictionary
request.

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:277: base::Value* SdchInfoToValue() const;
On 2014/08/05 17:50:34, rdsmith wrote:
> I keep wincing at the existence of this function; to my mind, the SdchManager
> shouldn't know about consing up its internal state for the net-internals
> debugging functions, and the interface shouldn't have methods purely for that
> purpose.  I'd rather have the net internals system reach in for the data and
> construct the json itself.  
> 
> When I look at what data isn't public, I get:
> * The dictionary list.  I'd be good with GetAvailDictionaryList returning a
list
> of dictionary pointers (with strong constraints on how long they're valid
for).
> * blacklisted domains.  I don't see a way around breaking the interface for
> this, so I'd suggest putting an interface to get the blacklisted domains, with
> details (maybe with a ForDebugging suffix to make it clear this isn't intended
> for ordinary use) into the SdchManager interface.

This is how things are often done, currently, when you want to log non-public
data.  I'm a bit reluctant to make more data public, since that encourages
tighter coupling, API bloat, and misuse/abuse of functions really intended only
for logging.  Also, why should the caller know or care about the structure of
the internal state of the SdchManager?  Of course, net-internals still needs
dependencies on the data in the dictionary.

[Randy Smith (Not in Mondays), 2014-08-05 19:41:01]

On 2014/08/05 18:55:17, mmenke wrote:
> Talked a bit to rdsmith about this.
> 
> What we're thinking is more along the lines of passing in the
> UrlRequestHttpJob's BoundNetLog to the filter object, and have it log events
to
> that NetLog as needed related to the status of that filter.
> 
> For dictionary  requests, once the URLRequest is created, but before it's
> started, the SdchManager can log a special event to the request's BoundNetLog
to
> indicate it's an SDCH dictionary request.  This behavior isn't lovely, but it
> seems better than having a single source for all SDCH dictionary requests, and
> less spammy than having a top level event for each individual dictionary
> request.

One clarification: The SdchDictionaryFetcher should log the special event, not
the SdchManager.

> 
> https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h
> File net/base/sdch_manager.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
> net/base/sdch_manager.h:277: base::Value* SdchInfoToValue() const;
> On 2014/08/05 17:50:34, rdsmith wrote:
> > I keep wincing at the existence of this function; to my mind, the
SdchManager
> > shouldn't know about consing up its internal state for the net-internals
> > debugging functions, and the interface shouldn't have methods purely for
that
> > purpose.  I'd rather have the net internals system reach in for the data and
> > construct the json itself.  
> > 
> > When I look at what data isn't public, I get:
> > * The dictionary list.  I'd be good with GetAvailDictionaryList returning a
> list
> > of dictionary pointers (with strong constraints on how long they're valid
> for).
> > * blacklisted domains.  I don't see a way around breaking the interface for
> > this, so I'd suggest putting an interface to get the blacklisted domains,
with
> > details (maybe with a ForDebugging suffix to make it clear this isn't
intended
> > for ordinary use) into the SdchManager interface.
> 
> This is how things are often done, currently, when you want to log non-public
> data.  I'm a bit reluctant to make more data public, since that encourages
> tighter coupling, API bloat, and misuse/abuse of functions really intended
only
> for logging.  Also, why should the caller know or care about the structure of
> the internal state of the SdchManager?  Of course, net-internals still needs
> dependencies on the data in the dictionary.

Ok.  If this is an existing pattern, I'm ok with it.  I will want a comment on
the method, though :-}.

[baranovich, 2014-08-05 19:49:11]

Hi Matt, Randy!

> + In the absence of Matt's opinion, I'd still like to see a different
>   positioning of SDCH in the dropdown menu list.

Sure, let's choose a better position:)

> + It looks like the SOURCE_SDCH_DICTIONARY_FETCHER only shows up in
>   the events list in the event of a problem.  I don't like that; if we
>   do a dictionary fetch, I'd like to see an event for it.  Willing to
>   to do additional instrumentation on ScheduleDelayedRun(),
>   StartFetching, and OnURLFetchComplete for the success case?

One more thing left which bothers me about dictionary fetch errors is that part
of the errors (which are returned from SdchManager::FetchDictionary()) are
logged into log of request which got Get-Dictionary header. Ideally all these
errors should go to SOURCE_SDCH_DICTIONARY_FETCHER. But that require some
inversion of control in code flow (URLRequestHttpJob calls
SdchManager::FetchDict, SdchManager calls Fetcher::Schedule, fetcher calls
SdchManager::CanFetch and gets all errors). What do you think about that?

> For dictionary  requests, once the URLRequest is created, but before it's
> started, the SdchManager can log a special event to the request's BoundNetLog
to
> indicate it's an SDCH dictionary request.  This behavior isn't lovely, but it
> seems better than having a single source for all SDCH dictionary requests, and
> less spammy than having a top level event for each individual dictionary
> request.

Unfortunately we don't have URLRequest there, only URLFetcher. I don't see the
way to get the underlying request or it's log:( switching to raw URLRequest is
too messy to do here imho. Getting net-log from URLFetcher is tricky, since
URLFetcher is thread-agnostic. No sure if we should add a method with some
subtle implications about calling thread.

> + Rather than putting SDCH related interfaces in the fairly abstract
>   Filter and FilterContext interfaces, I'd be inclined instead to
>   expore a BoundNetLog from FilterContext to which filters that wish
>   to can log whatever events they want.  More likely to be useful to
>   general FilterContext consumers.

There is a tricky place here: URLRequestJob lifetime differs from URLRequest.
job may outlive request if Delegate cancels the latter. In that case some ugly
plumbing (providing dummy BoundNetLog, or returning ptr instead of const ref) is
required for GetBoundNetLog() method. At first I wrote that way, but it appeared
to me a complexity for nothing, so I changed the interface to suit the immediate
needs... but if you're strongly in favor of exposing BoundNetLog, I'll do that:)

Regarding URLRequestJob lifetime: seems to me that the reference counting is not
necessary. WeakPtr guard (like in SpdyStream) may as well be used instead. Thus
we get more sound ownership semantics and might get rid of all if (request_)
checks in the URLRequest*Job code. But that's about different patch anyway=)

[mmenke, 2014-08-05 20:14:14]

On 2014/08/05 19:49:11, baranovich wrote:
> Hi Matt, Randy!
> 
> > + In the absence of Matt's opinion, I'd still like to see a different
> >   positioning of SDCH in the dropdown menu list.
> 
> Sure, let's choose a better position:)
> 
> > + It looks like the SOURCE_SDCH_DICTIONARY_FETCHER only shows up in
> >   the events list in the event of a problem.  I don't like that; if we
> >   do a dictionary fetch, I'd like to see an event for it.  Willing to
> >   to do additional instrumentation on ScheduleDelayedRun(),
> >   StartFetching, and OnURLFetchComplete for the success case?
> 
> One more thing left which bothers me about dictionary fetch errors is that
part
> of the errors (which are returned from SdchManager::FetchDictionary()) are
> logged into log of request which got Get-Dictionary header. Ideally all these
> errors should go to SOURCE_SDCH_DICTIONARY_FETCHER. But that require some
> inversion of control in code flow (URLRequestHttpJob calls
> SdchManager::FetchDict, SdchManager calls Fetcher::Schedule, fetcher calls
> SdchManager::CanFetch and gets all errors). What do you think about that?
> 
> > For dictionary  requests, once the URLRequest is created, but before it's
> > started, the SdchManager can log a special event to the request's
BoundNetLog
> to
> > indicate it's an SDCH dictionary request.  This behavior isn't lovely, but
it
> > seems better than having a single source for all SDCH dictionary requests,
and
> > less spammy than having a top level event for each individual dictionary
> > request.
> 
> Unfortunately we don't have URLRequest there, only URLFetcher. I don't see the
> way to get the underlying request or it's log:( switching to raw URLRequest is
> too messy to do here imho. Getting net-log from URLFetcher is tricky, since
> URLFetcher is thread-agnostic. No sure if we should add a method with some
> subtle implications about calling thread.

Ahh...Very good point.  Unless Randy has some ideas, I'll do some thinking and
digging around tomorrow.

[mmenke, 2014-08-06 14:24:03]

On 2014/08/05 20:14:14, mmenke wrote:
> On 2014/08/05 19:49:11, baranovich wrote:
> > Hi Matt, Randy!
> > 
> > > + In the absence of Matt's opinion, I'd still like to see a different
> > >   positioning of SDCH in the dropdown menu list.
> > 
> > Sure, let's choose a better position:)
> > 
> > > + It looks like the SOURCE_SDCH_DICTIONARY_FETCHER only shows up in
> > >   the events list in the event of a problem.  I don't like that; if we
> > >   do a dictionary fetch, I'd like to see an event for it.  Willing to
> > >   to do additional instrumentation on ScheduleDelayedRun(),
> > >   StartFetching, and OnURLFetchComplete for the success case?
> > 
> > One more thing left which bothers me about dictionary fetch errors is that
> part
> > of the errors (which are returned from SdchManager::FetchDictionary()) are
> > logged into log of request which got Get-Dictionary header. Ideally all
these
> > errors should go to SOURCE_SDCH_DICTIONARY_FETCHER. But that require some
> > inversion of control in code flow (URLRequestHttpJob calls
> > SdchManager::FetchDict, SdchManager calls Fetcher::Schedule, fetcher calls
> > SdchManager::CanFetch and gets all errors). What do you think about that?
> > 
> > > For dictionary  requests, once the URLRequest is created, but before it's
> > > started, the SdchManager can log a special event to the request's
> BoundNetLog
> > to
> > > indicate it's an SDCH dictionary request.  This behavior isn't lovely, but
> it
> > > seems better than having a single source for all SDCH dictionary requests,
> and
> > > less spammy than having a top level event for each individual dictionary
> > > request.
> > 
> > Unfortunately we don't have URLRequest there, only URLFetcher. I don't see
the
> > way to get the underlying request or it's log:( switching to raw URLRequest
is
> > too messy to do here imho. Getting net-log from URLFetcher is tricky, since
> > URLFetcher is thread-agnostic. No sure if we should add a method with some
> > subtle implications about calling thread.
> 
> Ahh...Very good point.  Unless Randy has some ideas, I'll do some thinking and
> digging around tomorrow.

Sorry, may not have time today.

[Randy Smith (Not in Mondays), 2014-08-07 17:41:35]

On 2014/08/05 20:14:14, mmenke wrote:
> On 2014/08/05 19:49:11, baranovich wrote:
> > Hi Matt, Randy!
> > 
> > > + In the absence of Matt's opinion, I'd still like to see a different
> > >   positioning of SDCH in the dropdown menu list.
> > 
> > Sure, let's choose a better position:)
> > 
> > > + It looks like the SOURCE_SDCH_DICTIONARY_FETCHER only shows up in
> > >   the events list in the event of a problem.  I don't like that; if we
> > >   do a dictionary fetch, I'd like to see an event for it.  Willing to
> > >   to do additional instrumentation on ScheduleDelayedRun(),
> > >   StartFetching, and OnURLFetchComplete for the success case?
> > 
> > One more thing left which bothers me about dictionary fetch errors is that
> part
> > of the errors (which are returned from SdchManager::FetchDictionary()) are
> > logged into log of request which got Get-Dictionary header. Ideally all
these
> > errors should go to SOURCE_SDCH_DICTIONARY_FETCHER. But that require some
> > inversion of control in code flow (URLRequestHttpJob calls
> > SdchManager::FetchDict, SdchManager calls Fetcher::Schedule, fetcher calls
> > SdchManager::CanFetch and gets all errors). What do you think about that?
> > 
> > > For dictionary  requests, once the URLRequest is created, but before it's
> > > started, the SdchManager can log a special event to the request's
> BoundNetLog
> > to
> > > indicate it's an SDCH dictionary request.  This behavior isn't lovely, but
> it
> > > seems better than having a single source for all SDCH dictionary requests,
> and
> > > less spammy than having a top level event for each individual dictionary
> > > request.
> > 
> > Unfortunately we don't have URLRequest there, only URLFetcher. I don't see
the
> > way to get the underlying request or it's log:( switching to raw URLRequest
is
> > too messy to do here imho. Getting net-log from URLFetcher is tricky, since
> > URLFetcher is thread-agnostic. No sure if we should add a method with some
> > subtle implications about calling thread.
> 
> Ahh...Very good point.  Unless Randy has some ideas, I'll do some thinking and
> digging around tomorrow.

Nothing brilliant from this corner, I'm afraid.  I am coming to the conclusion
that it would be a goodness to rewrite SdchDictionaryFetcher to use URLRequest
instead of URLFetcher; there are several small improvements that we could make
it we do that.  But I don't think this CL should be blocked on that--at a
minimum we could do something slightly more hacky than would normally be ok, and
fix it when we change over to URLRequest.  I'm not coming up with that slightly
hacky thing, though--adding a arbitrary netlogging interface to URLFetcher I
doubt would be very hard, but IMO goes above *slightly* hacky :-} :-{.

[Randy Smith (Not in Mondays), 2014-08-07 17:46:23]

On 2014/08/05 19:49:11, baranovich wrote:
> Hi Matt, Randy!
> 
> > + In the absence of Matt's opinion, I'd still like to see a different
> >   positioning of SDCH in the dropdown menu list.
> 
> Sure, let's choose a better position:)
> 
> > + It looks like the SOURCE_SDCH_DICTIONARY_FETCHER only shows up in
> >   the events list in the event of a problem.  I don't like that; if we
> >   do a dictionary fetch, I'd like to see an event for it.  Willing to
> >   to do additional instrumentation on ScheduleDelayedRun(),
> >   StartFetching, and OnURLFetchComplete for the success case?
> 
> One more thing left which bothers me about dictionary fetch errors is that
part
> of the errors (which are returned from SdchManager::FetchDictionary()) are
> logged into log of request which got Get-Dictionary header. Ideally all these
> errors should go to SOURCE_SDCH_DICTIONARY_FETCHER. But that require some
> inversion of control in code flow (URLRequestHttpJob calls
> SdchManager::FetchDict, SdchManager calls Fetcher::Schedule, fetcher calls
> SdchManager::CanFetch and gets all errors). What do you think about that?

Let's re-address this after we work out the basic architecture of the logging;
I've become a convert to having everything in the URLRequest associated with the
fetch, if we can find a way to do it.

> > For dictionary  requests, once the URLRequest is created, but before it's
> > started, the SdchManager can log a special event to the request's
BoundNetLog
> to
> > indicate it's an SDCH dictionary request.  This behavior isn't lovely, but
it
> > seems better than having a single source for all SDCH dictionary requests,
and
> > less spammy than having a top level event for each individual dictionary
> > request.
> 
> Unfortunately we don't have URLRequest there, only URLFetcher. I don't see the
> way to get the underlying request or it's log:( switching to raw URLRequest is
> too messy to do here imho. Getting net-log from URLFetcher is tricky, since
> URLFetcher is thread-agnostic. No sure if we should add a method with some
> subtle implications about calling thread.
> 
> > + Rather than putting SDCH related interfaces in the fairly abstract
> >   Filter and FilterContext interfaces, I'd be inclined instead to
> >   expore a BoundNetLog from FilterContext to which filters that wish
> >   to can log whatever events they want.  More likely to be useful to
> >   general FilterContext consumers.
> 
> There is a tricky place here: URLRequestJob lifetime differs from URLRequest.
> job may outlive request if Delegate cancels the latter. In that case some ugly
> plumbing (providing dummy BoundNetLog, or returning ptr instead of const ref)
is
> required for GetBoundNetLog() method. At first I wrote that way, but it
appeared
> to me a complexity for nothing, so I changed the interface to suit the
immediate
> needs... but if you're strongly in favor of exposing BoundNetLog, I'll do
that:)

I think I am; it makes much more sense to me to expose a netlog interface on the
filter context, than something SDCH specific.

> Regarding URLRequestJob lifetime: seems to me that the reference counting is
not
> necessary. WeakPtr guard (like in SpdyStream) may as well be used instead.
Thus
> we get more sound ownership semantics and might get rid of all if (request_)
> checks in the URLRequest*Job code. But that's about different patch anyway=)

Just calling out this as an area that I'd fear to tread in without a fair amount
of analysis and consultation.  Getting rid of refcounting is definitely good,
but this is a core area of the net stack, and (I'm pretty sure) has been gone
over by people more lifetime knowledgeable and anti-refcounting than I, and it's
still setup the way it is :-}.  Not wanting to discourage you if you want to
dive in, and I agree with your goals, but it may be a yak shave.  But if so, as
you say it's a yak shave in a different CL :-}.

[mmenke, 2014-08-07 17:52:47]

On 2014/08/07 17:41:35, rdsmith wrote:
> Nothing brilliant from this corner, I'm afraid.  I am coming to the conclusion
> that it would be a goodness to rewrite SdchDictionaryFetcher to use URLRequest
> instead of URLFetcher; there are several small improvements that we could make
> it we do that.  But I don't think this CL should be blocked on that--at a
> minimum we could do something slightly more hacky than would normally be ok,
and
> fix it when we change over to URLRequest.  I'm not coming up with that
slightly
> hacky thing, though--adding a arbitrary netlogging interface to URLFetcher I
> doubt would be very hard, but IMO goes above *slightly* hacky :-} :-{.

Something a little hacky until we have a better solution SGTM.  I have a bunch
of reviews this week, and just haven't had time to think about this.

On 2014/08/07 17:46:23, rdsmith wrote:
> On 2014/08/05 19:49:11, baranovich wrote:
> > Regarding URLRequestJob lifetime: seems to me that the reference counting is
> not
> > necessary. WeakPtr guard (like in SpdyStream) may as well be used instead.
> Thus
> > we get more sound ownership semantics and might get rid of all if (request_)
> > checks in the URLRequest*Job code. But that's about different patch anyway=)
> 
> Just calling out this as an area that I'd fear to tread in without a fair
amount
> of analysis and consultation.  Getting rid of refcounting is definitely good,
> but this is a core area of the net stack, and (I'm pretty sure) has been gone
> over by people more lifetime knowledgeable and anti-refcounting than I, and
it's
> still setup the way it is :-}.  Not wanting to discourage you if you want to
> dive in, and I agree with your goals, but it may be a yak shave.  But if so,
as
> you say it's a yak shave in a different CL :-}.

Baranovich is correct - the only reason for the refcounting is to handle
synchronous deletion during callbacks, and WeakPtrs could be used instead.  I
don't think this would be a big or complicated change, just have to be careful
about always checking for deletion.

[baranovich, 2014-08-12 20:54:51]

All comments addressed, PTAL once more.

Although still not clear what to do with Dictionary fetch events. We may leave
it as is (logging errors only), and add more verbose logging after we move from
URLFetcher to URLRequest for instance.

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:91: // no error occurs (SDCH or SDCH-over-https may be
disabled).
On 2014/08/05 17:50:34, rdsmith wrote:
> I would think we'd want to add errors in this case.  The reason for returning
an
> AddResult structure is that we get context for why a failure occurred, so
there
> being cases where we don't get that context is unfortunate.
> 
> This opens a (relatively small) can of worms in that ProblemCodes is a list of
> values that go into a histogram, and if you change that it's messy (you need
to
> rev the histograms that use it).  Possibly we should create a new enum for the
> possible return values from this routine.  
> 

Done.

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:92: struct AddResult {
On 2014/08/05 17:50:34, rdsmith wrote:
> I don't think there's any need to make this a structure, at least if you go
with
> my comment above--just return the problem code and make it ..._OK if the
> dictionary was added.  If you don't end up going that route, I think it's more
> in line with chromium coding conventions to return a boolean and take a
pointer
> to a ProblemCode to give more detail.

Done. Decided to go with bool + out param

https://codereview.chromium.org/423813002/diff/40001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:148: base::DictionaryValue* DictionaryInfoToValue()
const;
On 2014/08/05 17:50:34, rdsmith wrote:
> I think it's reasonable to make all of the information needed by this function
> part of Dictionary's public interface, and I'd rather have it there than make
> Dictionary construct a json value purely for net-internals (see my note below
to
> Matt about SdchInfoToValue()).

Done. Moved this to SdchInfoToValue()

https://codereview.chromium.org/423813002/diff/40001/net/filter/sdch_filter.h
File net/filter/sdch_filter.h (right):

https://codereview.chromium.org/423813002/diff/40001/net/filter/sdch_filter.h...
net/filter/sdch_filter.h:68: SdchManager* GetSdchManager() const;
On 2014/08/05 17:50:34, rdsmith wrote:
> As best I can tell, this is only used once, and the definition is simple
enough
> that I'm inclined to inline it.

Done. Deleted, It was unnecessary.

https://codereview.chromium.org/423813002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/423813002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:352: if (request_)
On 2014/08/05 17:50:34, rdsmith wrote:
> We've indirected request_ several lines above; either there's no need for this
> check, or there's a bug in that line.

Done.

[Randy Smith (Not in Mondays), 2014-08-13 17:35:46]

Quick round of responses; I want to go over the refactoring of
SdchErrorRecovery() and the split apart of IsInSupportedDomain() in more detail.

https://codereview.chromium.org/423813002/diff/120001/net/base/net_log_source...
File net/base/net_log_source_type_list.h (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/net_log_source...
net/base/net_log_source_type_list.h:31: SOURCE_TYPE(SDCH_DICTIONARY_FETCHER)
Probably worth adding a TODO() that we want to remove this when we switch to
URLRequest.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:41: if (!fetch_queue_.empty() &&
fetch_queue_.back() == dictionary_url) {
Huh.  It looks to me like this line is in error; we should actually scan the
entire queue, not just look at the last entry.  Willing to make the change while
you're here?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:43: dictionary_url);
I'm not sure this counts as an error that should be flashing red on the event
list.  Maybe plumb some way to log it s.t. the net error will be OK?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:48: dictionary_url);
Similar; this could be the result of a race, and I wouldn't think should show up
as an error.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:105: LogSdchProblem(problem,
source->GetURL());
I'm a bit uncomfortable with the implication of this call, which I read as that
true will be returned if-and-only-if the returned problem is PROBLEM_CODE_OK. 
Why duplicate the information?  It seems like it's asking for skew down the
line.  

If you're not comfortable just returning a problem code, could you check the
boolean return here?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:447: }
The extra braces aren't necessary by the chrome style guide, but I don't object
if you want them.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:515: return PROBLEM_CODE_OK;  // not a problem. SDCH is
disabled.
Hmmm.  Wouldn't we want to see this in net-internals?  (Though I don't think it
should happen.)

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:535: if (!IsInSupportedDomain(target_url)) {
I think it makes sense to avoid unnecessary changes in areas of the code where
you're not doing other changes.  I could even make a claim that adding the
braces goes against the intent of the style guide (which says to avoid vertical
whitespace if possible and that it's ok to skip braces on conditionals as long
as the containing statement and the condition itself is each one line in length,
and the then and else clause (if existing) match in terms of being braced.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:627: // Do not send error up. It's internal stuff, I
guess.
"I guess" doesn't seem like the right phrasing for a comment :-}; rephrase?   

You're welcome to surface this error or not as you wish; the latency experiment
is a random test where we hold back on 1 in 100 connections from using SDCH and
report UMA to compare the results, and the places I see where we disallow that
experiment is when we blacklist domains.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:26: #include <vector>
Is this still needed?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:34: #include "net/base/net_log.h"
Can't this be in the cc file?  Maybe with a forward declaration here, though I'm
not seeing the type use that would require it.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:39: class DictionaryValue;
Is this still needed?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:125: // given data, that arrived in response to get of
dictionary_url.
Specify return value interpretation?  (And below.)

It's a bit weird that you're specifying one method (AddSdchDictionary) that
returns a bool + writes a pointer with a problem code, but returning problem
codes from all the rest of these routines.  Is there some reason you'd rather
they didn't have the same signature pattern?

https://codereview.chromium.org/423813002/diff/120001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/filter/filter.cc#ne...
net/filter/filter.cc:174: success = filter_context.GetURL(&url);
Why the change?

[baranovich, 2014-08-13 18:37:16]

IsInSupportedDomain is tricky: it checks 3 things actually: if SDCH enabled, if
SDCH-over-https enabled and blacklisting. First two are not really problems. If
the problem is about blacklist, the method calls SdchErrorRecovery. This method
was called from URLRequestHttpJob, from  SdchManager::GetAvailDictionaties, from
SdchManager::AddSdchDictionary() and from SdchManager::GetVcdiffDictionary. The
latter two now return ProblemCodes to be logged outside, so if they use
IsInSupportedDomain as is, we will record histogram 2 times (or we may have some
ugly condition about it). First two call sites, on the other hand, should be
silent about the problems (especially GetAvailDictionaries, which calls
IsInSupportedDomain for each saved dictionary), but we still need to add the
histograms (to behave like before) but I guess these warnings should not go the
the net-log.

To my mind the interface of SdchManager becomes messy, SdchProblems enum was not
intended to be a return value, but changing this require much deeper
refactoring.

>I want to go over the refactoring of
>SdchErrorRecovery() and the split apart of IsInSupportedDomain() in more
detail.

[baranovich, 2014-08-13 18:38:58]

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:105: LogSdchProblem(problem,
source->GetURL());
PROBLEM_CODE_OK may be even if we haven't added the dictionary (if SDCH is
disabled, for instance).

On 2014/08/13 17:35:45, rdsmith wrote:
> I'm a bit uncomfortable with the implication of this call, which I read as
that
> true will be returned if-and-only-if the returned problem is PROBLEM_CODE_OK. 
> Why duplicate the information?  It seems like it's asking for skew down the
> line.  
> 
> If you're not comfortable just returning a problem code, could you check the
> boolean return here?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:125: // given data, that arrived in response to get of
dictionary_url.
If SDCH is disabled (or SDCH over https is disabled, or format-version != 1.0
(but this should be a problem I believe)), then AddSdchDictionary should return
false (dictionary not added), although there are no such problem codes. As we
discussed earlier, three options were available:
1) Add new problem codes (means messing up with histograms)
2) Create new enum to return from AddSdchDictionary. This enum would be a subset
of SdchProblems (~10 values) + 3 codes.
3) Return boolean meaning addition success/failure and problem as an addition to
it.
I decided to go with the last option.

On 2014/08/13 17:35:46, rdsmith wrote:
> Specify return value interpretation?  (And below.)
> 
> It's a bit weird that you're specifying one method (AddSdchDictionary) that
> returns a bool + writes a pointer with a problem code, but returning problem
> codes from all the rest of these routines.  Is there some reason you'd rather
> they didn't have the same signature pattern?

[baranovich, 2014-08-13 19:13:48]

https://codereview.chromium.org/423813002/diff/120001/net/base/net_log_source...
File net/base/net_log_source_type_list.h (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/net_log_source...
net/base/net_log_source_type_list.h:31: SOURCE_TYPE(SDCH_DICTIONARY_FETCHER)
On 2014/08/13 17:35:45, rdsmith wrote:
> Probably worth adding a TODO() that we want to remove this when we switch to
> URLRequest.

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:41: if (!fetch_queue_.empty() &&
fetch_queue_.back() == dictionary_url) {
I think it's done more or less by purpose, since |attempted_load_| contains
everything from the |fetch_queue_|, but the last one is just shorter and works
faster on.
On 2014/08/13 17:35:45, rdsmith wrote:
> Huh.  It looks to me like this line is in error; we should actually scan the
> entire queue, not just look at the last entry.  Willing to make the change
while
> you're here?

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:43: dictionary_url);
On 2014/08/13 17:35:45, rdsmith wrote:
> I'm not sure this counts as an error that should be flashing red on the event
> list.  Maybe plumb some way to log it s.t. the net error will be OK?

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:48: dictionary_url);
On 2014/08/13 17:35:45, rdsmith wrote:
> Similar; this could be the result of a race, and I wouldn't think should show
up
> as an error.

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:447: }
On 2014/08/13 17:35:45, rdsmith wrote:
> The extra braces aren't necessary by the chrome style guide, but I don't
object
> if you want them.

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:515: return PROBLEM_CODE_OK;  // not a problem. SDCH is
disabled.
Then we need some new enum values (smth like SDCH_DISABLED and
SDCH_HTTPS_DISABLED)
On 2014/08/13 17:35:45, rdsmith wrote:
> Hmmm.  Wouldn't we want to see this in net-internals?  (Though I don't think
it
> should happen.)

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:535: if (!IsInSupportedDomain(target_url)) {
On 2014/08/13 17:35:45, rdsmith wrote:
> I think it makes sense to avoid unnecessary changes in areas of the code where
> you're not doing other changes.  I could even make a claim that adding the
> braces goes against the intent of the style guide (which says to avoid
vertical
> whitespace if possible and that it's ok to skip braces on conditionals as long
> as the containing statement and the condition itself is each one line in
length,
> and the then and else clause (if existing) match in terms of being braced.

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:627: // Do not send error up. It's internal stuff, I
guess.
On 2014/08/13 17:35:45, rdsmith wrote:
> "I guess" doesn't seem like the right phrasing for a comment :-}; rephrase?   
> 
> You're welcome to surface this error or not as you wish; the latency
experiment
> is a random test where we hold back on 1 in 100 connections from using SDCH
and
> report UMA to compare the results, and the places I see where we disallow that
> experiment is when we blacklist domains.  
> 

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:26: #include <vector>
On 2014/08/13 17:35:46, rdsmith wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:34: #include "net/base/net_log.h"
On 2014/08/13 17:35:45, rdsmith wrote:
> Can't this be in the cc file?  Maybe with a forward declaration here, though
I'm
> not seeing the type use that would require it.

Done.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:39: class DictionaryValue;
On 2014/08/13 17:35:45, rdsmith wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/423813002/diff/120001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/filter/filter.cc#ne...
net/filter/filter.cc:174: success = filter_context.GetURL(&url);
On 2014/08/13 17:35:46, rdsmith wrote:
> Why the change?

Done.

[mmenke, 2014-08-15 18:35:11]

Sorry for slowness here, just one heavy review week after another.  The simplest
solution, in terms of complexity in production code, may be switching the
dictionary fetcher from using URLFetcher to URLRequest.  Unfortunately, that
would require a fair number of integration tests, to make sure we fair correctly
on auth and cert requests (And handle redirects correctly).

I'm having trouble coming up with reasonable alternatives.

[Randy Smith (Not in Mondays), 2014-08-19 19:00:09]

On 2014/08/13 18:37:16, baranovich wrote:
> IsInSupportedDomain is tricky: it checks 3 things actually: if SDCH enabled,
if
> SDCH-over-https enabled and blacklisting. First two are not really problems.
If
> the problem is about blacklist, the method calls SdchErrorRecovery. This
method
> was called from URLRequestHttpJob, from  SdchManager::GetAvailDictionaties,
from
> SdchManager::AddSdchDictionary() and from SdchManager::GetVcdiffDictionary.
The
> latter two now return ProblemCodes to be logged outside, so if they use
> IsInSupportedDomain as is, we will record histogram 2 times (or we may have
some
> ugly condition about it). First two call sites, on the other hand, should be
> silent about the problems (especially GetAvailDictionaries, which calls
> IsInSupportedDomain for each saved dictionary), but we still need to add the
> histograms (to behave like before) but I guess these warnings should not go
the
> the net-log.

Yeah, I've gone through IsInSupportedDomain() in detail, and I think the call to
SdchErrorRecovery() from IsInSupportedDomain() 
is basically broken.  We want that called once for each request, and we're
currently calling many more times 
than that (and in possibly related news, DOMAIN_BLACKLIST_INCLUDES_TARGET is by
*far* the most commonly 
reported error from the SdchErrorRecovery histogram).  However, not your
problem.  I may throw together 
a CL to fix it; if that happens before you land this one, I'd appreciate it if
you could look again at whether the refactoring of 
IsInSupportedDomain is necessary.  Otherwise, I will.

> 
> To my mind the interface of SdchManager becomes messy, SdchProblems enum was
not
> intended to be a return value, but changing this require much deeper
> refactoring.
> 
> >I want to go over the refactoring of
> >SdchErrorRecovery() and the split apart of IsInSupportedDomain() in more
> detail.

[Randy Smith (Not in Mondays), 2014-08-19 19:00:53]

I'm going to take a quick look at what would be involved in switching from
URLFetcher -> URLRequest; it might be simple, and if it is, I'll just do it.

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:105: LogSdchProblem(problem,
source->GetURL());
On 2014/08/13 18:38:58, baranovich wrote:
> PROBLEM_CODE_OK may be even if we haven't added the dictionary (if SDCH is
> disabled, for instance).
> 
> On 2014/08/13 17:35:45, rdsmith wrote:
> > I'm a bit uncomfortable with the implication of this call, which I read as
> that
> > true will be returned if-and-only-if the returned problem is
PROBLEM_CODE_OK. 
> > Why duplicate the information?  It seems like it's asking for skew down the
> > line.  
> > 
> > If you're not comfortable just returning a problem code, could you check the
> > boolean return here?
> 

As noted in my other comment, I think we're ok adding problem codes without
messing with the histogram, and I think that frees us up to be cleaner about API
specification, which is important to me.  My understanding of the primary reason
why we haven't just added new values for the conditions in which
AddSdchDictionary can not succeed is my comment about not changing the
histogram; if you agree with that, could you just add new values to cover those
cases?

(I'm partially guided in this by the fact that we don't ever *check* the return
value of AddSdchDictionary, which makes me think we're doing it wrong.)

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
net/base/sdch_manager.h:125: // given data, that arrived in response to get of
dictionary_url.
On 2014/08/13 18:38:58, baranovich wrote:
> If SDCH is disabled (or SDCH over https is disabled, or format-version != 1.0
> (but this should be a problem I believe)), then AddSdchDictionary should
return
> false (dictionary not added), although there are no such problem codes. As we
> discussed earlier, three options were available:
> 1) Add new problem codes (means messing up with histograms)
> 2) Create new enum to return from AddSdchDictionary. This enum would be a
subset
> of SdchProblems (~10 values) + 3 codes.
> 3) Return boolean meaning addition success/failure and problem as an addition
to
> it.
> I decided to go with the last option.

Right, thank you for the summary.  Thinking this through more carefully, I think
I'm good with adding new problem codes without messing with the histograms; as
long as we just add (and don't reuse old values, which the comments in the enum
should prevent) I think we're good keeping the same histogram.  And I think that
allows us to clean up the interface (i.e. make a general rule that, other than
the simplest boolean probes, every routine returns a problem code).

I think I'd like to make CanAdvertise consistent with the other Can* routines,
though.
 
> 
> On 2014/08/13 17:35:46, rdsmith wrote:
> > Specify return value interpretation?  (And below.)
> > 
> > It's a bit weird that you're specifying one method (AddSdchDictionary) that
> > returns a bool + writes a pointer with a problem code, but returning problem
> > codes from all the rest of these routines.  Is there some reason you'd
rather
> > they didn't have the same signature pattern?
>

[Randy Smith (Not in Mondays), 2014-08-20 18:06:21]

On 2014/08/19 19:00:53, rdsmith wrote:
> I'm going to take a quick look at what would be involved in switching from
> URLFetcher -> URLRequest; it might be simple, and if it is, I'll just do it.

It wasn't that big a deal, and is up for review at
https://codereview.chromium.org/495523003/.  Comments welcome, but I've tagged
someone else for primary review, so they aren't necessary.

> 
>
https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
> File net/base/sdch_dictionary_fetcher.cc (right):
> 
>
https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.cc:105: LogSdchProblem(problem,
> source->GetURL());
> On 2014/08/13 18:38:58, baranovich wrote:
> > PROBLEM_CODE_OK may be even if we haven't added the dictionary (if SDCH is
> > disabled, for instance).
> > 
> > On 2014/08/13 17:35:45, rdsmith wrote:
> > > I'm a bit uncomfortable with the implication of this call, which I read as
> > that
> > > true will be returned if-and-only-if the returned problem is
> PROBLEM_CODE_OK. 
> > > Why duplicate the information?  It seems like it's asking for skew down
the
> > > line.  
> > > 
> > > If you're not comfortable just returning a problem code, could you check
the
> > > boolean return here?
> > 
> 
> As noted in my other comment, I think we're ok adding problem codes without
> messing with the histogram, and I think that frees us up to be cleaner about
API
> specification, which is important to me.  My understanding of the primary
reason
> why we haven't just added new values for the conditions in which
> AddSdchDictionary can not succeed is my comment about not changing the
> histogram; if you agree with that, could you just add new values to cover
those
> cases?
> 
> (I'm partially guided in this by the fact that we don't ever *check* the
return
> value of AddSdchDictionary, which makes me think we're doing it wrong.)
> 
> https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h
> File net/base/sdch_manager.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/120001/net/base/sdch_manager.h...
> net/base/sdch_manager.h:125: // given data, that arrived in response to get of
> dictionary_url.
> On 2014/08/13 18:38:58, baranovich wrote:
> > If SDCH is disabled (or SDCH over https is disabled, or format-version !=
1.0
> > (but this should be a problem I believe)), then AddSdchDictionary should
> return
> > false (dictionary not added), although there are no such problem codes. As
we
> > discussed earlier, three options were available:
> > 1) Add new problem codes (means messing up with histograms)
> > 2) Create new enum to return from AddSdchDictionary. This enum would be a
> subset
> > of SdchProblems (~10 values) + 3 codes.
> > 3) Return boolean meaning addition success/failure and problem as an
addition
> to
> > it.
> > I decided to go with the last option.
> 
> Right, thank you for the summary.  Thinking this through more carefully, I
think
> I'm good with adding new problem codes without messing with the histograms; as
> long as we just add (and don't reuse old values, which the comments in the
enum
> should prevent) I think we're good keeping the same histogram.  And I think
that
> allows us to clean up the interface (i.e. make a general rule that, other than
> the simplest boolean probes, every routine returns a problem code).
> 
> I think I'd like to make CanAdvertise consistent with the other Can* routines,
> though.
>  
> > 
> > On 2014/08/13 17:35:46, rdsmith wrote:
> > > Specify return value interpretation?  (And below.)
> > > 
> > > It's a bit weird that you're specifying one method (AddSdchDictionary)
that
> > > returns a bool + writes a pointer with a problem code, but returning
problem
> > > codes from all the rest of these routines.  Is there some reason you'd
> rather
> > > they didn't have the same signature pattern?
> >

[baranovich, 2014-09-01 21:01:35]

Hi Randy,
I've rebased this CL to the latest trunk, and made AddSdchDictionary to return
everything as a problem code. I've added more problem codes behind
PROBLEM_CODE_MAX to preserve histogram max value. Also, I've reverted
IsInSupportedDomain refactoring and made it to return ProblemCodes as well. Now
SdchErrorRecovery is called from UrlRequestHttpJob for
DOMAIN_BLACKLIST_INCLUDES_TARGET errors only once. However,
Sdch3.BlacklistReason works the old way, and I don't see how to fix it easy.

[baranovich, 2014-09-01 21:03:17]

So now I wait for https://codereview.chromium.org/495523003 to land to make the
final approach to all that:)

[Randy Smith (Not in Mondays), 2014-09-02 14:17:07]

On 2014/09/01 21:03:17, baranovich wrote:
> So now I wait for https://codereview.chromium.org/495523003 to land to make
the
> final approach to all that:)

Yeah, my apologies; that CL has been subject to mission creep and careful
reviewing, as sometimes happens :-J.  If you want to keep making progress on
this CL, you can patach that one into a branch in your repository, merge it into
what you're doing, and mark this CL as dependent on that one.  Or you can
wait--I'm good either way, I just feel guilty about blocking you.

[mmenke, 2014-09-02 14:33:02]

On 2014/09/02 14:17:07, rdsmith wrote:
> On 2014/09/01 21:03:17, baranovich wrote:
> > So now I wait for https://codereview.chromium.org/495523003 to land to make
> the
> > final approach to all that:)
> 
> Yeah, my apologies; that CL has been subject to mission creep and careful
> reviewing, as sometimes happens :-J.  If you want to keep making progress on
> this CL, you can patach that one into a branch in your repository, merge it
into
> what you're doing, and mark this CL as dependent on that one.  Or you can
> wait--I'm good either way, I just feel guilty about blocking you.

Just posting a message here (without sending mail) so this CL will no longer be
bold on my list.  Have 10 from the long weekend, and hard to keep them straight.

[baranovich, 2014-09-02 16:58:40]

On 2014/09/02 14:17:07, rdsmith wrote:
> On 2014/09/01 21:03:17, baranovich wrote:
> > So now I wait for https://codereview.chromium.org/495523003 to land to make
> the
> > final approach to all that:)
> 
> Yeah, my apologies; that CL has been subject to mission creep and careful
> reviewing, as sometimes happens :-J.  If you want to keep making progress on
> this CL, you can patach that one into a branch in your repository, merge it
into
> what you're doing, and mark this CL as dependent on that one.  Or you can
> wait--I'm good either way, I just feel guilty about blocking you.

No problem, I'll just wait:) Feel free to work on that CL as long as it's
required.

[Randy Smith (Not in Mondays), 2014-09-10 16:35:15]

On 2014/09/02 16:58:40, baranovich wrote:
> On 2014/09/02 14:17:07, rdsmith wrote:
> > On 2014/09/01 21:03:17, baranovich wrote:
> > > So now I wait for https://codereview.chromium.org/495523003 to land to
make
> > the
> > > final approach to all that:)
> > 
> > Yeah, my apologies; that CL has been subject to mission creep and careful
> > reviewing, as sometimes happens :-J.  If you want to keep making progress on
> > this CL, you can patach that one into a branch in your repository, merge it
> into
> > what you're doing, and mark this CL as dependent on that one.  Or you can
> > wait--I'm good either way, I just feel guilty about blocking you.
> 
> No problem, I'll just wait:) Feel free to work on that CL as long as it's
> required.

The URLFetcher->URLRequest change has landed at commit
1df5b71300297a83efb39b8666ba0ff44369947c (pseudo-revision # p294002).  Whew.  I
*thought* it would be a simple change :-}.

[Randy Smith (Not in Mondays), 2014-09-10 16:36:16]

On 2014/09/10 16:35:15, rdsmith wrote:
> On 2014/09/02 16:58:40, baranovich wrote:
> > On 2014/09/02 14:17:07, rdsmith wrote:
> > > On 2014/09/01 21:03:17, baranovich wrote:
> > > > So now I wait for https://codereview.chromium.org/495523003 to land to
> make
> > > the
> > > > final approach to all that:)
> > > 
> > > Yeah, my apologies; that CL has been subject to mission creep and careful
> > > reviewing, as sometimes happens :-J.  If you want to keep making progress
on
> > > this CL, you can patach that one into a branch in your repository, merge
it
> > into
> > > what you're doing, and mark this CL as dependent on that one.  Or you can
> > > wait--I'm good either way, I just feel guilty about blocking you.
> > 
> > No problem, I'll just wait:) Feel free to work on that CL as long as it's
> > required.
> 
> The URLFetcher->URLRequest change has landed at commit
> 1df5b71300297a83efb39b8666ba0ff44369947c (pseudo-revision # p294002).  Whew. 
I
> *thought* it would be a simple change :-}.

And to be explicit; I'm currently planning to delay my next review until this CL
is rebased/merged on top of that one and the SdchDictionaryFetcher logging
switched over to the URLRequest.  Let me know if you'd like me to change that
plan.

[baranovich, 2014-09-17 18:53:38]

On 2014/09/10 16:36:16, rdsmith wrote:
> On 2014/09/10 16:35:15, rdsmith wrote:
> > On 2014/09/02 16:58:40, baranovich wrote:
> > > On 2014/09/02 14:17:07, rdsmith wrote:
> > > > On 2014/09/01 21:03:17, baranovich wrote:
> > > > > So now I wait for https://codereview.chromium.org/495523003 to land to
> > make
> > > > the
> > > > > final approach to all that:)
> > > > 
> > > > Yeah, my apologies; that CL has been subject to mission creep and
careful
> > > > reviewing, as sometimes happens :-J.  If you want to keep making
progress
> on
> > > > this CL, you can patach that one into a branch in your repository, merge
> it
> > > into
> > > > what you're doing, and mark this CL as dependent on that one.  Or you
can
> > > > wait--I'm good either way, I just feel guilty about blocking you.
> > > 
> > > No problem, I'll just wait:) Feel free to work on that CL as long as it's
> > > required.
> > 
> > The URLFetcher->URLRequest change has landed at commit
> > 1df5b71300297a83efb39b8666ba0ff44369947c (pseudo-revision # p294002).  Whew.

> I
> > *thought* it would be a simple change :-}.
> 
> And to be explicit; I'm currently planning to delay my next review until this
CL
> is rebased/merged on top of that one and the SdchDictionaryFetcher logging
> switched over to the URLRequest.  Let me know if you'd like me to change that
> plan.

I've rebased the CL on new Fetcher...
some questions left:
1) I've removed expiration date from the table, since I'm not sure how to
display it. Its base::Time, and all js helpers work with base::TimeTicks. I
don't know if it's possible to convert base::Time to it. Also I cannot use
base/i18n inside SdchManager (deps). I could either move JSON creation to
net_internals_ui.cc (means adding a bunch of methods to SdchManager or change
Dictionary::expiration to be TimeTicks.
2) From my tests, URLRequests are painted red quite often for SDCH. I've tried
to address it in SdchFilter (by looking into if SDCH is tentative) but still
FixupEncodingTypes will paint the request red if it adds the filters which
actually may not be required, since the server serves non-SDCH content on
purpose (for instance, opensearch.xml).

[mmenke, 2014-09-18 15:45:02]

On 2014/09/17 18:53:38, baranovich wrote:
> On 2014/09/10 16:36:16, rdsmith wrote:
> > On 2014/09/10 16:35:15, rdsmith wrote:
> > > On 2014/09/02 16:58:40, baranovich wrote:
> > > > On 2014/09/02 14:17:07, rdsmith wrote:
> > > > > On 2014/09/01 21:03:17, baranovich wrote:
> > > > > > So now I wait for https://codereview.chromium.org/495523003 to land
to
> > > make
> > > > > the
> > > > > > final approach to all that:)
> > > > > 
> > > > > Yeah, my apologies; that CL has been subject to mission creep and
> careful
> > > > > reviewing, as sometimes happens :-J.  If you want to keep making
> progress
> > on
> > > > > this CL, you can patach that one into a branch in your repository,
merge
> > it
> > > > into
> > > > > what you're doing, and mark this CL as dependent on that one.  Or you
> can
> > > > > wait--I'm good either way, I just feel guilty about blocking you.
> > > > 
> > > > No problem, I'll just wait:) Feel free to work on that CL as long as
it's
> > > > required.
> > > 
> > > The URLFetcher->URLRequest change has landed at commit
> > > 1df5b71300297a83efb39b8666ba0ff44369947c (pseudo-revision # p294002). 
Whew.
> 
> > I
> > > *thought* it would be a simple change :-}.
> > 
> > And to be explicit; I'm currently planning to delay my next review until
this
> CL
> > is rebased/merged on top of that one and the SdchDictionaryFetcher logging
> > switched over to the URLRequest.  Let me know if you'd like me to change
that
> > plan.
> 
> I've rebased the CL on new Fetcher...
> some questions left:
> 1) I've removed expiration date from the table, since I'm not sure how to
> display it. Its base::Time, and all js helpers work with base::TimeTicks. I
> don't know if it's possible to convert base::Time to it. Also I cannot use
> base/i18n inside SdchManager (deps). I could either move JSON creation to
> net_internals_ui.cc (means adding a bunch of methods to SdchManager or change
> Dictionary::expiration to be TimeTicks.
> 2) From my tests, URLRequests are painted red quite often for SDCH. I've tried
> to address it in SdchFilter (by looking into if SDCH is tentative) but still
> FixupEncodingTypes will paint the request red if it adds the filters which
> actually may not be required, since the server serves non-SDCH content on
> purpose (for instance, opensearch.xml).

Randy:  Mind taking lead on this review?  I don't have a whole lot of time.

[Randy Smith (Not in Mondays), 2014-09-18 18:59:43]

On 2014/09/18 15:45:02, mmenke wrote:
> On 2014/09/17 18:53:38, baranovich wrote:
> > On 2014/09/10 16:36:16, rdsmith wrote:
> > > On 2014/09/10 16:35:15, rdsmith wrote:
> > > > On 2014/09/02 16:58:40, baranovich wrote:
> > > > > On 2014/09/02 14:17:07, rdsmith wrote:
> > > > > > On 2014/09/01 21:03:17, baranovich wrote:
> > > > > > > So now I wait for https://codereview.chromium.org/495523003 to
land
> to
> > > > make
> > > > > > the
> > > > > > > final approach to all that:)
> > > > > > 
> > > > > > Yeah, my apologies; that CL has been subject to mission creep and
> > careful
> > > > > > reviewing, as sometimes happens :-J.  If you want to keep making
> > progress
> > > on
> > > > > > this CL, you can patach that one into a branch in your repository,
> merge
> > > it
> > > > > into
> > > > > > what you're doing, and mark this CL as dependent on that one.  Or
you
> > can
> > > > > > wait--I'm good either way, I just feel guilty about blocking you.
> > > > > 
> > > > > No problem, I'll just wait:) Feel free to work on that CL as long as
> it's
> > > > > required.
> > > > 
> > > > The URLFetcher->URLRequest change has landed at commit
> > > > 1df5b71300297a83efb39b8666ba0ff44369947c (pseudo-revision # p294002). 
> Whew.
> > 
> > > I
> > > > *thought* it would be a simple change :-}.
> > > 
> > > And to be explicit; I'm currently planning to delay my next review until
> this
> > CL
> > > is rebased/merged on top of that one and the SdchDictionaryFetcher logging
> > > switched over to the URLRequest.  Let me know if you'd like me to change
> that
> > > plan.
> > 
> > I've rebased the CL on new Fetcher...
> > some questions left:
> > 1) I've removed expiration date from the table, since I'm not sure how to
> > display it. Its base::Time, and all js helpers work with base::TimeTicks. I
> > don't know if it's possible to convert base::Time to it. Also I cannot use
> > base/i18n inside SdchManager (deps). I could either move JSON creation to
> > net_internals_ui.cc (means adding a bunch of methods to SdchManager or
change
> > Dictionary::expiration to be TimeTicks.
> > 2) From my tests, URLRequests are painted red quite often for SDCH. I've
tried
> > to address it in SdchFilter (by looking into if SDCH is tentative) but still
> > FixupEncodingTypes will paint the request red if it adds the filters which
> > actually may not be required, since the server serves non-SDCH content on
> > purpose (for instance, opensearch.xml).
> 
> Randy:  Mind taking lead on this review?  I don't have a whole lot of time.

Sure, that's fine (I'm assuming you'll do a later review after I've, hopefully,
hammered out the big picture stuff).  When you have a chance, give me a summary
of the specific issues you'd like me to look for in the net-internals code?

@baranovich: I'm going to try to do a round on this today, but after that I
won't get back to it until Monday (and will be rate limiting according to other
stuff I need to get done; there are a couple of priorities I'm juggling).  I'd
say let me know if I'm not responsive enough and we'll do something about it,
but I'm not completely certain I'm sure what we'll do; Matt and I are pretty
much the only appropriate reviewers for this section of the code.  But let me
know anyway :-}.

[mmenke, 2014-09-18 19:11:02]

On 2014/09/18 18:59:43, rdsmith wrote:
> On 2014/09/18 15:45:02, mmenke wrote:
> > On 2014/09/17 18:53:38, baranovich wrote:
> > > On 2014/09/10 16:36:16, rdsmith wrote:
> > > > On 2014/09/10 16:35:15, rdsmith wrote:
> > > > > On 2014/09/02 16:58:40, baranovich wrote:
> > > > > > On 2014/09/02 14:17:07, rdsmith wrote:
> > > > > > > On 2014/09/01 21:03:17, baranovich wrote:
> > > > > > > > So now I wait for https://codereview.chromium.org/495523003 to
> land
> > to
> > > > > make
> > > > > > > the
> > > > > > > > final approach to all that:)
> > > > > > > 
> > > > > > > Yeah, my apologies; that CL has been subject to mission creep and
> > > careful
> > > > > > > reviewing, as sometimes happens :-J.  If you want to keep making
> > > progress
> > > > on
> > > > > > > this CL, you can patach that one into a branch in your repository,
> > merge
> > > > it
> > > > > > into
> > > > > > > what you're doing, and mark this CL as dependent on that one.  Or
> you
> > > can
> > > > > > > wait--I'm good either way, I just feel guilty about blocking you.
> > > > > > 
> > > > > > No problem, I'll just wait:) Feel free to work on that CL as long as
> > it's
> > > > > > required.
> > > > > 
> > > > > The URLFetcher->URLRequest change has landed at commit
> > > > > 1df5b71300297a83efb39b8666ba0ff44369947c (pseudo-revision # p294002). 
> > Whew.
> > > 
> > > > I
> > > > > *thought* it would be a simple change :-}.
> > > > 
> > > > And to be explicit; I'm currently planning to delay my next review until
> > this
> > > CL
> > > > is rebased/merged on top of that one and the SdchDictionaryFetcher
logging
> > > > switched over to the URLRequest.  Let me know if you'd like me to change
> > that
> > > > plan.
> > > 
> > > I've rebased the CL on new Fetcher...
> > > some questions left:
> > > 1) I've removed expiration date from the table, since I'm not sure how to
> > > display it. Its base::Time, and all js helpers work with base::TimeTicks.
I
> > > don't know if it's possible to convert base::Time to it. Also I cannot use
> > > base/i18n inside SdchManager (deps). I could either move JSON creation to
> > > net_internals_ui.cc (means adding a bunch of methods to SdchManager or
> change
> > > Dictionary::expiration to be TimeTicks.
> > > 2) From my tests, URLRequests are painted red quite often for SDCH. I've
> tried
> > > to address it in SdchFilter (by looking into if SDCH is tentative) but
still
> > > FixupEncodingTypes will paint the request red if it adds the filters which
> > > actually may not be required, since the server serves non-SDCH content on
> > > purpose (for instance, opensearch.xml).
> > 
> > Randy:  Mind taking lead on this review?  I don't have a whole lot of time.
> 
> Sure, that's fine (I'm assuming you'll do a later review after I've,
hopefully,
> hammered out the big picture stuff).  When you have a chance, give me a
summary
> of the specific issues you'd like me to look for in the net-internals code?

Yea, I'll definitely take a look.

My two main concerns from a net-internals standpoint are the ability to load
logs (Just receiving all data through a pollable data helper should ensure
that), and compatibility between versions when loading logs, both forward and
backward.  Occasionally we have to break things, but best to avoid it if
possible.  I'll take a careful look over the net-internals stuff, in particular,
when you're happy.

> @baranovich: I'm going to try to do a round on this today, but after that I
> won't get back to it until Monday (and will be rate limiting according to
other
> stuff I need to get done; there are a couple of priorities I'm juggling).  I'd
> say let me know if I'm not responsive enough and we'll do something about it,
> but I'm not completely certain I'm sure what we'll do; Matt and I are pretty
> much the only appropriate reviewers for this section of the code.  But let me
> know anyway :-}.

[Randy Smith (Not in Mondays), 2014-09-18 20:55:51]

Initial re-review.  There are a couple of things I want to come back to after
I've thought about them:
* AddSdchDictionary() taking a BoundNetLog.  I understand why you did it (and
its my fault for getting rid of the return value from AddSdchDictionary) but it
violates the abstraction boundary we decided on earlier.
* The net-internals code.  I'm not as familiar with that, so I'll want to do a
review from scratch on it.

I'll take another swing on Monday.  Thanks for your patience.

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list...
net/base/net_error_list.h:666: NET_ERROR(SDCH_PROBLEM, -504)
I don't think this is used anymore; remove?

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:346:
SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
Just to summarize what I think the refactor of this routine did in terms of
calls to SdchManager::SdchErrorRecovery(), in case I missed something: It looks
like you removed this call here, return problem codes but don't log them in the
callers, *except* that in
URLRequestHttpJob::{NotifyHeadersComplete,AddExtraHeaders} you log only
DOMAIN_BLACKLIST_INCLUDES_TARGET.

So I'm struggling with what I'd like you to do here.  The existing code is a
mess, for several different reasons.  However, I feel as if, if we change the
places in the code that file UMA for DOMAIN_BLACKLIST_INCLUDES_TARGET, we need
to rev the histogram with all the hassle that involves (or, to be fair,
deprecate this constant and create a new one that means something different).  I
think this change (and the changes in the callers) does indeed change the
frequency and context in which this UMA is logged (since it's no longer logged
in the GetVcdiffDictionary/GetAvailDictionary/AddSdchDictionary cases.

I don't want to fix the histogram or deprecate the constant until I'm clear on
what the right thing to do is, and I don't want to do it multiple times.  And
I'd also like to keep a 1-1 correspondence between a logging of
BLACKLIST_INCLUDES_TARGET and an entry in Sdch3.BlacklistReason.  

So what I'd like to suggest is that you put the SdchErrorRecovery() call back
here, and remove the UMA logging from URLRequestHttpJob::* (no change needed to
the net log logging; you're creating that, so there's no consistency issues). 
My I get my proverbial round tuit, I'll kill BLACKLIST_INCLUDES_TARGET with
fire, but until then we'll keep it in its current form.

Please let me know if you see any problems with that plan.  My apologies if I've
asked for different behavior in the past; I skimmed previous comments to see if
I could figure out what guidance I've previously given and didn't find any, but
given the history of this CL, I might have missed a discussion.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:653: // static
Why not make this a file static or put it in an anonymous namespace in this
file?  It's not called from outside this file, so making the visibility as
narrow as possible seems a good thing.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h...
net/base/sdch_manager.h:51: enum ScheduleResult {
So this (the enum, switching Schedule() to return the enum, and transforming the
enum into an SdchManager::ProblemCode at point of call) is a correct
transformation of the current code, but I don't think it's that useful from a
debugability point of view, and it's a lot of ugly boilerplate in several
places, so I'd like to skip it.  Part of the reason I don't think it's useful
from a debugability point of view is that I don't see a real distinction between
ALREADY_TRIED and ALREADY_SCHEDULED in SdchDictionaryFetcher::Schedule; they
both mean "yeah, I already took a shot at that dictionary, for better or worse"
(i.e. I think the existing code is broken).  

So I think I'd like to ask you to do a slight functionality change in this CL to
make the debugging implementation cleaner: Could you obsolete
DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD, and
DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD and replace them with a new ProblemCode
(PREVIOUSLY_SCHEDULED?), and change Schedule() to return a bool indicating
whether the load was successfully scheduled, and turn a false return from that
function into the new ProblemCode?  That'll remove a lot of boilerplate.

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc#ne...
net/filter/filter.cc:19: 
I don't particularly care, but was there a reason for the shift to nested
namespaces?

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h#new...
net/filter/filter.h:41: #include "net/base/sdch_manager.h"
I don't see this used anywhere in this file; remove it?

https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:455: // errors.
Suggestion: Move this comment to right above the !possible_pass_through_; on my
first read through, I wasn't sure what you were referring to.

https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:460: !possible_pass_through_));
My read of Filter::FixupEncodingTypes() is that we won't use use
FILTER_TYPE_SDCH_POSSIBLE unless we expected an SDCH response (because we
advertised a dictionary and the server didn't say X-SDCH-Encode: 0).  In that
case, I think it's reasonable to call out problems as errors, though it's
probably worthwhile checking with Matt on what level of thing he'd like to be
seen as an "error" in net-internals.

[baranovich, 2014-09-19 12:42:45]

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list...
net/base/net_error_list.h:666: NET_ERROR(SDCH_PROBLEM, -504)
On 2014/09/18 20:55:51, rdsmith wrote:
> I don't think this is used anymore; remove?

No, it is still used by net-internals code. I need some error code to signal ui
to paint URL_REQUEST red in event log.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:346:
SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
Got your point, although GetVcdiffDictionary/AddSdchDictionary logs
DOMAIN_BLACKLIST_INCLUDES_TARGET (for GetVcdiffDictionary SdchFilter does this,
AddSdchDictionary logs everything inside it). but there are error in how they do
it now (should filter out SDCH_DISABLED and SECURE_SCHEME_NOT_SUPPORTED from
histograms). So what I've done is moving logging out of this function.
That implicitly removes SdchErrorRecovery() call from GetAvailDictionary. But
you were saying that this call is badly broken and histogram is a mess because
of it, since it was intended to have a single DOMAIN_BLACKLIST_INCLUDES_TARGET
(from the call to IsInSupportedDomain, I guess) record when URLRequestHttpJob
tries to get the dicts instead of zillions of them when GetAvailDictionary lists
all the dicts. So I decided to skip this, because the code looks much simpler
that way.

But I could make all the histograms be reported as-is with just a little more
hacks:) Introducing another histogram later is indeed a better dicision.

On 2014/09/18 20:55:51, rdsmith wrote:
> Just to summarize what I think the refactor of this routine did in terms of
> calls to SdchManager::SdchErrorRecovery(), in case I missed something: It
looks
> like you removed this call here, return problem codes but don't log them in
the
> callers, *except* that in
> URLRequestHttpJob::{NotifyHeadersComplete,AddExtraHeaders} you log only
> DOMAIN_BLACKLIST_INCLUDES_TARGET.
> 
> So I'm struggling with what I'd like you to do here.  The existing code is a
> mess, for several different reasons.  However, I feel as if, if we change the
> places in the code that file UMA for DOMAIN_BLACKLIST_INCLUDES_TARGET, we need
> to rev the histogram with all the hassle that involves (or, to be fair,
> deprecate this constant and create a new one that means something different). 
I
> think this change (and the changes in the callers) does indeed change the
> frequency and context in which this UMA is logged (since it's no longer logged
> in the GetVcdiffDictionary/GetAvailDictionary/AddSdchDictionary cases.
> 
> I don't want to fix the histogram or deprecate the constant until I'm clear on
> what the right thing to do is, and I don't want to do it multiple times.  And
> I'd also like to keep a 1-1 correspondence between a logging of
> BLACKLIST_INCLUDES_TARGET and an entry in Sdch3.BlacklistReason.  
> 
> So what I'd like to suggest is that you put the SdchErrorRecovery() call back
> here, and remove the UMA logging from URLRequestHttpJob::* (no change needed
to
> the net log logging; you're creating that, so there's no consistency issues). 
> My I get my proverbial round tuit, I'll kill BLACKLIST_INCLUDES_TARGET with
> fire, but until then we'll keep it in its current form.
> 
> Please let me know if you see any problems with that plan.  My apologies if
I've
> asked for different behavior in the past; I skimmed previous comments to see
if
> I could figure out what guidance I've previously given and didn't find any,
but
> given the history of this CL, I might have missed a discussion.
> 
>

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h...
net/base/sdch_manager.h:51: enum ScheduleResult {
On 2014/09/18 20:55:51, rdsmith wrote:
> So this (the enum, switching Schedule() to return the enum, and transforming
the
> enum into an SdchManager::ProblemCode at point of call) is a correct
> transformation of the current code, but I don't think it's that useful from a
> debugability point of view, and it's a lot of ugly boilerplate in several
> places, so I'd like to skip it.  Part of the reason I don't think it's useful
> from a debugability point of view is that I don't see a real distinction
between
> ALREADY_TRIED and ALREADY_SCHEDULED in SdchDictionaryFetcher::Schedule; they
> both mean "yeah, I already took a shot at that dictionary, for better or
worse"
> (i.e. I think the existing code is broken).  
> 
> So I think I'd like to ask you to do a slight functionality change in this CL
to
> make the debugging implementation cleaner: Could you obsolete
> DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD, and
> DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD and replace them with a new ProblemCode
> (PREVIOUSLY_SCHEDULED?), and change Schedule() to return a bool indicating
> whether the load was successfully scheduled, and turn a false return from that
> function into the new ProblemCode?  That'll remove a lot of boilerplate.

Sure, I'll try.

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc
File net/filter/filter.cc (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc#ne...
net/filter/filter.cc:19: 
On 2014/09/18 20:55:51, rdsmith wrote:
> I don't particularly care, but was there a reason for the shift to nested
> namespaces?

I wanted to avoid writing net:: prefixes in LogSdchProblem.
PS sometimes I think that (nesting anon namespaces inside of a net/content/etc)
would be a good point for the Chromium styleguide:)

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h#new...
net/filter/filter.h:41: #include "net/base/sdch_manager.h"
On 2014/09/18 20:55:51, rdsmith wrote:
> I don't see this used anywhere in this file; remove it?

My bad

[Randy Smith (Not in Mondays), 2014-09-23 18:11:11]

On 2014/09/19 12:42:45, baranovich wrote:
>
https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list.h
> File net/base/net_error_list.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list...
> net/base/net_error_list.h:666: NET_ERROR(SDCH_PROBLEM, -504)
> On 2014/09/18 20:55:51, rdsmith wrote:
> > I don't think this is used anymore; remove?
> 
> No, it is still used by net-internals code. I need some error code to signal
ui
> to paint URL_REQUEST red in event log.
> 
> https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (left):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
> net/base/sdch_manager.cc:346:
> SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
> Got your point, although GetVcdiffDictionary/AddSdchDictionary logs
> DOMAIN_BLACKLIST_INCLUDES_TARGET (for GetVcdiffDictionary SdchFilter does
this,
> AddSdchDictionary logs everything inside it). but there are error in how they
do
> it now (should filter out SDCH_DISABLED and SECURE_SCHEME_NOT_SUPPORTED from
> histograms). So what I've done is moving logging out of this function.
> That implicitly removes SdchErrorRecovery() call from GetAvailDictionary. But
> you were saying that this call is badly broken and histogram is a mess because
> of it, since it was intended to have a single DOMAIN_BLACKLIST_INCLUDES_TARGET
> (from the call to IsInSupportedDomain, I guess) record when URLRequestHttpJob
> tries to get the dicts instead of zillions of them when GetAvailDictionary
lists
> all the dicts. So I decided to skip this, because the code looks much simpler
> that way.
> 
> But I could make all the histograms be reported as-is with just a little more
> hacks:) Introducing another histogram later is indeed a better dicision.
> 
> On 2014/09/18 20:55:51, rdsmith wrote:
> > Just to summarize what I think the refactor of this routine did in terms of
> > calls to SdchManager::SdchErrorRecovery(), in case I missed something: It
> looks
> > like you removed this call here, return problem codes but don't log them in
> the
> > callers, *except* that in
> > URLRequestHttpJob::{NotifyHeadersComplete,AddExtraHeaders} you log only
> > DOMAIN_BLACKLIST_INCLUDES_TARGET.
> > 
> > So I'm struggling with what I'd like you to do here.  The existing code is a
> > mess, for several different reasons.  However, I feel as if, if we change
the
> > places in the code that file UMA for DOMAIN_BLACKLIST_INCLUDES_TARGET, we
need
> > to rev the histogram with all the hassle that involves (or, to be fair,
> > deprecate this constant and create a new one that means something
different). 
> I
> > think this change (and the changes in the callers) does indeed change the
> > frequency and context in which this UMA is logged (since it's no longer
logged
> > in the GetVcdiffDictionary/GetAvailDictionary/AddSdchDictionary cases.
> > 
> > I don't want to fix the histogram or deprecate the constant until I'm clear
on
> > what the right thing to do is, and I don't want to do it multiple times. 
And
> > I'd also like to keep a 1-1 correspondence between a logging of
> > BLACKLIST_INCLUDES_TARGET and an entry in Sdch3.BlacklistReason.  
> > 
> > So what I'd like to suggest is that you put the SdchErrorRecovery() call
back
> > here, and remove the UMA logging from URLRequestHttpJob::* (no change needed
> to
> > the net log logging; you're creating that, so there's no consistency
issues). 
> > My I get my proverbial round tuit, I'll kill BLACKLIST_INCLUDES_TARGET with
> > fire, but until then we'll keep it in its current form.
> > 
> > Please let me know if you see any problems with that plan.  My apologies if
> I've
> > asked for different behavior in the past; I skimmed previous comments to see
> if
> > I could figure out what guidance I've previously given and didn't find any,
> but
> > given the history of this CL, I might have missed a discussion.
> > 
> >
> 
> https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h
> File net/base/sdch_manager.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h...
> net/base/sdch_manager.h:51: enum ScheduleResult {
> On 2014/09/18 20:55:51, rdsmith wrote:
> > So this (the enum, switching Schedule() to return the enum, and transforming
> the
> > enum into an SdchManager::ProblemCode at point of call) is a correct
> > transformation of the current code, but I don't think it's that useful from
a
> > debugability point of view, and it's a lot of ugly boilerplate in several
> > places, so I'd like to skip it.  Part of the reason I don't think it's
useful
> > from a debugability point of view is that I don't see a real distinction
> between
> > ALREADY_TRIED and ALREADY_SCHEDULED in SdchDictionaryFetcher::Schedule; they
> > both mean "yeah, I already took a shot at that dictionary, for better or
> worse"
> > (i.e. I think the existing code is broken).  
> > 
> > So I think I'd like to ask you to do a slight functionality change in this
CL
> to
> > make the debugging implementation cleaner: Could you obsolete
> > DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD, and
> > DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD and replace them with a new ProblemCode
> > (PREVIOUSLY_SCHEDULED?), and change Schedule() to return a bool indicating
> > whether the load was successfully scheduled, and turn a false return from
that
> > function into the new ProblemCode?  That'll remove a lot of boilerplate.
> 
> Sure, I'll try.
> 
> https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc
> File net/filter/filter.cc (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc#ne...
> net/filter/filter.cc:19: 
> On 2014/09/18 20:55:51, rdsmith wrote:
> > I don't particularly care, but was there a reason for the shift to nested
> > namespaces?
> 
> I wanted to avoid writing net:: prefixes in LogSdchProblem.
> PS sometimes I think that (nesting anon namespaces inside of a
net/content/etc)
> would be a good point for the Chromium styleguide:)
> 
> https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h
> File net/filter/filter.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h#new...
> net/filter/filter.h:41: #include "net/base/sdch_manager.h"
> On 2014/09/18 20:55:51, rdsmith wrote:
> > I don't see this used anywhere in this file; remove it?
> 
> My bad

My apologies for not getting back to you--I'm hoping to do another round of
thorough review tomorrow.  In the meantime: What commit on trunk are you
branched from?  I'm not getting the CL to patch cleanly in my checkout.

[baranovich, 2014-09-23 18:57:29]

Now it's based on that, but I'll update it

commit 952a6ad2c8f23d103aac0114dc15e4de8d42db92
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date:   Tue Sep 16 06:50:34 2014 -0700

    Skia roll 49005bf:9564ce6
    
   
https://skia.googlesource.com/skia/+log/49005bf8929dd8ca86431e13414d683b509cd...
    
    CQ_EXTRA_TRYBOTS=tryserver.blink:linux_blink_rel,linux_blink_dbg
    TBR=stephana@google.com
    
    Review URL: https://codereview.chromium.org/578553003
    
    Cr-Commit-Position: refs/heads/master@{#295058}

[Randy Smith (Not in Mondays), 2014-09-25 18:21:39]

On 2014/09/18 20:55:51, rdsmith wrote:
> Initial re-review.  There are a couple of things I want to come back to after
> I've thought about them:
> * AddSdchDictionary() taking a BoundNetLog.  I understand why you did it (and
> its my fault for getting rid of the return value from AddSdchDictionary) but
it
> violates the abstraction boundary we decided on earlier.

I've let this sit in the back of my mind, and I really don't see a better answer
than the first thing I thought of (which I'm not very happy with).  In case you
can think of something better: My goals are to keep the SdchManager net-log free
and not create any dependencies from SdchFetcher subclasses on SdchManager
(requirement from one of my reviewers in the CL in which I got rid of
URLFetcher).  So what I'd like you to do, if you're willing:
* Pull ProblemCodes out of SdchManager.  This probably requires adding a prefix
to all the enum codes, since it'll then be in the global net: namespace.  
* Make SdchFetcher::AddSdchDictionary() return a ProblemCodes (sic).  
* Pull the logging around success/failure of AddSdchDictionary() in the calling
sites.

Moving on to do the net-internals review....

> * The net-internals code.  I'm not as familiar with that, so I'll want to do a
> review from scratch on it.
> 
> I'll take another swing on Monday.  Thanks for your patience.
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list.h
> File net/base/net_error_list.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list...
> net/base/net_error_list.h:666: NET_ERROR(SDCH_PROBLEM, -504)
> I don't think this is used anymore; remove?
> 
> https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (left):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
> net/base/sdch_manager.cc:346:
> SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
> Just to summarize what I think the refactor of this routine did in terms of
> calls to SdchManager::SdchErrorRecovery(), in case I missed something: It
looks
> like you removed this call here, return problem codes but don't log them in
the
> callers, *except* that in
> URLRequestHttpJob::{NotifyHeadersComplete,AddExtraHeaders} you log only
> DOMAIN_BLACKLIST_INCLUDES_TARGET.
> 
> So I'm struggling with what I'd like you to do here.  The existing code is a
> mess, for several different reasons.  However, I feel as if, if we change the
> places in the code that file UMA for DOMAIN_BLACKLIST_INCLUDES_TARGET, we need
> to rev the histogram with all the hassle that involves (or, to be fair,
> deprecate this constant and create a new one that means something different). 
I
> think this change (and the changes in the callers) does indeed change the
> frequency and context in which this UMA is logged (since it's no longer logged
> in the GetVcdiffDictionary/GetAvailDictionary/AddSdchDictionary cases.
> 
> I don't want to fix the histogram or deprecate the constant until I'm clear on
> what the right thing to do is, and I don't want to do it multiple times.  And
> I'd also like to keep a 1-1 correspondence between a logging of
> BLACKLIST_INCLUDES_TARGET and an entry in Sdch3.BlacklistReason.  
> 
> So what I'd like to suggest is that you put the SdchErrorRecovery() call back
> here, and remove the UMA logging from URLRequestHttpJob::* (no change needed
to
> the net log logging; you're creating that, so there's no consistency issues). 
> My I get my proverbial round tuit, I'll kill BLACKLIST_INCLUDES_TARGET with
> fire, but until then we'll keep it in its current form.
> 
> Please let me know if you see any problems with that plan.  My apologies if
I've
> asked for different behavior in the past; I skimmed previous comments to see
if
> I could figure out what guidance I've previously given and didn't find any,
but
> given the history of this CL, I might have missed a discussion.
> 
> https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
> net/base/sdch_manager.cc:653: // static
> Why not make this a file static or put it in an anonymous namespace in this
> file?  It's not called from outside this file, so making the visibility as
> narrow as possible seems a good thing.
> 
> https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h
> File net/base/sdch_manager.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h...
> net/base/sdch_manager.h:51: enum ScheduleResult {
> So this (the enum, switching Schedule() to return the enum, and transforming
the
> enum into an SdchManager::ProblemCode at point of call) is a correct
> transformation of the current code, but I don't think it's that useful from a
> debugability point of view, and it's a lot of ugly boilerplate in several
> places, so I'd like to skip it.  Part of the reason I don't think it's useful
> from a debugability point of view is that I don't see a real distinction
between
> ALREADY_TRIED and ALREADY_SCHEDULED in SdchDictionaryFetcher::Schedule; they
> both mean "yeah, I already took a shot at that dictionary, for better or
worse"
> (i.e. I think the existing code is broken).  
> 
> So I think I'd like to ask you to do a slight functionality change in this CL
to
> make the debugging implementation cleaner: Could you obsolete
> DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD, and
> DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD and replace them with a new ProblemCode
> (PREVIOUSLY_SCHEDULED?), and change Schedule() to return a bool indicating
> whether the load was successfully scheduled, and turn a false return from that
> function into the new ProblemCode?  That'll remove a lot of boilerplate.
> 
> https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc
> File net/filter/filter.cc (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.cc#ne...
> net/filter/filter.cc:19: 
> I don't particularly care, but was there a reason for the shift to nested
> namespaces?
> 
> https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h
> File net/filter/filter.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h#new...
> net/filter/filter.h:41: #include "net/base/sdch_manager.h"
> I don't see this used anywhere in this file; remove it?
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter.cc
> File net/filter/sdch_filter.cc (right):
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter....
> net/filter/sdch_filter.cc:455: // errors.
> Suggestion: Move this comment to right above the !possible_pass_through_; on
my
> first read through, I wasn't sure what you were referring to.
> 
>
https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter....
> net/filter/sdch_filter.cc:460: !possible_pass_through_));
> My read of Filter::FixupEncodingTypes() is that we won't use use
> FILTER_TYPE_SDCH_POSSIBLE unless we expected an SDCH response (because we
> advertised a dictionary and the server didn't say X-SDCH-Encode: 0).  In that
> case, I think it's reasonable to call out problems as errors, though it's
> probably worthwhile checking with Matt on what level of thing he'd like to be
> seen as an "error" in net-internals.

[Randy Smith (Not in Mondays), 2014-09-29 21:21:57]

Full review round.  Sorry this took so long--the past couple of weeks have been
a bit crazy.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/main.js:204: addTab(SdchView);
I think this belongs further up in the list, with the other protocols; move it
to after QuicView?  (My goal is shifting the order on the dropdown menu that
picks the tab, and I think this is the right place for that, but I may be
mistaken due to not knowing the net-internals code well enough.)

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.html (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
Suggestion: I think this section might look better to the user if it was
something like:

SDCH Errors:  _Resource_  <tab> _Dictionary_

With the links inactive and " (none)" appended to the names if there aren't any
errors of that type (if that's possible).

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
I don't think this is in the style guide, so it should be taken as purely my
preference and just a suggestion, but: I'd feel better if these lines were < 80
characters, and I don't think there's anything stopping that if they're nested.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
These two links are effectively shortcuts for common searches that people may
want to do in the events tab.  I like that (though Matt might shoot it down
based on overall net-internals design) but if we're going to do that, I'd
personally like a couple more:
* All SDCH dictionary fetches.
* All URL requests encoded with SDCH
* All URL requests advertising SDCH dictionaries
* (Stretching; not sure this is a good idea) All URL requests advertising SDCH
dictionaries that did/did not have responses with SDCH encoding.

https://codereview.chromium.org/423813002/diff/220001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/net_internals_test.js (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/net_internals_test.js:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Have you looked at adding SDCH tests parallel to the tests for the other tabs in
this directory?

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list...
net/base/net_error_list.h:666: NET_ERROR(SDCH_PROBLEM, -504)
On 2014/09/19 12:42:44, baranovich wrote:
> On 2014/09/18 20:55:51, rdsmith wrote:
> > I don't think this is used anymore; remove?
> 
> No, it is still used by net-internals code. I need some error code to signal
ui
> to paint URL_REQUEST red in event log.

Ah, gotcha, that makes sense.

I could imagine you'll be unhappy with me for saying this :-{, but: I'd rather
not add a net error code for this purpose.  Would you be willing to write a
mapping function from SDCH problem codes to existing net error codes and call it
at the various points at which you call NetLog::AddEvent(*SDCH*ERROR, ...)?  
Something like content/browser/download/download_interrupt_reasons_impl.cc
(though that goes the other way).  It doesn't have to be a precisely correct
mapping; something that gestures in the right direction is sufficient.

Or it occurs to me, now that I've suggested the complicated and baroque
solution: Is there any reason you can't just use NET_ERROR_FAILED?

https://codereview.chromium.org/423813002/diff/220001/net/base/net_log_event_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2408: EVENT_TYPE(SDCH_RESOURCE_ERROR)
What would you think of changing the name of this to SDCH_DECODING_ERROR?  I was
a bit confused reading through the code what this meant until I traced where it
came from, and I think DECODING captures that these are errors that happen
during the process of receiving a request encoded with SDCH and trying to decode
it.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:21: #include "net/base/net_log.h"
Why is this needed in the header file?  It doesn't seem used.

[baranovich, 2014-09-30 13:16:52]

More or less done with all comments, plz take a look once more:)

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/main.js:204: addTab(SdchView);
On 2014/09/29 21:21:56, rdsmith wrote:
> I think this belongs further up in the list, with the other protocols; move it
> to after QuicView?  (My goal is shifting the order on the dropdown menu that
> picks the tab, and I think this is the right place for that, but I may be
> mistaken due to not knowing the net-internals code well enough.)

Done.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.html (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
On 2014/09/29 21:21:56, rdsmith wrote:
> Suggestion: I think this section might look better to the user if it was
> something like:
> 
> SDCH Errors:  _Resource_  <tab> _Dictionary_
> 
> With the links inactive and " (none)" appended to the names if there aren't
any
> errors of that type (if that's possible).

Done.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
On 2014/09/29 21:21:56, rdsmith wrote:
> These two links are effectively shortcuts for common searches that people may
> want to do in the events tab.  I like that (though Matt might shoot it down
> based on overall net-internals design) but if we're going to do that, I'd
> personally like a couple more:
> * All SDCH dictionary fetches.
> * All URL requests encoded with SDCH
> * All URL requests advertising SDCH dictionaries
> * (Stretching; not sure this is a good idea) All URL requests advertising SDCH
> dictionaries that did/did not have responses with SDCH encoding.

Done.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
On 2014/09/29 21:21:56, rdsmith wrote:
> These two links are effectively shortcuts for common searches that people may
> want to do in the events tab.  I like that (though Matt might shoot it down
> based on overall net-internals design) but if we're going to do that, I'd
> personally like a couple more:
> * All SDCH dictionary fetches.
> * All URL requests encoded with SDCH
> * All URL requests advertising SDCH dictionaries
> * (Stretching; not sure this is a good idea) All URL requests advertising SDCH
> dictionaries that did/did not have responses with SDCH encoding.

Done. I've figured out how to do only 2 of those although. 

"All URL requests encoded with SDCH" means filtering events by smth
like/Content-Type\:.*,\s*sdch\s*,.*/ which is not possible. Otherwise, that
means adding special Event to filter by it. These requests are a subset of url
requests advertising dictionaries. Minus small fractions of errors which can be
found using "Decoding" error link.

"(Stretching; not sure this is a good idea) All URL requests advertising SDCH
dictionaries that did/did not have responses with SDCH encoding." also
impossible with simple filtering. Effectively, this means a subset of requests
with Decoding errors:)

I'm not sure if such granularity is required.. imho the purpose of this tab to
aid developers to debug their SDCH servers, which means that number of requests
will be more or less limited and even if not, the developer probably will now
the problem url and filter by it. But that's totally my intuition which may be
absolutely wrong, plus wish to get this CL finally done as some MVP, and add
more bells and whistles later:).

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/net_error_list...
net/base/net_error_list.h:666: NET_ERROR(SDCH_PROBLEM, -504)
On 2014/09/29 21:21:57, rdsmith wrote:
> On 2014/09/19 12:42:44, baranovich wrote:
> > On 2014/09/18 20:55:51, rdsmith wrote:
> > > I don't think this is used anymore; remove?
> > 
> > No, it is still used by net-internals code. I need some error code to signal
> ui
> > to paint URL_REQUEST red in event log.
> 
> Ah, gotcha, that makes sense.
> 
> I could imagine you'll be unhappy with me for saying this :-{, but: I'd rather
> not add a net error code for this purpose.  Would you be willing to write a
> mapping function from SDCH problem codes to existing net error codes and call
it
> at the various points at which you call NetLog::AddEvent(*SDCH*ERROR, ...)?  
> Something like content/browser/download/download_interrupt_reasons_impl.cc
> (though that goes the other way).  It doesn't have to be a precisely correct
> mapping; something that gestures in the right direction is sufficient.
> 
> Or it occurs to me, now that I've suggested the complicated and baroque
> solution: Is there any reason you can't just use NET_ERROR_FAILED?

Done. Using ERR_FAILED instead

https://codereview.chromium.org/423813002/diff/220001/net/base/net_log_event_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2408: EVENT_TYPE(SDCH_RESOURCE_ERROR)
On 2014/09/29 21:21:57, rdsmith wrote:
> What would you think of changing the name of this to SDCH_DECODING_ERROR?  I
was
> a bit confused reading through the code what this meant until I traced where
it
> came from, and I think DECODING captures that these are errors that happen
> during the process of receiving a request encoded with SDCH and trying to
decode
> it.

Done.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:21: #include "net/base/net_log.h"
On 2014/09/29 21:21:57, rdsmith wrote:
> Why is this needed in the header file?  It doesn't seem used.

Done.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:346:
SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
On 2014/09/18 20:55:51, rdsmith wrote:
> Just to summarize what I think the refactor of this routine did in terms of
> calls to SdchManager::SdchErrorRecovery(), in case I missed something: It
looks
> like you removed this call here, return problem codes but don't log them in
the
> callers, *except* that in
> URLRequestHttpJob::{NotifyHeadersComplete,AddExtraHeaders} you log only
> DOMAIN_BLACKLIST_INCLUDES_TARGET.
> 
> So I'm struggling with what I'd like you to do here.  The existing code is a
> mess, for several different reasons.  However, I feel as if, if we change the
> places in the code that file UMA for DOMAIN_BLACKLIST_INCLUDES_TARGET, we need
> to rev the histogram with all the hassle that involves (or, to be fair,
> deprecate this constant and create a new one that means something different). 
I
> think this change (and the changes in the callers) does indeed change the
> frequency and context in which this UMA is logged (since it's no longer logged
> in the GetVcdiffDictionary/GetAvailDictionary/AddSdchDictionary cases.
> 
> I don't want to fix the histogram or deprecate the constant until I'm clear on
> what the right thing to do is, and I don't want to do it multiple times.  And
> I'd also like to keep a 1-1 correspondence between a logging of
> BLACKLIST_INCLUDES_TARGET and an entry in Sdch3.BlacklistReason.  
> 
> So what I'd like to suggest is that you put the SdchErrorRecovery() call back
> here, and remove the UMA logging from URLRequestHttpJob::* (no change needed
to
> the net log logging; you're creating that, so there's no consistency issues). 
> My I get my proverbial round tuit, I'll kill BLACKLIST_INCLUDES_TARGET with
> fire, but until then we'll keep it in its current form.
> 
> Please let me know if you see any problems with that plan.  My apologies if
I've
> asked for different behavior in the past; I skimmed previous comments to see
if
> I could figure out what guidance I've previously given and didn't find any,
but
> given the history of this CL, I might have missed a discussion.
> 
> 

Done. I've restored the frequency if the blacklist histogram. Now it is recorded
outside by the caller.

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/base/sdch_manager.h...
net/base/sdch_manager.h:51: enum ScheduleResult {
On 2014/09/18 20:55:51, rdsmith wrote:
> So this (the enum, switching Schedule() to return the enum, and transforming
the
> enum into an SdchManager::ProblemCode at point of call) is a correct
> transformation of the current code, but I don't think it's that useful from a
> debugability point of view, and it's a lot of ugly boilerplate in several
> places, so I'd like to skip it.  Part of the reason I don't think it's useful
> from a debugability point of view is that I don't see a real distinction
between
> ALREADY_TRIED and ALREADY_SCHEDULED in SdchDictionaryFetcher::Schedule; they
> both mean "yeah, I already took a shot at that dictionary, for better or
worse"
> (i.e. I think the existing code is broken).  
> 
> So I think I'd like to ask you to do a slight functionality change in this CL
to
> make the debugging implementation cleaner: Could you obsolete
> DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD, and
> DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD and replace them with a new ProblemCode
> (PREVIOUSLY_SCHEDULED?), and change Schedule() to return a bool indicating
> whether the load was successfully scheduled, and turn a false return from that
> function into the new ProblemCode?  That'll remove a lot of boilerplate.

Done.

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h
File net/filter/filter.h (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/filter.h#new...
net/filter/filter.h:41: #include "net/base/sdch_manager.h"
On 2014/09/18 20:55:51, rdsmith wrote:
> I don't see this used anywhere in this file; remove it?

Done.

https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/220001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:460: !possible_pass_through_));
On 2014/09/18 20:55:51, rdsmith wrote:
> My read of Filter::FixupEncodingTypes() is that we won't use use
> FILTER_TYPE_SDCH_POSSIBLE unless we expected an SDCH response (because we
> advertised a dictionary and the server didn't say X-SDCH-Encode: 0).  In that
> case, I think it's reasonable to call out problems as errors, though it's
> probably worthwhile checking with Matt on what level of thing he'd like to be
> seen as an "error" in net-internals.  

You're right. I removed this

[baranovich, 2014-10-02 09:43:49]

1 more thing about testing
Similar tabs (SPDY for instance) does not have any tests, and I guess there's
nothing to test here. If js does not break the page, then it works.
Not sure if this reason is good enough although:)

[baranovich, 2014-10-02 09:43:50]

1 more thing about testing
Similar tabs (SPDY for instance) does not have any tests, and I guess there's
nothing to test here. If js does not break the page, then it works.
Not sure if this reason is good enough although:)

[mmenke, 2014-10-02 14:50:07]

On 2014/10/02 09:43:50, baranovich wrote:
> 1 more thing about testing
> Similar tabs (SPDY for instance) does not have any tests, and I guess there's
> nothing to test here. If js does not break the page, then it works.
> Not sure if this reason is good enough although:)

Note:  The below was written without actually looking at the code.  I'd hope to
have time tomorrow to do so, but I'm not confident of that.

For a long time net-internals had no tests, then after a couple regressions, and
the addition of a WebUI testing framework, we finally started adding them. 
Ideally, new tabs should a couple tests, if they can be reasonably tested in a
meaningful way (SPDY and the sockets tab were grandfathered in...  but they also
can't be tested that meaningfully with the out of process test server, since it
doesn't support SPDY, and uses connection-close on all sockets - may be possible
to work around the sockets tab issues).

I think we could reasonably land this without tests, but...  The SDCH tab, while
certainly useful and worth adding, probably won't get a lot of use by Chrome
developers on every release, so regressions are quite likely, and it would be
great if we could have one or two very basic tests.  Randy's also doing some
refactoring there, which could also cause breakages.

The current SDCH browser tests are flaky, I believe, but I think we can make an
SDCH net-internals test that isn't:  Download a resource with the SDCH support
header, wait for net-internals to be updated with information about downloading
the dictionary.  If information is recorded on use, can then download a resource
that uses the dictionary, and make sure we see the expected result.

Could also have a test of an error case or two (Dictionary download fails,
second resource doesn't use SDCH, etc).

These tests would also have the side benefit of increasing integration tests
coverage of the SDCH infrastructure.

[Randy Smith (Not in Mondays), 2014-10-02 21:28:45]

Review comments.  One high level question: Do you have any idea why the
SdchDictionaryFetcher URL_REQUESTs show blank where there should be a URL in the
event source listing?  It's probably a bug in the current code and hence my
fault, but it's worth fixing for making this work well with net-internals.  But
feel free to bounce it back to me if you don't know.

Given that Matt's expressing an interest again, I'll call out that things that
I'd specifically like Matt's eye on:
* The quick links from the sdch_view.html page.  Such links do exist on other
pages, but I'm not sure if they're best practices, or if we want all such
navigations to go through JS.  (There's some other stuff on the sdch_view.html
page that raised the same type of question).
* The below question about how easy/possible it would be to make the links on
the sdch_view.html tab change appearance and enabled/disabled state based on
whether there are any events of the type they link to.
* The amount of testing required :-}.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.html (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
On 2014/09/30 13:16:51, baranovich wrote:
> On 2014/09/29 21:21:56, rdsmith wrote:
> > These two links are effectively shortcuts for common searches that people
may
> > want to do in the events tab.  I like that (though Matt might shoot it down
> > based on overall net-internals design) but if we're going to do that, I'd
> > personally like a couple more:
> > * All SDCH dictionary fetches.
> > * All URL requests encoded with SDCH
> > * All URL requests advertising SDCH dictionaries
> > * (Stretching; not sure this is a good idea) All URL requests advertising
SDCH
> > dictionaries that did/did not have responses with SDCH encoding.
> 
> Done. I've figured out how to do only 2 of those although. 

That's fine--I was brainstorming.

> "All URL requests encoded with SDCH" means filtering events by smth
> like/Content-Type\:.*,\s*sdch\s*,.*/ which is not possible. Otherwise, that
> means adding special Event to filter by it. These requests are a subset of url
> requests advertising dictionaries. Minus small fractions of errors which can
be
> found using "Decoding" error link.
> 
> "(Stretching; not sure this is a good idea) All URL requests advertising SDCH
> dictionaries that did/did not have responses with SDCH encoding." also
> impossible with simple filtering. Effectively, this means a subset of requests
> with Decoding errors:)
> 
> I'm not sure if such granularity is required.. imho the purpose of this tab to
> aid developers to debug their SDCH servers, which means that number of
requests
> will be more or less limited and even if not, the developer probably will now
> the problem url and filter by it. But that's totally my intuition which may be
> absolutely wrong, plus wish to get this CL finally done as some MVP, and add
> more bells and whistles later:).

We have different goals :-}; I want this tab to be usable for people (sic :-};
in practice this means me) debugging Chrome's implementation of SDCH.  That's
net-internals primary purpose (helping debug chrome problems).  But I don't
think there's any conflict between those goals.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
On 2014/09/30 13:16:51, baranovich wrote:
> On 2014/09/29 21:21:56, rdsmith wrote:
> > Suggestion: I think this section might look better to the user if it was
> > something like:
> > 
> > SDCH Errors:  _Resource_  <tab> _Dictionary_
> > 
> > With the links inactive and " (none)" appended to the names if there aren't
> any
> > errors of that type (if that's possible).
> 
> Done.

Is it reasonable to make the links inactive and have " (none)" appended if there
aren't any of a particular type of error?  That doesn't appear to be done in the
current PS.  (This may be a question for Matt.)

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:367:
SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
So I'm still confused and concerned by this refactor.  It looks as if you've
pulled the logging into some but not all of the calling tree (.e.g
URLRHJ::AddExtraHeaders ignores the return code from GetAvailDictionaryList,
which calls this, and SDF::DoCompleteHeaders logs everything except a subset of
return codes, and it calls AddSdchDictionary, which calls this).  So I don't see
how the BLACKLIST_INCLUDES_TARGET logging is unchanged across this CL, which is
what I thought we were going for with the most recent change.  And I also think
we're going to be changing the contexts in which the other error codes from this
routine get passed to SdchErrorRecovery().  

This is the only place that returns SDCH_BLACKLIST_INCLUDES_TARGET.  Given that,
I think the minimal code change (and hence lowest likelyhood of bugs) is to
leave this call to SdchErrorRecovery here, and make sure not to call
SdchErrorRecovery based on the return code from this function in the calling
tree.  I realize that this may be an almost impossible thing to do.

Alternatively, you could rev the Sdch3.ProblemCodes_4 histogram and go to town,
which might actually end up being the smallest code change.  I'm not ecstatic
about that just because I'll probably do another rev at some point in the
future, but truth to tell, it's not actually a horrible thing (either abstractly
or in terms of amount of coding), and I'm not coming up with a clean solution
other than that which fufills both your goals for this CL and my goal of not
having the histogram distribution change across this CL.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:15: #include "net/base/sdch_net_log_params.h"
By the compartmentalization I'm reaching for, this shouldn't be needed anymore?

[mmenke, 2014-10-02 21:53:56]

On 2014/10/02 21:28:45, rdsmith wrote:
> Given that Matt's expressing an interest again, I'll call out that things that
> I'd specifically like Matt's eye on:
> * The quick links from the sdch_view.html page.  Such links do exist on other
> pages, but I'm not sure if they're best practices, or if we want all such
> navigations to go through JS.  (There's some other stuff on the sdch_view.html
> page that raised the same type of question).
Those links are fine (And encouraged!)

> * The below question about how easy/possible it would be to make the links on
> the sdch_view.html tab change appearance and enabled/disabled state based on
> whether there are any events of the type they link to.
> * The amount of testing required :-}.

Most net-internals tests are full integration tests, though we do have unit
tests for some of the more convoluted logic.  Glancing over the JS and HTML,
this looks fairly simple, so I'd think the obvious cases to test are:
1)  An integration test with a happily loaded dictionary.  Make sure we populate
domain, port, etc properly.
2)  Integration test that blacklists a domain - ideally two cases, to make sure
sdchProblemCodeToString works properly.

The way a unit test would work would be to register its own onSdchInfoChanged
observer, switch to the sdch tab, and then kick off a load (Or two) that
triggers the error, and just wait until it sees the corresponding data and make
sure the tab was updated properly.

Did I miss anything you particularly wanted from me?  I should have time for a
full pass tomorrow.

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.html (right):

https://codereview.chromium.org/423813002/diff/220001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:9: <p><a
href="#events&q=type:URL_REQUEST%20is:error%20SDCH_DICTIONARY">View SDCH
dictionary fetch errors</a></p>
On 2014/10/02 21:28:45, rdsmith wrote:
> On 2014/09/30 13:16:51, baranovich wrote:
> > On 2014/09/29 21:21:56, rdsmith wrote:
> > > Suggestion: I think this section might look better to the user if it was
> > > something like:
> > > 
> > > SDCH Errors:  _Resource_  <tab> _Dictionary_
> > > 
> > > With the links inactive and " (none)" appended to the names if there
aren't
> > any
> > > errors of that type (if that's possible).
> > 
> > Done.
> 
> Is it reasonable to make the links inactive and have " (none)" appended if
there
> aren't any of a particular type of error?  That doesn't appear to be done in
the
> current PS.  (This may be a question for Matt.)

It's possible, but you'd have to search though all events for such an error as
they occur.  I'd recommend not trying to get too fancy on the first pass.

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.html (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.html:10: <a
href="#events&q=type:URL_REQUEST%20SDCH_DICTIONARY_ERROR">Dictionary</a>
Would it be more user friendly to just have on link, using
#events&q=type:URL_REQUEST%20sdch_problem_code?

[baranovich, 2014-10-02 23:01:50]

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:367:
SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
On 2014/10/02 21:28:45, rdsmith wrote:
> So I'm still confused and concerned by this refactor.  It looks as if you've
> pulled the logging into some but not all of the calling tree (.e.g
> URLRHJ::AddExtraHeaders ignores the return code from GetAvailDictionaryList,
> which calls this, and SDF::DoCompleteHeaders logs everything except a subset
of
> return codes, and it calls AddSdchDictionary, which calls this).  So I don't
see
> how the BLACKLIST_INCLUDES_TARGET logging is unchanged across this CL, which
is
> what I thought we were going for with the most recent change.  And I also
think
> we're going to be changing the contexts in which the other error codes from
this
> routine get passed to SdchErrorRecovery().  
> 
> This is the only place that returns SDCH_BLACKLIST_INCLUDES_TARGET.  Given
that,
> I think the minimal code change (and hence lowest likelyhood of bugs) is to
> leave this call to SdchErrorRecovery here, and make sure not to call
> SdchErrorRecovery based on the return code from this function in the calling
> tree.  I realize that this may be an almost impossible thing to do.
No. that's possible, but I was not sure if it makes the code cleaner, given that
netlog event should still be logged. So I decided to do as I did. But I'll try
that approach.

> 
> Alternatively, you could rev the Sdch3.ProblemCodes_4 histogram and go to
town,
> which might actually end up being the smallest code change.  I'm not ecstatic
> about that just because I'll probably do another rev at some point in the
> future, but truth to tell, it's not actually a horrible thing (either
abstractly
> or in terms of amount of coding), and I'm not coming up with a clean solution
> other than that which fufills both your goals for this CL and my goal of not
> having the histogram distribution change across this CL.
I'm afraid I didn't got what you suggest here, my English sometimes is too bad(


Yes, unfortunately the code became pretty ugly =(
Below are some clarifications:
- means what happens now
+ means what this CL does

- URLRHJ::AddExtraHeaders may write one blacklist histogram by calling to
SM::IsInSupportedDomain.
+ Now this histogram is added by URLRHJ::AddExtraHeaders when it checks retval
of SM::IsInSupportedDomain. Also netlog event is logged. 

- URLRHJ::AddExtraHeaders may write lots of them by calling to
SM::GetAvailDictionaryList which calls SM::IsInSupportedDomain many times
+ Now these histograms are written by SM::GetAvailDictionaryList when it check
retvals from SM::IsInSupportedDomain. netlog events are not written. I guess
they won't be useful:)

- URLRHJ::NotifyHeadersComplete may write one blacklist histogram by calling to
SM::IsInSupportedDomain
+ Now this histogram is added by URLRHJ::NotifyHeadersComplete when it checks
retval of SM::IsInSupportedDomain. Also netlog event is logged.

- SDCHFilter::InitializeDictionary calls SM::GetVcdiffDictionary which calls to
SM::IsInSupportedDomain and may add one blacklist histogram
+SDCHFilter::InitializeDictionary will log this histogram after checking retval
of SM::GetVcdiffDictionary and add netlog event

- at last SM::AddSdchDictionary may write one blacklist histogram when calling
to SM::IsInSupportedDomain
+ Now SM::AddSdchDictionary returns error to SDF::DoCompleteRequest, which
writes blacklist histogram among other problem codes and adds netlog event.

[mmenke, 2014-10-03 18:59:27]

I'm deferring heavily to Randy in terms of the changes to the SDCH logic.

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/main.js:315: SdchProblemCode =
Constants.sdchProblemCode;
Should declare this up with the others (Not currently necessary, because this
code currently falls outside of the 'use strict', but seems like a good idea)

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.js:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit:  New files shouldn't use a (c).  No idea why.

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/log_util.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/log_util.js:179: sdch: true,
nit:  It's not done consistently, but this should match the order in main.js, so
it should be just below "quic" (x2).

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/main.js:44: sdch: true
nit:  It's not done consistently, but this should match the order in main.js, so
it should be just below "quic".

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/net_internals_test.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/net_internals_test.js:306: sdch:
SdchView.TAB_ID
nit:  Move this just below "quic".

https://codereview.chromium.org/423813002/diff/240001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/net_log_logger...
net/base/net_log_logger.cc:163: dict->SetInteger(#label,
static_cast<int>(value));
nit:  +2 indent.

https://codereview.chromium.org/423813002/diff/240001/net/base/net_log_logger...
net/base/net_log_logger.cc:165: #undef SDCH_PROBLEM_CODE
The code in this file has been refactored to reduce binary size.  Please merge
and copy the new style.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:146: return SDCH_DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN;
 // domain was a TLD.
nit:  Should use braces when the conditional takes up multiple lines.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:159: return
SDCH_DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX;
nit:  Use braces

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_net_log_p...
File net/base/sdch_net_log_params.h (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_net_log_p...
net/base/sdch_net_log_params.h:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
nit:  Don't use (c) in new files (Goes for all the other added files, too).

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_net_log_p...
net/base/sdch_net_log_params.h:25: const GURL* url,
nit:  "const GURL& url".  Use const references instead of pointers when a value
can't be NULL, and isn't modified.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_problem_c...
File net/base/sdch_problem_codes.h (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_problem_c...
net/base/sdch_problem_codes.h:12: enum SdchProblemCodes {
nit:  "SdchProblemCode" is the more common style, with the exception of enums
that are flags.

[Randy Smith (Not in Mondays), 2014-10-06 19:24:57]

Matt: I'm happy to have you defer to me if it's to save yourself time, but if
there are things that you see in the code that you don't like, please do call
them out and I'll argue (or, more likely, agree) if I have an opinion on them
:-}.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (left):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:367:
SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
On 2014/10/02 23:01:50, baranovich wrote:
> On 2014/10/02 21:28:45, rdsmith wrote:
> > So I'm still confused and concerned by this refactor.  It looks as if you've
> > pulled the logging into some but not all of the calling tree (.e.g
> > URLRHJ::AddExtraHeaders ignores the return code from GetAvailDictionaryList,
> > which calls this, and SDF::DoCompleteHeaders logs everything except a subset
> of
> > return codes, and it calls AddSdchDictionary, which calls this).  So I don't
> see
> > how the BLACKLIST_INCLUDES_TARGET logging is unchanged across this CL, which
> is
> > what I thought we were going for with the most recent change.  And I also
> think
> > we're going to be changing the contexts in which the other error codes from
> this
> > routine get passed to SdchErrorRecovery().  
> > 
> > This is the only place that returns SDCH_BLACKLIST_INCLUDES_TARGET.  Given
> that,
> > I think the minimal code change (and hence lowest likelyhood of bugs) is to
> > leave this call to SdchErrorRecovery here, and make sure not to call
> > SdchErrorRecovery based on the return code from this function in the calling
> > tree.  I realize that this may be an almost impossible thing to do.
> No. that's possible, but I was not sure if it makes the code cleaner, given
that
> netlog event should still be logged. So I decided to do as I did. But I'll try
> that approach.
> 
> > 
> > Alternatively, you could rev the Sdch3.ProblemCodes_4 histogram and go to
> town,
> > which might actually end up being the smallest code change.  I'm not
ecstatic
> > about that just because I'll probably do another rev at some point in the
> > future, but truth to tell, it's not actually a horrible thing (either
> abstractly
> > or in terms of amount of coding), and I'm not coming up with a clean
solution
> > other than that which fufills both your goals for this CL and my goal of not
> > having the histogram distribution change across this CL.
> I'm afraid I didn't got what you suggest here, my English sometimes is too
bad(

No, my apologies; I was writing too quickly.  What I mean is:
* Create a new histogram, Sdch3.ProblemCodes_5, and mark the old one obsolete
(tools/metrics/histograms/histograms.xml)
* Change the code to write to that histogram instead of the old one.
* Log to that histogram as makes sense for your new code, without worrying about
whether you're going to have the logging counts for different errors as the old
code did--since it's a new histogram, it can have different shapes.  

I think that'll be easier than what you've done, and produce cleaner code.  Does
that makes sense?

> Yes, unfortunately the code became pretty ugly =(
> Below are some clarifications:
> - means what happens now
> + means what this CL does
> 
> - URLRHJ::AddExtraHeaders may write one blacklist histogram by calling to
> SM::IsInSupportedDomain.
> + Now this histogram is added by URLRHJ::AddExtraHeaders when it checks retval
> of SM::IsInSupportedDomain. Also netlog event is logged. 
> 
> - URLRHJ::AddExtraHeaders may write lots of them by calling to
> SM::GetAvailDictionaryList which calls SM::IsInSupportedDomain many times
> + Now these histograms are written by SM::GetAvailDictionaryList when it check
> retvals from SM::IsInSupportedDomain. netlog events are not written. I guess
> they won't be useful:)
> 
> - URLRHJ::NotifyHeadersComplete may write one blacklist histogram by calling
to
> SM::IsInSupportedDomain
> + Now this histogram is added by URLRHJ::NotifyHeadersComplete when it checks
> retval of SM::IsInSupportedDomain. Also netlog event is logged.
> 
> - SDCHFilter::InitializeDictionary calls SM::GetVcdiffDictionary which calls
to
> SM::IsInSupportedDomain and may add one blacklist histogram
> +SDCHFilter::InitializeDictionary will log this histogram after checking
retval
> of SM::GetVcdiffDictionary and add netlog event
> 
> - at last SM::AddSdchDictionary may write one blacklist histogram when calling
> to SM::IsInSupportedDomain
> + Now SM::AddSdchDictionary returns error to SDF::DoCompleteRequest, which
> writes blacklist histogram among other problem codes and adds netlog event.

I have a hard time visualizing this without seeing the new code, but it doesn't
sound a lot cleaner.  What do you think of creating the new histogram as
sketched above?

[baranovich, 2014-10-07 19:16:20]

On 2014/10/06 19:24:57, rdsmith wrote:
> Matt: I'm happy to have you defer to me if it's to save yourself time, but if
> there are things that you see in the code that you don't like, please do call
> them out and I'll argue (or, more likely, agree) if I have an opinion on them
> :-}.
> 
> https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (left):
> 
>
https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
> net/base/sdch_manager.cc:367:
> SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
> On 2014/10/02 23:01:50, baranovich wrote:
> > On 2014/10/02 21:28:45, rdsmith wrote:
> > > So I'm still confused and concerned by this refactor.  It looks as if
you've
> > > pulled the logging into some but not all of the calling tree (.e.g
> > > URLRHJ::AddExtraHeaders ignores the return code from
GetAvailDictionaryList,
> > > which calls this, and SDF::DoCompleteHeaders logs everything except a
subset
> > of
> > > return codes, and it calls AddSdchDictionary, which calls this).  So I
don't
> > see
> > > how the BLACKLIST_INCLUDES_TARGET logging is unchanged across this CL,
which
> > is
> > > what I thought we were going for with the most recent change.  And I also
> > think
> > > we're going to be changing the contexts in which the other error codes
from
> > this
> > > routine get passed to SdchErrorRecovery().  
> > > 
> > > This is the only place that returns SDCH_BLACKLIST_INCLUDES_TARGET.  Given
> > that,
> > > I think the minimal code change (and hence lowest likelyhood of bugs) is
to
> > > leave this call to SdchErrorRecovery here, and make sure not to call
> > > SdchErrorRecovery based on the return code from this function in the
calling
> > > tree.  I realize that this may be an almost impossible thing to do.
> > No. that's possible, but I was not sure if it makes the code cleaner, given
> that
> > netlog event should still be logged. So I decided to do as I did. But I'll
try
> > that approach.
> > 
> > > 
> > > Alternatively, you could rev the Sdch3.ProblemCodes_4 histogram and go to
> > town,
> > > which might actually end up being the smallest code change.  I'm not
> ecstatic
> > > about that just because I'll probably do another rev at some point in the
> > > future, but truth to tell, it's not actually a horrible thing (either
> > abstractly
> > > or in terms of amount of coding), and I'm not coming up with a clean
> solution
> > > other than that which fufills both your goals for this CL and my goal of
not
> > > having the histogram distribution change across this CL.
> > I'm afraid I didn't got what you suggest here, my English sometimes is too
> bad(
> 
> No, my apologies; I was writing too quickly.  What I mean is:
> * Create a new histogram, Sdch3.ProblemCodes_5, and mark the old one obsolete
> (tools/metrics/histograms/histograms.xml)
> * Change the code to write to that histogram instead of the old one.
> * Log to that histogram as makes sense for your new code, without worrying
about
> whether you're going to have the logging counts for different errors as the
old
> code did--since it's a new histogram, it can have different shapes.  
> 
> I think that'll be easier than what you've done, and produce cleaner code. 
Does
> that makes sense?
> 
> > Yes, unfortunately the code became pretty ugly =(
> > Below are some clarifications:
> > - means what happens now
> > + means what this CL does
> > 
> > - URLRHJ::AddExtraHeaders may write one blacklist histogram by calling to
> > SM::IsInSupportedDomain.
> > + Now this histogram is added by URLRHJ::AddExtraHeaders when it checks
retval
> > of SM::IsInSupportedDomain. Also netlog event is logged. 
> > 
> > - URLRHJ::AddExtraHeaders may write lots of them by calling to
> > SM::GetAvailDictionaryList which calls SM::IsInSupportedDomain many times
> > + Now these histograms are written by SM::GetAvailDictionaryList when it
check
> > retvals from SM::IsInSupportedDomain. netlog events are not written. I guess
> > they won't be useful:)
> > 
> > - URLRHJ::NotifyHeadersComplete may write one blacklist histogram by calling
> to
> > SM::IsInSupportedDomain
> > + Now this histogram is added by URLRHJ::NotifyHeadersComplete when it
checks
> > retval of SM::IsInSupportedDomain. Also netlog event is logged.
> > 
> > - SDCHFilter::InitializeDictionary calls SM::GetVcdiffDictionary which calls
> to
> > SM::IsInSupportedDomain and may add one blacklist histogram
> > +SDCHFilter::InitializeDictionary will log this histogram after checking
> retval
> > of SM::GetVcdiffDictionary and add netlog event
> > 
> > - at last SM::AddSdchDictionary may write one blacklist histogram when
calling
> > to SM::IsInSupportedDomain
> > + Now SM::AddSdchDictionary returns error to SDF::DoCompleteRequest, which
> > writes blacklist histogram among other problem codes and adds netlog event.
> 
> I have a hard time visualizing this without seeing the new code, but it
doesn't
> sound a lot cleaner.  What do you think of creating the new histogram as
> sketched above?

Got it, sure I'd better add a new histogram:)

[baranovich, 2014-10-14 10:43:38]

Hi Randy,

coming back to this code, unfortunately I was too busy with other stuff :(

Do you indeed mean creation of the whole new histogram? Or I may just deprecate
DOMAIN_BLACKLIST_INCLUDES_TARGET in `Sdch3.ProblemCodes_4` and add a new one?

[Randy Smith (Not in Mondays), 2014-10-14 12:15:38]

On 2014/10/14 10:43:38, baranovich wrote:
> Hi Randy,
> 
> coming back to this code, unfortunately I was too busy with other stuff :(
> 
> Do you indeed mean creation of the whole new histogram? Or I may just
deprecate
> DOMAIN_BLACKLIST_INCLUDES_TARGET in `Sdch3.ProblemCodes_4` and add a new one?

I had meant create a whole new histogram, which I don't actually think would be
that big a deal (just copy ProblemCodes_4 and change the number, then change all
instances of ProblemCodes_4 in the code to _5).  I don't have any objection to
deprecating DBIT and adding a new entry if that doesn't change the frequency of
other elements of the histogram, but I'm worried it would either do that or we'd
be left with the awkward "We'll log some of these codes at this point and not
others" code we currently have.

[baranovich, 2014-10-27 20:49:08]

Hi Matt, Randy,

I've changed the histogram, made some tests and fixed other nits.

You're welcome to review this again:)

PS I should add someone else for the histograms.xml review I guess, right?

[baranovich, 2014-10-27 20:49:25]

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/main.js:315: SdchProblemCode =
Constants.sdchProblemCode;
On 2014/10/03 18:59:26, mmenke wrote:
> Should declare this up with the others (Not currently necessary, because this
> code currently falls outside of the 'use strict', but seems like a good idea)

Matt, not quite sure if I got what you mean. I guess I'm don't know enough about
net-internals internals:) This is where all constants are initialized, I thought
this is the right place.

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.js:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/10/03 18:59:26, mmenke wrote:
> nit:  New files shouldn't use a (c).  No idea why.

Done.

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/log_util.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/log_util.js:179: sdch: true,
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  It's not done consistently, but this should match the order in main.js,
so
> it should be just below "quic" (x2).

Done.

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/main.js:44: sdch: true
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  It's not done consistently, but this should match the order in main.js,
so
> it should be just below "quic".

Done.

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/net_internals_test.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/net_internals_test.js:306: sdch:
SdchView.TAB_ID
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  Move this just below "quic".

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/net_log_logger.cc
File net/base/net_log_logger.cc (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/net_log_logger...
net/base/net_log_logger.cc:163: dict->SetInteger(#label,
static_cast<int>(value));
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  +2 indent.

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/net_log_logger...
net/base/net_log_logger.cc:165: #undef SDCH_PROBLEM_CODE
On 2014/10/03 18:59:27, mmenke wrote:
> The code in this file has been refactored to reduce binary size.  Please merge
> and copy the new style.

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:146: return SDCH_DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN;
 // domain was a TLD.
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  Should use braces when the conditional takes up multiple lines.

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:159: return
SDCH_DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX;
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  Use braces

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_net_log_p...
File net/base/sdch_net_log_params.h (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_net_log_p...
net/base/sdch_net_log_params.h:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  Don't use (c) in new files (Goes for all the other added files, too).

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_net_log_p...
net/base/sdch_net_log_params.h:25: const GURL* url,
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  "const GURL& url".  Use const references instead of pointers when a
value
> can't be NULL, and isn't modified.

Done.

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_problem_c...
File net/base/sdch_problem_codes.h (right):

https://codereview.chromium.org/423813002/diff/240001/net/base/sdch_problem_c...
net/base/sdch_problem_codes.h:12: enum SdchProblemCodes {
On 2014/10/03 18:59:27, mmenke wrote:
> nit:  "SdchProblemCode" is the more common style, with the exception of enums
> that are flags.

Done.

[mmenke, 2014-10-27 21:29:34]

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/main.js:315: SdchProblemCode =
Constants.sdchProblemCode;
On 2014/10/27 20:49:24, baranovich wrote:
> On 2014/10/03 18:59:26, mmenke wrote:
> > Should declare this up with the others (Not currently necessary, because
this
> > code currently falls outside of the 'use strict', but seems like a good
idea)
> 
> Matt, not quite sure if I got what you mean. I guess I'm don't know enough
about
> net-internals internals:) This is where all constants are initialized, I
thought
> this is the right place.

This is where they're initialized, but not where they're declared.  Look at top
of file.

[baranovich, 2014-10-27 21:52:16]

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/net_internals/main.js (right):

https://codereview.chromium.org/423813002/diff/240001/chrome/browser/resource...
chrome/browser/resources/net_internals/main.js:315: SdchProblemCode =
Constants.sdchProblemCode;
On 2014/10/27 21:29:34, mmenke wrote:
> On 2014/10/27 20:49:24, baranovich wrote:
> > On 2014/10/03 18:59:26, mmenke wrote:
> > > Should declare this up with the others (Not currently necessary, because
> this
> > > code currently falls outside of the 'use strict', but seems like a good
> idea)
> > 
> > Matt, not quite sure if I got what you mean. I guess I'm don't know enough
> about
> > net-internals internals:) This is where all constants are initialized, I
> thought
> > this is the right place.
> 
> This is where they're initialized, but not where they're declared.  Look at
top
> of file.

Done.
Ah, thanks, my bad(

[Randy Smith (Not in Mondays), 2014-10-28 22:16:27]

Review of non-net-internals code.  I intend to review the net-internals code
too, but I'm heading home soon, and I figured a partial review return was better
than non.

It looks like the URL is still not being shown in the events list on the
URL_REQUEST for an SDCH dictionary fetch--any idea why?  That seems a minus.

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:15: #include "net/base/sdch_net_log_params.h"
Is this still needed?

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.h...
net/base/sdch_manager.h:240: // corresponding problem code.
As I read the code, it only returns SDCH_OK if there isn't a matching dictionary
or we're returning a dictionary; otherwise, it's ok.  Looking at the call site,
we're only using the return value for logging, and we're expecting a dictionary
(because we're passing in a hash we got from a response.).  So I think this can
be changed to make this interface cleaner *and* log a condition that it'd be
useful to know about in net-internals by returning a HASH_NOT_FOUND error in
that case, and just have this only return SDCH_OK if-and-only-if we found a
dictionary.  WDYT?

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.h...
net/base/sdch_manager.h:275: // dictionary_url; dictionary already added, etc.).
Would you add a line indicating that SDCH_OK will be returned on success, and
some other error code on failure?

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_problem_c...
File net/base/sdch_problem_codes.h (right):

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_problem_c...
net/base/sdch_problem_codes.h:13: SDCH_OK = 0,
Any reason for this not to be in sdch_problem_code_list.h?  Not a big deal, but
I lean towards keeping all values in the same conceptual namespace in the same
place.

https://codereview.chromium.org/423813002/diff/320001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/320001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:210: if (filter_context_.GetResponseCode() == 404) {
@baranovich: This is the beginning of the conditional cascade that Matt & I were
talking about in http://crbug.com/418975.  The information we were talking about
is what's contained in the new histograms I added below
(Sdch3.ResponseCorruptionDetection.*).  It would be useful to have that
information in the netlog (tied to a particular request) as well.  You're
welcome to skip it if you'd just like to get this CL done :-}; I added the new
histogram, and am happy to take responsibility for it and add it to
net-internals after this CL lands.

https://codereview.chromium.org/423813002/diff/320001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/423813002/diff/320001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:336: // If SDCH is just disabled, it is
not a real error.
This comment is hard to understand in the context of this code.  I think what
you're trying to say is "IsInSupportedDomain() only returns one real error,
which is DOMAIN_BLACKLIST_INCLUDES_TARGET, so that's the only that will be
logged".  But that's depending on the internals of IsINSupportedDomain() in a
way that I think violates good layering.  There are a couple of different ways
to solve this, but I think the best one is just to change the conditional to
!DISABLED && !SECURE_SCHEME_NOT_SUPPORTED and leave the comment alone.  That ok?

(Applies to the second call to IsInSupportedDomain() below as well.)

[baranovich, 2014-10-29 00:04:19]

>It looks like the URL is still not being shown in the events list on the
>URL_REQUEST for an SDCH dictionary fetch--any idea why?  That seems a minus.
Could you please describe the scenario once again? I might have missed the whole
case:(
I guess you're talking about adding an event with dictionary URL to the netlog
of the request which have initiated dictionary fetch, right? If yes, then true,
it is missing now, and I'll definitely should add it, my bad.

*other comments are acked

[Randy Smith (Not in Mondays), 2014-10-29 13:56:35]

No, I'm concerned about the URL_REQUEST for the dictionary itself.  More
detailed description:  Start up chrome, open a net-internals tab, and then (in a
different tab) do something that'll result in downloading an SDCH dictionary
(e.g. do a couple of google searches).  Find the URL_REQUEST that did the
dictionary download in the event source list (not the details tab).  Most
URL_REQUESTs have a url next to them in that list; the one that downloads the
dictionary does not.  That seems like a bug to me.

[mmenke, 2014-10-29 14:35:24]

On 2014/10/29 13:56:35, rdsmith wrote:
> No, I'm concerned about the URL_REQUEST for the dictionary itself.  More
> detailed description:  Start up chrome, open a net-internals tab, and then (in
a
> different tab) do something that'll result in downloading an SDCH dictionary
> (e.g. do a couple of google searches).  Find the URL_REQUEST that did the
> dictionary download in the event source list (not the details tab).  Most
> URL_REQUESTs have a url next to them in that list; the one that downloads the
> dictionary does not.  That seems like a bug to me.

See browser/resources/net_internals/source_entry.js, updateDescription_ method,
to see the code for getting the event with the description.  If you added an
event to the start of URLRequests, you'll have to have logic to skip over that
new line (We don't just skim over all entries until we find a description for
performance reasons, and because the initial events with the description may not
appear in the log, if there was a request in-progress when net-internals was
opened.  Code for that could still be much better, admittedly)

[baranovich, 2014-10-29 14:55:36]

On 2014/10/29 14:35:24, mmenke wrote:
> On 2014/10/29 13:56:35, rdsmith wrote:
> > No, I'm concerned about the URL_REQUEST for the dictionary itself.  More
> > detailed description:  Start up chrome, open a net-internals tab, and then
(in
> a
> > different tab) do something that'll result in downloading an SDCH dictionary
> > (e.g. do a couple of google searches).  Find the URL_REQUEST that did the
> > dictionary download in the event source list (not the details tab).  Most
> > URL_REQUESTs have a url next to them in that list; the one that downloads
the
> > dictionary does not.  That seems like a bug to me.
> 
> See browser/resources/net_internals/source_entry.js, updateDescription_
method,
> to see the code for getting the event with the description.  If you added an
> event to the start of URLRequests, you'll have to have logic to skip over that
> new line (We don't just skim over all entries until we find a description for
> performance reasons, and because the initial events with the description may
not
> appear in the log, if there was a request in-progress when net-internals was
> opened.  Code for that could still be much better, admittedly)

Yep, thanks for the directions. The quick fix I've made was to move adding
TYPE_SDCH_DICTIONARY_FETCH event below URLRequest::Start() call, since they both
occur in the same method.
If everyone will be happy with having this event after URL_REQUEST_START_JOB,
then I'll prefer not to increase entropy of the universe and add some custom
logic to SourceEntry update :)

[mmenke, 2014-10-29 14:57:07]

On 2014/10/29 14:55:36, baranovich wrote:
> On 2014/10/29 14:35:24, mmenke wrote:
> > On 2014/10/29 13:56:35, rdsmith wrote:
> > > No, I'm concerned about the URL_REQUEST for the dictionary itself.  More
> > > detailed description:  Start up chrome, open a net-internals tab, and then
> (in
> > a
> > > different tab) do something that'll result in downloading an SDCH
dictionary
> > > (e.g. do a couple of google searches).  Find the URL_REQUEST that did the
> > > dictionary download in the event source list (not the details tab).  Most
> > > URL_REQUESTs have a url next to them in that list; the one that downloads
> the
> > > dictionary does not.  That seems like a bug to me.
> > 
> > See browser/resources/net_internals/source_entry.js, updateDescription_
> method,
> > to see the code for getting the event with the description.  If you added an
> > event to the start of URLRequests, you'll have to have logic to skip over
that
> > new line (We don't just skim over all entries until we find a description
for
> > performance reasons, and because the initial events with the description may
> not
> > appear in the log, if there was a request in-progress when net-internals was
> > opened.  Code for that could still be much better, admittedly)
> 
> Yep, thanks for the directions. The quick fix I've made was to move adding
> TYPE_SDCH_DICTIONARY_FETCH event below URLRequest::Start() call, since they
both
> occur in the same method.
> If everyone will be happy with having this event after URL_REQUEST_START_JOB,
> then I'll prefer not to increase entropy of the universe and add some custom
> logic to SourceEntry update :)

That works for me - not a huge fan of all that logic, and the less cruft we have
there, the better.

[Randy Smith (Not in Mondays), 2014-10-29 16:15:46]

This is all I came up with for the net-internals code; after you handle this
stuff, Matt can come in and tell me all the things I should have caught :-} (at
least hopefully :-}).

https://codereview.chromium.org/423813002/diff/320001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/423813002/diff/320001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc:354: //
Sample domain for SDCH-view test. Dictionarties for localhost/127.0.0.1
nit: Dictionaries.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:18: var stateLis = 
tab.querySelectorAll('ul>li');
I don't think of this selector as being as "future proof" as it could be; if
anyone adds a second unordered list to the tab, this'll break.  Maybe better to
add a name and refer to it by the name?

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:87: if
(sdchInfo.dictionaries.length > 0) {
There's an anti-pattern here I want to call out, though I don't see a way to
avoid it: If the dictionary never shows up in the sdchInfo, the test will fail
by timing out, which is sub-ideal (means the test suite takes longer to run,
looks like flakiness rather than a real failure).  A better pattern is to wait
for an event that you know will happen, and that if the dictionary does get
loaded, that will happen first, then assert that the dictionary load should have
happened.  I don't actually see a way to do that, but I wanted to call out my
concern, in case you do.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:152: if
(sdchInfo.blacklisted.length > 0) {
Might there be a way to avoid the anti-pattern I mention above here?  Once the
page loads, the blacklisting should have been done; can you wait for the page
load, then assert that the blacklist has been set?

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:162:
sdchProblemCodeToString(entry.reason));
nit: Looks like these two lines could fit in one line in 80 chars?

[baranovich, 2014-10-29 18:10:21]

>@baranovich: This is the beginning of the conditional cascade that Matt & I
were
>talking about in http://crbug.com/418975.  The information we were talking
about
>is what's contained in the new histograms I added below
>(Sdch3.ResponseCorruptionDetection.*).  It would be useful to have that
>information in the netlog (tied to a particular request) as well.  You're
>welcome to skip it if you'd just like to get this CL done :-}; I added the new
>histogram, and am happy to take responsibility for it and add it to
>net-internals after this CL lands.

Despite the fact, I really want to get this CL done ;-), it seems to me that
ResponseCorruptionDetectionCause duplicates ProblemCodes for the perspective of
the netlog. At least if you assume that all prior information is already in the
URLRequest netlog.
I mean, we have HTTP_CACHE* events to know if the response was cached or not, we
have previous SDCH errors indicating that we have fixed content encoding (to
know if we do tentative decoding), and we have the immediate error from
SdchFilter (or prior errors from parsing server hash during dictionary init). We
can reconstruct what was going on there. I may be wrong, but I haven't found
cases which are covered uniquely by ResponseCorruptionDetectionCause histogram.

[baranovich, 2014-10-29 18:28:43]

https://codereview.chromium.org/423813002/diff/320001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/423813002/diff/320001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc:354: //
Sample domain for SDCH-view test. Dictionarties for localhost/127.0.0.1
On 2014/10/29 16:15:45, rdsmith wrote:
> nit: Dictionaries.

Done.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:18: var stateLis = 
tab.querySelectorAll('ul>li');
On 2014/10/29 16:15:46, rdsmith wrote:
> I don't think of this selector as being as "future proof" as it could be; if
> anyone adds a second unordered list to the tab, this'll break.  Maybe better
to
> add a name and refer to it by the name?

Done.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:87: if
(sdchInfo.dictionaries.length > 0) {
On 2014/10/29 16:15:46, rdsmith wrote:
> There's an anti-pattern here I want to call out, though I don't see a way to
> avoid it: If the dictionary never shows up in the sdchInfo, the test will fail
> by timing out, which is sub-ideal (means the test suite takes longer to run,
> looks like flakiness rather than a real failure).  A better pattern is to wait
> for an event that you know will happen, and that if the dictionary does get
> loaded, that will happen first, then assert that the dictionary load should
have
> happened.  I don't actually see a way to do that, but I wanted to call out my
> concern, in case you do.

I don't like that either, but this approach is used everywhere else in
net-internals tests. The last hope is that Matt may know some secret =)

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:162:
sdchProblemCodeToString(entry.reason));
On 2014/10/29 16:15:46, rdsmith wrote:
> nit: Looks like these two lines could fit in one line in 80 chars?

Done.

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.c...
net/base/sdch_manager.cc:15: #include "net/base/sdch_net_log_params.h"
On 2014/10/28 22:16:27, rdsmith wrote:
> Is this still needed?

Done.

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.h...
net/base/sdch_manager.h:240: // corresponding problem code.
On 2014/10/28 22:16:27, rdsmith wrote:
> As I read the code, it only returns SDCH_OK if there isn't a matching
dictionary
> or we're returning a dictionary; otherwise, it's ok.  Looking at the call
site,
> we're only using the return value for logging, and we're expecting a
dictionary
> (because we're passing in a hash we got from a response.).  So I think this
can
> be changed to make this interface cleaner *and* log a condition that it'd be
> useful to know about in net-internals by returning a HASH_NOT_FOUND error in
> that case, and just have this only return SDCH_OK if-and-only-if we found a
> dictionary.  WDYT?
Done. Although code on the callsite changed a little bit.

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_manager.h...
net/base/sdch_manager.h:275: // dictionary_url; dictionary already added, etc.).
On 2014/10/28 22:16:27, rdsmith wrote:
> Would you add a line indicating that SDCH_OK will be returned on success, and
> some other error code on failure?

Done.

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_problem_c...
File net/base/sdch_problem_codes.h (right):

https://codereview.chromium.org/423813002/diff/320001/net/base/sdch_problem_c...
net/base/sdch_problem_codes.h:13: SDCH_OK = 0,
On 2014/10/28 22:16:27, rdsmith wrote:
> Any reason for this not to be in sdch_problem_code_list.h?  Not a big deal,
but
> I lean towards keeping all values in the same conceptual namespace in the same
> place.

Done.

https://codereview.chromium.org/423813002/diff/320001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/423813002/diff/320001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:336: // If SDCH is just disabled, it is
not a real error.
On 2014/10/28 22:16:27, rdsmith wrote:
> This comment is hard to understand in the context of this code.  I think what
> you're trying to say is "IsInSupportedDomain() only returns one real error,
> which is DOMAIN_BLACKLIST_INCLUDES_TARGET, so that's the only that will be
> logged".  But that's depending on the internals of IsINSupportedDomain() in a
> way that I think violates good layering.  There are a couple of different ways
> to solve this, but I think the best one is just to change the conditional to
> !DISABLED && !SECURE_SCHEME_NOT_SUPPORTED and leave the comment alone.  That
ok?
> 
> (Applies to the second call to IsInSupportedDomain() below as well.)

Done.

[mmenke, 2014-10-29 22:03:21]

A couple comments on tests.  I'll do a full pass tomorrow.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:87: if
(sdchInfo.dictionaries.length > 0) {
On 2014/10/29 18:28:43, baranovich wrote:
> On 2014/10/29 16:15:46, rdsmith wrote:
> > There's an anti-pattern here I want to call out, though I don't see a way to
> > avoid it: If the dictionary never shows up in the sdchInfo, the test will
fail
> > by timing out, which is sub-ideal (means the test suite takes longer to run,
> > looks like flakiness rather than a real failure).  A better pattern is to
wait
> > for an event that you know will happen, and that if the dictionary does get
> > loaded, that will happen first, then assert that the dictionary load should
> have
> > happened.  I don't actually see a way to do that, but I wanted to call out
my
> > concern, in case you do.
> 
> I don't like that either, but this approach is used everywhere else in
> net-internals tests. The last hope is that Matt may know some secret =)

This is a valid point.  I suppose we could wait on the NetLog URLRequest events
to complete.  But we may get that event before the updated dictionary list, so
to make sure that doesn't happen, we'd have to wait for that events, and then
wait to receive updated SDCH info...Of course, if it matches the previous info,
we'd need to hook into the browser bridge, as it won't send us non-updated info.
 And then we're relying on the dictionary list being updated synchronously
relative the end URLRequest REQUEST_ALIVE event...  I think that would all work
out, but it's depending on code far enough removed from net-internals that it
seems like an even worse anti-pattern than timing out on tests.

Waiting for something not to happen and then failing (Or passing, depending on
the test) is a pretty general problem we run into a lot, but there's not a
general solution, unless there's another thing we can also wait on, as Randy
suggest (Of course, if we don't get *that* event either, we still timeout,
unless we have yet another event to wait on....)

Anyhow, I agree that it's a problem, but not sure there's a reasonable solution
to it here.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:88: var testDict =
sdchInfo.dictionaries.filter(function(e) {
nit:  Per the style guide, don't use single-letter variable names.  Write out a
full word (dictionary?).  Does mean this will take up an extra line, but not the
end of the world.

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:152: if
(sdchInfo.blacklisted.length > 0) {
On 2014/10/29 16:15:46, rdsmith wrote:
> Might there be a way to avoid the anti-pattern I mention above here?  Once the
> page loads, the blacklisting should have been done; can you wait for the page
> load, then assert that the blacklist has been set?

The blacklist is pulled, not pushed, so not quite that simple.  Also, the
dictionaries go IO->UI->IO->renderer...  The main request goes
IO->Renderer->IO->UI, so that is presumably a race it's impossible to win,
but...ergh.  I don't want to have to think about that when reading this test.

[mmenke, 2014-10-29 22:20:44]

https://codereview.chromium.org/423813002/diff/340001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/423813002/diff/340001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:619:
&IOThreadImpl::CallbackHelper, &IOThreadImpl::OnGetSdchInfo, proxy_));
Note:  I added a new file net/base/net_log_util with a single function to
retrieve a bunch of info, that takes as input a bunch of flags.  Should move
this stuff there.  Should be a fairly minor change, but have to modify the
Javascript browser_bridge code slightly, too.

Forgot it would affect this CL.

One trickiness is that when there's no sdch_manager, we'll still have to return
something, with the new ways things are done (base::Dictionary doesn't support
NULLs, I believe).



The motivation for that code is to allow creating full log dumps without
net-internals.

[mmenke, 2014-10-29 22:51:14]

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:87: if
(sdchInfo.dictionaries.length > 0) {
On 2014/10/29 22:03:21, mmenke wrote:
> On 2014/10/29 18:28:43, baranovich wrote:
> > On 2014/10/29 16:15:46, rdsmith wrote:
> > > There's an anti-pattern here I want to call out, though I don't see a way
to
> > > avoid it: If the dictionary never shows up in the sdchInfo, the test will
> fail
> > > by timing out, which is sub-ideal (means the test suite takes longer to
run,
> > > looks like flakiness rather than a real failure).  A better pattern is to
> wait
> > > for an event that you know will happen, and that if the dictionary does
get
> > > loaded, that will happen first, then assert that the dictionary load
should
> > have
> > > happened.  I don't actually see a way to do that, but I wanted to call out
> my
> > > concern, in case you do.
> > 
> > I don't like that either, but this approach is used everywhere else in
> > net-internals tests. The last hope is that Matt may know some secret =)
> 
> This is a valid point.  I suppose we could wait on the NetLog URLRequest
events
> to complete.  But we may get that event before the updated dictionary list, so
> to make sure that doesn't happen, we'd have to wait for that events, and then
> wait to receive updated SDCH info...Of course, if it matches the previous
info,
> we'd need to hook into the browser bridge, as it won't send us non-updated
info.
>  And then we're relying on the dictionary list being updated synchronously
> relative the end URLRequest REQUEST_ALIVE event...  I think that would all
work
> out, but it's depending on code far enough removed from net-internals that it
> seems like an even worse anti-pattern than timing out on tests.
> 
> Waiting for something not to happen and then failing (Or passing, depending on
> the test) is a pretty general problem we run into a lot, but there's not a
> general solution, unless there's another thing we can also wait on, as Randy
> suggest (Of course, if we don't get *that* event either, we still timeout,
> unless we have yet another event to wait on....)
> 
> Anyhow, I agree that it's a problem, but not sure there's a reasonable
solution
> to it here.

Thinking about it a bit more, waiting on the NetLog event may not be safe - we
may be able to get a dictionary older than a NetLog event after we see the
event, I'm not sure.  Navigation, on the other hand, should not have that issue,
and it should be fairly safe to depend on that.

There's also an option to receive notifications even when the value didn't
actually change, I believe, so we could wait for load to complete on tests that
expect errors, though it still makes me a little nervous.  That wouldn't work
for tests that expect to get dictionaries successfully, though.

[baranovich, 2014-10-30 00:33:22]

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:88: var testDict =
sdchInfo.dictionaries.filter(function(e) {
On 2014/10/29 22:03:21, mmenke wrote:
> nit:  Per the style guide, don't use single-letter variable names.  Write out
a
> full word (dictionary?).  Does mean this will take up an extra line, but not
the
> end of the world.

Done.

[baranovich, 2014-10-30 00:38:10]

On 2014/10/29 22:51:14, mmenke wrote:
>
https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
> File chrome/test/data/webui/net_internals/sdch_view.js (right):
> 
>
https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
> chrome/test/data/webui/net_internals/sdch_view.js:87: if
> (sdchInfo.dictionaries.length > 0) {
> On 2014/10/29 22:03:21, mmenke wrote:
> > On 2014/10/29 18:28:43, baranovich wrote:
> > > On 2014/10/29 16:15:46, rdsmith wrote:
> > > > There's an anti-pattern here I want to call out, though I don't see a
way
> to
> > > > avoid it: If the dictionary never shows up in the sdchInfo, the test
will
> > fail
> > > > by timing out, which is sub-ideal (means the test suite takes longer to
> run,
> > > > looks like flakiness rather than a real failure).  A better pattern is
to
> > wait
> > > > for an event that you know will happen, and that if the dictionary does
> get
> > > > loaded, that will happen first, then assert that the dictionary load
> should
> > > have
> > > > happened.  I don't actually see a way to do that, but I wanted to call
out
> > my
> > > > concern, in case you do.
> > > 
> > > I don't like that either, but this approach is used everywhere else in
> > > net-internals tests. The last hope is that Matt may know some secret =)
> > 
> > This is a valid point.  I suppose we could wait on the NetLog URLRequest
> events
> > to complete.  But we may get that event before the updated dictionary list,
so
> > to make sure that doesn't happen, we'd have to wait for that events, and
then
> > wait to receive updated SDCH info...Of course, if it matches the previous
> info,
> > we'd need to hook into the browser bridge, as it won't send us non-updated
> info.
> >  And then we're relying on the dictionary list being updated synchronously
> > relative the end URLRequest REQUEST_ALIVE event...  I think that would all
> work
> > out, but it's depending on code far enough removed from net-internals that
it
> > seems like an even worse anti-pattern than timing out on tests.
> > 
> > Waiting for something not to happen and then failing (Or passing, depending
on
> > the test) is a pretty general problem we run into a lot, but there's not a
> > general solution, unless there's another thing we can also wait on, as Randy
> > suggest (Of course, if we don't get *that* event either, we still timeout,
> > unless we have yet another event to wait on....)
> > 
> > Anyhow, I agree that it's a problem, but not sure there's a reasonable
> solution
> > to it here.
> 
> Thinking about it a bit more, waiting on the NetLog event may not be safe - we
> may be able to get a dictionary older than a NetLog event after we see the
> event, I'm not sure.  Navigation, on the other hand, should not have that
issue,
> and it should be fairly safe to depend on that.
> 
> There's also an option to receive notifications even when the value didn't
> actually change, I believe, so we could wait for load to complete on tests
that
> expect errors, though it still makes me a little nervous.  That wouldn't work
> for tests that expect to get dictionaries successfully, though.

Done with moving to net_log_util

So what will be the final decision on what the tests wait? Hook the navigation
end for error tests and wait for netlog event for dictionary fetch? Or make it
all-or-nothing and leave everything as is?

[mmenke, 2014-10-30 00:49:11]

On 2014/10/30 00:38:10, baranovich wrote:
> On 2014/10/29 22:51:14, mmenke wrote:
> >
>
https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
> > File chrome/test/data/webui/net_internals/sdch_view.js (right):
> > 
> >
>
https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
> > chrome/test/data/webui/net_internals/sdch_view.js:87: if
> > (sdchInfo.dictionaries.length > 0) {
> > On 2014/10/29 22:03:21, mmenke wrote:
> > > On 2014/10/29 18:28:43, baranovich wrote:
> > > > On 2014/10/29 16:15:46, rdsmith wrote:
> > > > > There's an anti-pattern here I want to call out, though I don't see a
> way
> > to
> > > > > avoid it: If the dictionary never shows up in the sdchInfo, the test
> will
> > > fail
> > > > > by timing out, which is sub-ideal (means the test suite takes longer
to
> > run,
> > > > > looks like flakiness rather than a real failure).  A better pattern is
> to
> > > wait
> > > > > for an event that you know will happen, and that if the dictionary
does
> > get
> > > > > loaded, that will happen first, then assert that the dictionary load
> > should
> > > > have
> > > > > happened.  I don't actually see a way to do that, but I wanted to call
> out
> > > my
> > > > > concern, in case you do.
> > > > 
> > > > I don't like that either, but this approach is used everywhere else in
> > > > net-internals tests. The last hope is that Matt may know some secret =)
> > > 
> > > This is a valid point.  I suppose we could wait on the NetLog URLRequest
> > events
> > > to complete.  But we may get that event before the updated dictionary
list,
> so
> > > to make sure that doesn't happen, we'd have to wait for that events, and
> then
> > > wait to receive updated SDCH info...Of course, if it matches the previous
> > info,
> > > we'd need to hook into the browser bridge, as it won't send us non-updated
> > info.
> > >  And then we're relying on the dictionary list being updated synchronously
> > > relative the end URLRequest REQUEST_ALIVE event...  I think that would all
> > work
> > > out, but it's depending on code far enough removed from net-internals that
> it
> > > seems like an even worse anti-pattern than timing out on tests.
> > > 
> > > Waiting for something not to happen and then failing (Or passing,
depending
> on
> > > the test) is a pretty general problem we run into a lot, but there's not a
> > > general solution, unless there's another thing we can also wait on, as
Randy
> > > suggest (Of course, if we don't get *that* event either, we still timeout,
> > > unless we have yet another event to wait on....)
> > > 
> > > Anyhow, I agree that it's a problem, but not sure there's a reasonable
> > solution
> > > to it here.
> > 
> > Thinking about it a bit more, waiting on the NetLog event may not be safe -
we
> > may be able to get a dictionary older than a NetLog event after we see the
> > event, I'm not sure.  Navigation, on the other hand, should not have that
> issue,
> > and it should be fairly safe to depend on that.
> > 
> > There's also an option to receive notifications even when the value didn't
> > actually change, I believe, so we could wait for load to complete on tests
> that
> > expect errors, though it still makes me a little nervous.  That wouldn't
work
> > for tests that expect to get dictionaries successfully, though.
> 
> Done with moving to net_log_util
> 
> So what will be the final decision on what the tests wait? Hook the navigation
> end for error tests and wait for netlog event for dictionary fetch? Or make it
> all-or-nothing and leave everything as is?

Waiting for NetLog has me very concerned about flake.  I'd say the non-error
tests we should definitely just leave them as-is.  I'd rather have a test that
hangs on regression than one that fails (flakily) on changes that aren't
regressions.

We could safely wait on the failure tests, but I'm not sure it's worth the
effort - Randy seems to feel more strongly about this than I do.  If too many
tests fail, we just abort the entire tryjob now, so I don't think it's quite the
concern it once was, at least for browser tests.

[mmenke, 2014-10-30 21:18:52]

Sorry for the slow response.  From a NetLog standpoint, this LGTM.  Please wait
for rdsmith's signoff before landed.

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
File chrome/browser/resources/net_internals/browser_bridge.js (right):

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
chrome/browser/resources/net_internals/browser_bridge.js:214: },
No longer needed.

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
chrome/browser/resources/net_internals/browser_bridge.js:365: receivedSdchInfo:
function(sdchInfo) {
No longer needed.

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.js:51: if (!sdchInfo)
Since sdchInfo can be an empty dictionary now, this should probably be:

if (!sdchInfo || typeof(x.sdch_enabled) != 'undefined')

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:35: }
Not sure this gets you anything that the following does not:

var text = $(SdchView.BLACKLIST_TBODY_ID).innerText;
expectFalse(/undefined/i.test(text));

My concern here is the case if/when new columns are added.  Same goes for the
other table.

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:65: // 127.0.0.1 is not
allowed to be an SDCH domain, so we need another one.
nit:  Don't use "we" in comments, as it's generally ambiguous (Goes for this
whole file).

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:141: * @param {object}
sdchInfo Results a SDCH manager info query.
nit:  Results -> Results of

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:170: * Load a page, which
provide SDCH dictionary. Make sure we have its data
nit:  "Which provides"  (Or "which results in downloading a"), and "Make sure
its data is displayed."

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_event_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2427: //     "sdch_problem_code": <SDCH
problem code>
Add something like:  "net_error":  <Always ERR_FAILED, present just to indicate
this is a failure>

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2437: //                   with red>,
Probably shouldn't mention net-internals in net/, since net-internals is at a
higher layer.  Also, should indicate when the field is and isn't present.  Maybe
something like:

"net_error":  <Only present on unexpected errors.  
Always ERR_FAILED when present.  Used to indicate this is a real failure>

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_util.cc
File net/base/net_log_util.cc (right):

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_util.c...
net/base/net_log_util.cc:477: base::Value* info_dict = nullptr;
nit:  No need to initialize.  We're in trouble if we call DictionaryValue::Set
with a null pointer, anyways, I believe.

[Randy Smith (Not in Mondays), 2014-10-31 18:23:15]

On 2014/10/30 00:49:11, mmenke wrote:
> On 2014/10/30 00:38:10, baranovich wrote:
> > On 2014/10/29 22:51:14, mmenke wrote:
> > >
> >
>
https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
> > > File chrome/test/data/webui/net_internals/sdch_view.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/423813002/diff/320001/chrome/test/data/webui/...
> > > chrome/test/data/webui/net_internals/sdch_view.js:87: if
> > > (sdchInfo.dictionaries.length > 0) {
> > > On 2014/10/29 22:03:21, mmenke wrote:
> > > > On 2014/10/29 18:28:43, baranovich wrote:
> > > > > On 2014/10/29 16:15:46, rdsmith wrote:
> > > > > > There's an anti-pattern here I want to call out, though I don't see
a
> > way
> > > to
> > > > > > avoid it: If the dictionary never shows up in the sdchInfo, the test
> > will
> > > > fail
> > > > > > by timing out, which is sub-ideal (means the test suite takes longer
> to
> > > run,
> > > > > > looks like flakiness rather than a real failure).  A better pattern
is
> > to
> > > > wait
> > > > > > for an event that you know will happen, and that if the dictionary
> does
> > > get
> > > > > > loaded, that will happen first, then assert that the dictionary load
> > > should
> > > > > have
> > > > > > happened.  I don't actually see a way to do that, but I wanted to
call
> > out
> > > > my
> > > > > > concern, in case you do.
> > > > > 
> > > > > I don't like that either, but this approach is used everywhere else in
> > > > > net-internals tests. The last hope is that Matt may know some secret
=)
> > > > 
> > > > This is a valid point.  I suppose we could wait on the NetLog URLRequest
> > > events
> > > > to complete.  But we may get that event before the updated dictionary
> list,
> > so
> > > > to make sure that doesn't happen, we'd have to wait for that events, and
> > then
> > > > wait to receive updated SDCH info...Of course, if it matches the
previous
> > > info,
> > > > we'd need to hook into the browser bridge, as it won't send us
non-updated
> > > info.
> > > >  And then we're relying on the dictionary list being updated
synchronously
> > > > relative the end URLRequest REQUEST_ALIVE event...  I think that would
all
> > > work
> > > > out, but it's depending on code far enough removed from net-internals
that
> > it
> > > > seems like an even worse anti-pattern than timing out on tests.
> > > > 
> > > > Waiting for something not to happen and then failing (Or passing,
> depending
> > on
> > > > the test) is a pretty general problem we run into a lot, but there's not
a
> > > > general solution, unless there's another thing we can also wait on, as
> Randy
> > > > suggest (Of course, if we don't get *that* event either, we still
timeout,
> > > > unless we have yet another event to wait on....)
> > > > 
> > > > Anyhow, I agree that it's a problem, but not sure there's a reasonable
> > > solution
> > > > to it here.
> > > 
> > > Thinking about it a bit more, waiting on the NetLog event may not be safe
-
> we
> > > may be able to get a dictionary older than a NetLog event after we see the
> > > event, I'm not sure.  Navigation, on the other hand, should not have that
> > issue,
> > > and it should be fairly safe to depend on that.
> > > 
> > > There's also an option to receive notifications even when the value didn't
> > > actually change, I believe, so we could wait for load to complete on tests
> > that
> > > expect errors, though it still makes me a little nervous.  That wouldn't
> work
> > > for tests that expect to get dictionaries successfully, though.
> > 
> > Done with moving to net_log_util
> > 
> > So what will be the final decision on what the tests wait? Hook the
navigation
> > end for error tests and wait for netlog event for dictionary fetch? Or make
it
> > all-or-nothing and leave everything as is?
> 
> Waiting for NetLog has me very concerned about flake.  I'd say the non-error
> tests we should definitely just leave them as-is.  I'd rather have a test that
> hangs on regression than one that fails (flakily) on changes that aren't
> regressions.
> 
> We could safely wait on the failure tests, but I'm not sure it's worth the
> effort - Randy seems to feel more strongly about this than I do.  If too many
> tests fail, we just abort the entire tryjob now, so I don't think it's quite
the
> concern it once was, at least for browser tests.

I'm ok with leaving them as is.  I want to be in the habit of looking for better
ways to handle this case, but I understand there often may not be such ways.

[Randy Smith (Not in Mondays), 2014-10-31 18:43:22]

On 2014/10/29 18:10:21, baranovich wrote:
> >@baranovich: This is the beginning of the conditional cascade that Matt & I
> were
> >talking about in http://crbug.com/418975.  The information we were talking
> about
> >is what's contained in the new histograms I added below
> >(Sdch3.ResponseCorruptionDetection.*).  It would be useful to have that
> >information in the netlog (tied to a particular request) as well.  You're
> >welcome to skip it if you'd just like to get this CL done :-}; I added the
new
> >histogram, and am happy to take responsibility for it and add it to
> >net-internals after this CL lands.
> 
> Despite the fact, I really want to get this CL done ;-), it seems to me that
> ResponseCorruptionDetectionCause duplicates ProblemCodes for the perspective
of
> the netlog. At least if you assume that all prior information is already in
the
> URLRequest netlog.
> I mean, we have HTTP_CACHE* events to know if the response was cached or not,
we
> have previous SDCH errors indicating that we have fixed content encoding (to
> know if we do tentative decoding), and we have the immediate error from
> SdchFilter (or prior errors from parsing server hash during dictionary init).
We
> can reconstruct what was going on there. I may be wrong, but I haven't found
> cases which are covered uniquely by ResponseCorruptionDetectionCause
histogram.

There is some information lost; how important you consider that probably has
something to do with how much pain you've had in dealing with that particular
conditional cascade :-}.  For several condition results, the only thing we do is
set cause (see the "if (possible_pass_thru_)" and "if
(dictionary_hash_is_plausible_)" sections), and I think it would be useful to
have a record of that in the netlog.  But I"m also happy to do it after you land
this CL.

[Randy Smith (Not in Mondays), 2014-10-31 19:05:35]

LGTM with nit below.  Thanks very much for doing this!

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:403: if (rv != SDCH_OK) {
I think you can skip this check; if rv == SDCH_DICTIONARY_HASH_NOT_FOUND, it's
not going to be SDCH_OK.

[baranovich, 2014-10-31 19:40:18]

baranovich@yandex-team.ru changed reviewers:
+ jwd@chromium.org

[baranovich, 2014-10-31 19:55:17]

Randy, I've added logging of the ResponseCorruptionDetection, PTAL once more.

[baranovich, 2014-10-31 19:55:33]

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
File chrome/browser/resources/net_internals/browser_bridge.js (right):

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
chrome/browser/resources/net_internals/browser_bridge.js:214: },
On 2014/10/30 21:18:52, mmenke wrote:
> No longer needed.

Done.

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
chrome/browser/resources/net_internals/browser_bridge.js:365: receivedSdchInfo:
function(sdchInfo) {
On 2014/10/30 21:18:52, mmenke wrote:
> No longer needed.

Done.

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
File chrome/browser/resources/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/380001/chrome/browser/resource...
chrome/browser/resources/net_internals/sdch_view.js:51: if (!sdchInfo)
On 2014/10/30 21:18:52, mmenke wrote:
> Since sdchInfo can be an empty dictionary now, this should probably be:
> 
> if (!sdchInfo || typeof(x.sdch_enabled) != 'undefined')

Done.

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
File chrome/test/data/webui/net_internals/sdch_view.js (right):

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:35: }
On 2014/10/30 21:18:52, mmenke wrote:
> Not sure this gets you anything that the following does not:
> 
> var text = $(SdchView.BLACKLIST_TBODY_ID).innerText;
> expectFalse(/undefined/i.test(text));
> 
> My concern here is the case if/when new columns are added.  Same goes for the
> other table.

Added more checks to so this code is more reasonable now.

Regarding new columns, I would suggest testing that the tables have the expected
number of columns in each row, but there are no such helpers in net-internals
tests. Seems like this was never tested in any test, though there are plenty of
tables in net-internals. Imho it would be useful to add function to get the
number of columns for the row, but then we need a helper to get a row from the
table... not a rocket science of course, but a matter for another CL I think.

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:65: // 127.0.0.1 is not
allowed to be an SDCH domain, so we need another one.
On 2014/10/30 21:18:52, mmenke wrote:
> nit:  Don't use "we" in comments, as it's generally ambiguous (Goes for this
> whole file).

Done.

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:141: * @param {object}
sdchInfo Results a SDCH manager info query.
On 2014/10/30 21:18:52, mmenke wrote:
> nit:  Results -> Results of

Done.

https://codereview.chromium.org/423813002/diff/380001/chrome/test/data/webui/...
chrome/test/data/webui/net_internals/sdch_view.js:170: * Load a page, which
provide SDCH dictionary. Make sure we have its data
On 2014/10/30 21:18:52, mmenke wrote:
> nit:  "Which provides"  (Or "which results in downloading a"), and "Make sure
> its data is displayed."

Done.

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_event_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2427: //     "sdch_problem_code": <SDCH
problem code>
On 2014/10/30 21:18:52, mmenke wrote:
> Add something like:  "net_error":  <Always ERR_FAILED, present just to
indicate
> this is a failure>

Done.

https://codereview.chromium.org/423813002/diff/380001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2437: //                   with red>,
On 2014/10/30 21:18:52, mmenke wrote:
> Probably shouldn't mention net-internals in net/, since net-internals is at a
> higher layer.  Also, should indicate when the field is and isn't present. 
Maybe
> something like:
> 
> "net_error":  <Only present on unexpected errors.  
> Always ERR_FAILED when present.  Used to indicate this is a real failure>

Done.

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:403: if (rv != SDCH_OK) {
On 2014/10/31 19:05:35, rdsmith wrote:
> I think you can skip this check; if rv == SDCH_DICTIONARY_HASH_NOT_FOUND, it's
> not going to be SDCH_OK.

True, but it may also be SDCH_DISABLED, SDCH_*_BLACKLIST_* or SDCH_SECURE_*

[jwd, 2014-11-03 18:35:28]

https://codereview.chromium.org/423813002/diff/400001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/423813002/diff/400001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29987: +    Sdch3.ProblemCodes_5 used
instead.
Can you add the date month/year for these obsolete tags.

[baranovich, 2014-11-03 18:48:40]

https://codereview.chromium.org/423813002/diff/400001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/423813002/diff/400001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29987: +    Sdch3.ProblemCodes_5 used
instead.
On 2014/11/03 18:35:27, Jesse Doherty wrote:
> Can you add the date month/year for these obsolete tags.

Done.

[jwd, 2014-11-03 19:41:14]

lgtm

[baranovich, 2014-11-03 20:52:07]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-11-03 20:53:08]

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

[mmenke, 2014-11-03 21:01:05]

https://codereview.chromium.org/423813002/diff/420001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/420001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:75: return "<unknown>";
If you remove the default case, and move the return below the switch statement,
clang will give a compile error when new causes are added, without updating this
list.

[Randy Smith (Not in Mondays), 2014-11-03 21:01:27]

LGTM with changes below.

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:403: if (rv != SDCH_OK) {
On 2014/10/31 19:55:33, baranovich wrote:
> On 2014/10/31 19:05:35, rdsmith wrote:
> > I think you can skip this check; if rv == SDCH_DICTIONARY_HASH_NOT_FOUND,
it's
> > not going to be SDCH_OK.
> 
> True, but it may also be SDCH_DISABLED, SDCH_*_BLACKLIST_* or SDCH_SECURE_*
> 

Sorry, I may be being dense, but how does the effect of the code change if you
remove the "if (rv != SDCH_OK)"?  If it's one of the others you mentioned, the
code inside the SDCH_DICTIONARY_HASH_NOT_FOUND will still not be executed.

https://codereview.chromium.org/423813002/diff/400001/net/base/net_info_sourc...
File net/base/net_info_source_list.h (right):

https://codereview.chromium.org/423813002/diff/400001/net/base/net_info_sourc...
net/base/net_info_source_list.h:23: NET_INFO_SOURCE(SDCH, "sdchInfo", 1 << 9)
This looks like a style typo (inconsistent with the rest of the file).

Do we still need an SDCH source?

https://codereview.chromium.org/423813002/diff/400001/net/base/net_log_event_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/423813002/diff/400001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2439: //                   failure>,
I think I'd suggest leaving out the net_error/ERR_FAILED on this one.  There are
aspects of that conditional cascade that turn into non-failures (e.g. returning
an unencoded response from the cache), and I think any real failures are noted
through LogSdchError() elsewhere in the function.

[baranovich, 2014-11-03 22:12:19]

The CQ bit was unchecked by baranovich@yandex-team.ru

[baranovich, 2014-11-03 22:34:23]

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/380001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:403: if (rv != SDCH_OK) {
On 2014/11/03 21:01:27, rdsmith wrote:
> On 2014/10/31 19:55:33, baranovich wrote:
> > On 2014/10/31 19:05:35, rdsmith wrote:
> > > I think you can skip this check; if rv == SDCH_DICTIONARY_HASH_NOT_FOUND,
> it's
> > > not going to be SDCH_OK.
> > 
> > True, but it may also be SDCH_DISABLED, SDCH_*_BLACKLIST_* or SDCH_SECURE_*
> > 
> 
> Sorry, I may be being dense, but how does the effect of the code change if you
> remove the "if (rv != SDCH_OK)"?  If it's one of the others you mentioned, the
> code inside the SDCH_DICTIONARY_HASH_NOT_FOUND will still not be executed.  

Got it finally, sorry for being so slow:) Done.

https://codereview.chromium.org/423813002/diff/400001/net/base/net_info_sourc...
File net/base/net_info_source_list.h (right):

https://codereview.chromium.org/423813002/diff/400001/net/base/net_info_sourc...
net/base/net_info_source_list.h:23: NET_INFO_SOURCE(SDCH, "sdchInfo", 1 << 9)
On 2014/11/03 21:01:27, rdsmith wrote:
> This looks like a style typo (inconsistent with the rest of the file).
> 
> Do we still need an SDCH source?  

Done with formatting. We need SDCH source, it is used in GetNetInfo() to decide
what to update

https://codereview.chromium.org/423813002/diff/400001/net/base/net_log_event_...
File net/base/net_log_event_type_list.h (right):

https://codereview.chromium.org/423813002/diff/400001/net/base/net_log_event_...
net/base/net_log_event_type_list.h:2439: //                   failure>,
On 2014/11/03 21:01:27, rdsmith wrote:
> I think I'd suggest leaving out the net_error/ERR_FAILED on this one.  There
are
> aspects of that conditional cascade that turn into non-failures (e.g.
returning
> an unencoded response from the cache), and I think any real failures are noted
> through LogSdchError() elsewhere in the function.

Done.

https://codereview.chromium.org/423813002/diff/420001/net/filter/sdch_filter.cc
File net/filter/sdch_filter.cc (right):

https://codereview.chromium.org/423813002/diff/420001/net/filter/sdch_filter....
net/filter/sdch_filter.cc:75: return "<unknown>";
On 2014/11/03 21:01:04, mmenke wrote:
> If you remove the default case, and move the return below the switch
statement,
> clang will give a compile error when new causes are added, without updating
this
> list.

Done.

[baranovich, 2014-11-03 23:36:38]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-11-03 23:37:22]

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

[baranovich, 2014-11-04 00:27:55]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-11-04 00:29:37]

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

[commit-bot: I haz the power, 2014-11-04 02:04:25]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2014-11-04 02:05:08]

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

[tzik, 2014-11-04 04:14:15]

This CL seems to break netInternalsSdchViewBlacklistNonSdch.
Let me revert it as https://codereview.chromium.org/704493002/.

[tzik, 2014-11-04 04:19:51]

Well, the test stops failing without the revert.
I'll hold my revert CL and see the next cycle.

[tzik, 2014-11-04 06:34:32]

The test has been flaky.
Let me land the revert CL.

[baranovich, 2014-11-13 17:22:09]

Don't look it this changes yet. Uploaded without thinkng:)

[baranovich, 2014-11-13 18:43:00]

Hopefully fixed test flakes and adopted new Sdch changes about SdchObserver

[Randy Smith (Not in Mondays), 2014-11-13 18:54:24]

LGTM with nit.

https://codereview.chromium.org/423813002/diff/500001/net/base/net_info_sourc...
File net/base/net_info_source_list.h (right):

https://codereview.chromium.org/423813002/diff/500001/net/base/net_info_sourc...
net/base/net_info_source_list.h:23: NET_INFO_SOURCE(SDCH, "sdchInfo", 1 << 9)
nit: Align 1 << 9 with previous values.

[mmenke, 2014-11-13 19:01:49]

On 2014/11/13 18:54:24, rdsmith wrote:
> LGTM with nit.
> 
>
https://codereview.chromium.org/423813002/diff/500001/net/base/net_info_sourc...
> File net/base/net_info_source_list.h (right):
> 
>
https://codereview.chromium.org/423813002/diff/500001/net/base/net_info_sourc...
> net/base/net_info_source_list.h:23: NET_INFO_SOURCE(SDCH, "sdchInfo", 1 << 9)
> nit: Align 1 << 9 with previous values.

LGTM (Glanced at tests, but largely deferring to randy)

[baranovich, 2014-11-13 19:32:05]

https://codereview.chromium.org/423813002/diff/500001/net/base/net_info_sourc...
File net/base/net_info_source_list.h (right):

https://codereview.chromium.org/423813002/diff/500001/net/base/net_info_sourc...
net/base/net_info_source_list.h:23: NET_INFO_SOURCE(SDCH, "sdchInfo", 1 << 9)
On 2014/11/13 18:54:24, rdsmith wrote:
> nit: Align 1 << 9 with previous values.

Done. I guess its git cl format. I remember that I did that indents:)

[baranovich, 2014-11-13 20:10:59]

jwd@ PTAL

[baranovich, 2014-11-14 00:23:39]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-11-14 00:26:22]

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

[commit-bot: I haz the power, 2014-11-14 01:05:42]

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

[commit-bot: I haz the power, 2014-11-14 01:05:44]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[baranovich, 2014-11-14 01:16:01]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-11-14 01:17:37]

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

[commit-bot: I haz the power, 2014-11-14 03:08:35]

Committed patchset #28 (id:540001)

[commit-bot: I haz the power, 2014-11-14 03:09:20]

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

[baranovich, 2015-01-13 12:35:51]

baranovich@yandex-team.ru changed reviewers:
+ jschuh@chromium.org

[baranovich, 2015-01-13 12:36:25]

baranovich@yandex-team.ru changed reviewers:
- jschuh@chromium.org