Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher.

Change SDCHDictionaryFetcher to use URLRequest instead of URLFetcher.

Also shifts mechanism for minimizing interference with top-level traffic
from delaying dispatch to using the IDLE request priority.

BUG=387890
BUG=383404
R=rsleevi@chromium.org

Committed: https://crrev.com/1df5b71300297a83efb39b8666ba0ff44369947c
Cr-Commit-Position: refs/heads/master@{#294002}

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

Ryan: PTAL?  A couple of specific question:

* Under what circumstances will URLRequest make an OnCertificateRequested()
notification?  I'm going with the default behavior, but I'm not completely
certain that's correct for this application.

* I'm a bit concerned that the shift to IDLE priority may create some flakiness
in the recently committed SDCH browser tests
(chrome/browser/net/sdch_browsertest.cc).  Specifically, in that code when I'm
trying to make sure that *if* a dictionary fetch was initiated by a request, I
presume that after a round trip to the server initiated after that request
finishes the dictionary fetch will have finished (i.e. I do "request, request
complete, round trip to server, test to see if dictionary fetch has occurred"). 
Do you think the IDLE priority would get in the way of that mechanism?  If so,
any other thoughts as to how I could be confident that if the dictionary fetch
was initiated, it will have occurred?

* Not really part of this CL, but: Does SdchDictionaryFetcher belong in
net/base?  It includes things from other subdirectories of net.  Happy to move
it up to net/ if that's appropriate; I put it in net/base since that's where
sdch_manager.* was.

Thanks!

[Ryan Sleevi, 2014-08-20 18:27:56]

Ryan: PTAL?  A couple of specific question:

* Under what circumstances will URLRequest make an OnCertificateRequested()
notification?  I'm going with the default behavior, but I'm not completely
certain that's correct for this application.

It's totes gonna choke on requests to a server that would otherwise work. You're
also going to have the same issues with HTTP auth and proxy auth. That is, even
if the main page loads, these dictionary fetches are going to fail.

I suspect you want this up in the //content layer to be able to leverage the
ResourceLoader hooks that wire this up to the ContentBrowserClient and related
HttpNetworkSession to ensure proper auth (including NetworkDelegate hooks)


* I'm a bit concerned that the shift to IDLE priority may create some flakiness
in the recently committed SDCH browser tests
(chrome/browser/net/sdch_browsertest.cc).  Specifically, in that code when I'm
trying to make sure that *if* a dictionary fetch was initiated by a request, I
presume that after a round trip to the server initiated after that request
finishes the dictionary fetch will have finished (i.e. I do "request, request
complete, round trip to server, test to see if dictionary fetch has occurred"). 
Do you think the IDLE priority would get in the way of that mechanism?  If so,
any other thoughts as to how I could be confident that if the dictionary fetch
was initiated, it will have occurred?

No clue on priorities. mmenke@ or willchan@ know more about this subsystem.

* Not really part of this CL, but: Does SdchDictionaryFetcher belong in
net/base?  It includes things from other subdirectories of net.  Happy to move
it up to net/ if that's appropriate; I put it in net/base since that's where
sdch_manager.* was.

Nope. I think both are wrong, which I expand more in comments below.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:119: int64 delay = 0LL;
portability warning: Don't assume int64 is LL

Use C99's <stdint.h> and INT64_C(0) to ensure the correct suffix across all our
platforms.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:120: if (original_url_throttler_entry_.get()
== NULL) {
Use boolean testing

if (!original_url_throttler_entry_.get())


(dcheng@ is finishing up the script to replace the implicit conversions, which
will eventually let these be rewritten the same as scoped_ptr<>, which is "if
(!original_url_throttler_entry_)"

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:128: if (original_url_throttler_entry_.get()
!= NULL) {
ditto

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:136:
base::MessageLoop::current()->PostDelayedTask(
Don't assume a MessageLoop

base::ThreadTaskRunnerHandle::Get()

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:140: }
Short-circuit and structure for the early return?

if (delay != 0) {
  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(...)
  return;
}

StartURLRequest();

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:172: return base::TimeTicks();
Rewrite this with short-circuits

if (!original_url_throttler_entry_.get())
  return base::TimeTicks()

... stuff

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:22: #include
"net/url_request/url_request_context.h"
Totes a layering violation here, and pre-existing unfortunately.

net/base should, in an ideal world, only depend on //net/base

Will keeps talking about wanting/needing to tighten this up.

I don't know if //net/http is much better, because //net/url_request depends on
//net/http, and if you're going to make a URL request, you should sit at that
level.

//net/sdch seems wrong, because it conflates the low-level bits (needed by
either //net/http or //net/url_request) with the high-level bits (the fetching
code)

I'm open to suggestions, but that's the general idea (which is to reduce
circular dependencies, and have //net/ subdirs be hermetic and tightly layered,
loosely coupled)

[mmenke, 2014-08-20 18:28:56]

Drive-by thoughts on two of your questions.

On 2014/08/20 18:05:25, rdsmith wrote:
> Ryan: PTAL?  A couple of specific question:
> 
> * Under what circumstances will URLRequest make an OnCertificateRequested()
> notification?  I'm going with the default behavior, but I'm not completely
> certain that's correct for this application.
> 
> * I'm a bit concerned that the shift to IDLE priority may create some
flakiness
> in the recently committed SDCH browser tests
> (chrome/browser/net/sdch_browsertest.cc).  Specifically, in that code when I'm
> trying to make sure that *if* a dictionary fetch was initiated by a request, I
> presume that after a round trip to the server initiated after that request
> finishes the dictionary fetch will have finished (i.e. I do "request, request
> complete, round trip to server, test to see if dictionary fetch has
occurred"). 
> Do you think the IDLE priority would get in the way of that mechanism?  If so,
> any other thoughts as to how I could be confident that if the dictionary fetch
> was initiated, it will have occurred?

This shouldn't make any difference - we don't hold back requests based on
priority, we just assign them sockets/DNS lookups based on it (At least in the
non-SPDY/non-QUIC case).

That having been said, that test sounds potentially flaky to me, as-is.  Even
local TCP requests are not guaranteed to be consistently ordered.  I've written
tests that inject job factories things into the URLRequestFilter to wait until
requests have been sent or been replied to.  May be worth using a similar
approach (Be great if we could create an official way to do this, right now
tests roll their own solutions, though, admittedly, some wait for different
things).

> * Not really part of this CL, but: Does SdchDictionaryFetcher belong in
> net/base?  It includes things from other subdirectories of net.  Happy to move
> it up to net/ if that's appropriate; I put it in net/base since that's where
> sdch_manager.* was.
> 
> Thanks!

Hrm...  Maybe it belongs in net/filter?  Though that still gives us a circular
dependency between net/url_request and net/filter, though.

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

Thanks, Ryan and Matt!  PTAL?  

High level responses and comments:
* OnCertificateRequested: Well, I went over the old code and convinced myself
that this is equivalent, so I'm not going to deal in this CL.  Having said that,
I'm not certain how to deal; I need SdchDictionaryFetcher in net so that it can
be used by cronet.  Do you know of any examples of wiring up optional
certificate/http/proxy auth handling to something whose main logic is in net/?

* Location of code: see comments.

* Flakiness: Ok, given that IDLE doesn't affect things, I won't worry about it
for this CL, but I'll try to fix it independently.  In my ideal world, I
wouldn't block on the request being completed, because the way that failure is
detected in that case is through timeout.  Instead, I'd block on something I was
more certain was going to happen that if it did would happen after the request
completed, and check that the request had completed.  But I'll figure something
out.

* Note that I've made a small semantic change in PS4; I noticed that I hadn't
copied over the cookie blocking code from the old code.  Mind you, my
inclination is that the cookie blocking code shouldn't *be* there, and I'll
think about removing it.  But if this CL is just about the URLFetcher->Request
transformation, that doesn't belong in this CL.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:119: int64 delay = 0LL;
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> portability warning: Don't assume int64 is LL
> 
> Use C99's <stdint.h> and INT64_C(0) to ensure the correct suffix across all
our
> platforms.

Done, but this code was copied from URLFetcherCore.  I'll change this there as
well; let me know if you think that's mission creep for this CL.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:120: if (original_url_throttler_entry_.get()
== NULL) {
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> Use boolean testing
> 
> if (!original_url_throttler_entry_.get())
> 
> 
> (dcheng@ is finishing up the script to replace the implicit conversions, which
> will eventually let these be rewritten the same as scoped_ptr<>, which is "if
> (!original_url_throttler_entry_)"

Done, here and in url_fetcher_core.cc.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:128: if (original_url_throttler_entry_.get()
!= NULL) {
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> ditto

Done.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:136:
base::MessageLoop::current()->PostDelayedTask(
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> Don't assume a MessageLoop
> 
> base::ThreadTaskRunnerHandle::Get()

Done.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:140: }
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> Short-circuit and structure for the early return?
> 
> if (delay != 0) {
>   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(...)
>   return;
> }
> 
> StartURLRequest();

Done.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:172: return base::TimeTicks();
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> Rewrite this with short-circuits
> 
> if (!original_url_throttler_entry_.get())
>   return base::TimeTicks()
> 
> ... stuff

Done, here and in url_fetcher_core.cc.

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/495523003/diff/20001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:22: #include
"net/url_request/url_request_context.h"
On 2014/08/20 18:27:55, Ryan Sleevi wrote:
> Totes a layering violation here, and pre-existing unfortunately.
> 
> net/base should, in an ideal world, only depend on //net/base
> 
> Will keeps talking about wanting/needing to tighten this up.
> 
> I don't know if //net/http is much better, because //net/url_request depends
on
> //net/http, and if you're going to make a URL request, you should sit at that
> level.
> 
> //net/sdch seems wrong, because it conflates the low-level bits (needed by
> either //net/http or //net/url_request) with the high-level bits (the fetching
> code)
> 
> I'm open to suggestions, but that's the general idea (which is to reduce
> circular dependencies, and have //net/ subdirs be hermetic and tightly
layered,
> loosely coupled)

Makes sense.  I'm not clear why I can't leave sdch_manager.* in net/base and put
sdch_dictionary_fetcher.* in net/.  Would that be ok with you?

[Ryan Sleevi, 2014-08-20 19:38:20]

On 2014/08/20 19:21:50, rdsmith wrote:
> Thanks, Ryan and Matt!  PTAL?  
> 
> High level responses and comments:
> * OnCertificateRequested: Well, I went over the old code and convinced myself
> that this is equivalent, so I'm not going to deal in this CL.  Having said
that,
> I'm not certain how to deal; I need SdchDictionaryFetcher in net so that it
can
> be used by cronet.  Do you know of any examples of wiring up optional
> certificate/http/proxy auth handling to something whose main logic is in net/?

Delegates. Delegates everywhere.

My own vision of an ideal world is that dictionary fetches are associated with
the renderer that's causing the fetch/load, which means from a //net perspective
that we can associate an SDCH dictionary fetch with the resource
needing/recommending the dictionary.

By being able to tag it to the URLRequest, we can work up into the
ResourceLoader, which can then figure out the renderer/rendererframe that
initiated the request, as well as make sure it goes through ResourceLoader's
delegates, which can then trigger prompting/UI/policy/extensions.

I'm not familiar enough with SDCH to suggest exactly where the cutpoint is, but
I suspect it's some degree of distinction between "being aware of SDCH data"
(perhaps at the //net layer?), surfacing that up to the delegate, and making the
*delegate* handle obtaining that data. In ResourceLoader's case, it'd kick off a
new fetch, and tie it to the 'original's' renderer/frame tree, so that it
becomes "just like" any other page load.

For Cronet, you'd have something light and simple (perhaps in the //net/extras),
which would NOT be used by Chrome, and which could either use a
URLRequest::Delegate directly or use URLFetcher.

Just off the cuff idea, because I'm not familiar enough with the dependencies at
play here. But I totally don't think //net should be creating it's own
URLRequest::Delegate here to fetch.

[Randy Smith (Not in Mondays), 2014-08-20 19:45:12]

On 2014/08/20 19:38:20, Ryan Sleevi wrote:
> On 2014/08/20 19:21:50, rdsmith wrote:
> > Thanks, Ryan and Matt!  PTAL?  
> > 
> > High level responses and comments:
> > * OnCertificateRequested: Well, I went over the old code and convinced
myself
> > that this is equivalent, so I'm not going to deal in this CL.  Having said
> that,
> > I'm not certain how to deal; I need SdchDictionaryFetcher in net so that it
> can
> > be used by cronet.  Do you know of any examples of wiring up optional
> > certificate/http/proxy auth handling to something whose main logic is in
net/?
> 
> Delegates. Delegates everywhere.
> 
> My own vision of an ideal world is that dictionary fetches are associated with
> the renderer that's causing the fetch/load, which means from a //net
perspective
> that we can associate an SDCH dictionary fetch with the resource
> needing/recommending the dictionary.
> 
> By being able to tag it to the URLRequest, we can work up into the
> ResourceLoader, which can then figure out the renderer/rendererframe that
> initiated the request, as well as make sure it goes through ResourceLoader's
> delegates, which can then trigger prompting/UI/policy/extensions.
> 
> I'm not familiar enough with SDCH to suggest exactly where the cutpoint is,
but
> I suspect it's some degree of distinction between "being aware of SDCH data"
> (perhaps at the //net layer?), surfacing that up to the delegate, and making
the
> *delegate* handle obtaining that data. In ResourceLoader's case, it'd kick off
a
> new fetch, and tie it to the 'original's' renderer/frame tree, so that it
> becomes "just like" any other page load.
> 
> For Cronet, you'd have something light and simple (perhaps in the
//net/extras),
> which would NOT be used by Chrome, and which could either use a
> URLRequest::Delegate directly or use URLFetcher.
> 
> Just off the cuff idea, because I'm not familiar enough with the dependencies
at
> play here. But I totally don't think //net should be creating it's own
> URLRequest::Delegate here to fetch.

My image, which I think is pretty close to what you're suggesting, is that
cronet would expose a delegate interface that any consumer could use, that would
be the same delegate interface that content would use, and not using that
interface would cause something simple (like, say, the current behavior :-}) to
happen.

[Ryan Sleevi, 2014-08-20 19:50:30]

On 2014/08/20 19:45:12, rdsmith wrote:
> My image, which I think is pretty close to what you're suggesting, is that
> cronet would expose a delegate interface that any consumer could use, that
would
> be the same delegate interface that content would use, and not using that
> interface would cause something simple (like, say, the current behavior :-})
to
> happen.

I don't think so. We don't want //content using cronet. That is, cronet is a
sibling-to-content, not a child.

Which means there does need to be a base interface, and cronet will have some
behaviour, and content will have some behaviour. cronet will be dumb, //content
will be smart.

I'm particularly keen to ensure that split, because time and time again, having
'simple' defaults with the expectation that higher layers will do the right
thing continues to blow up in our face with regard to //net users.

Granted, having abstraction points where the higher layer is supposed to do the
right thing is also blowing up in our face, since people are just copy-pasta'ing
code rather than talking to us, so I guess we're doomed either way :)

[Randy Smith (Not in Mondays), 2014-08-25 18:45:41]

Ryan and I had an offline conversation, which I'm going to attempt to summarize
here; Ryan please correct me if I mis-state anything:

We need to do a re-layering of SdchDictionaryFetcher.  The original layering
(with it in the embedder) was good, but wasn't used for the purposes for which
it was good (hooking into the embedder authentication patterns, primarily) which
misled me into thinking it was ok to drop it into net/.  I'm taking that as an
action item, but not a high priority one.  The current implementation might be a
reasonable one for cronet to use, but if so, it should go in whatever directory
cronet keeps the embedder logic for net.  I'm going to put a TODO() into the
sdch_dictionary_fetcher.h sketching this issue out.  Ryan agreed, at least in
principle, with letting this CL go in without this functionality, since it
doesn't make things any worse and has value from other perspectives (sketched
out in attached bug).

We're not going to try to find "the right place" for sdch_dictionary_fetcher.*
in this CL.  net/base is clearly wrong, but it should go with the embedder(s),
so it doesn't make sense to move it to an intermediate awkward pace in the
meantime.

[Randy Smith (Not in Mondays), 2014-08-25 19:11:17]

Ryan: PTAL?  I've made a note in sdch_dictionary_fetcher.h about the layering
and yanked a no-longer-necessary use of TrivialURLRequestContextGetter.

[Ryan Sleevi, 2014-08-25 20:00:01]

Unit tests that make sure backoff works as expected?

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:69: dictionary_ = "";
s/""/std::string()

or just dictionary_.clear()

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:77: request->Read(buffer_.get(),
kBufferSize, &bytes_read);
Read() returns a bool. Please check it before assuming bytes_read is valid.

We should be distinguishing an error from an EOF.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:97: dictionary_ +=
std::string(buffer_->data(), bytes_read);
dictionary_.append(), and avoid an unnecessary allocation+copy

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:108: dictionary_ = "";
.clear()

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:143: StartURLRequest();
DANGER: This potentially leads to large recursion.

Line 158 -> Line 78 -> line 110 -> line 143.

I would prefer to see this worked into an explicit state machine using the //net
idioms, so that we can have a single looping iteration of pending URLs until
exhaustion, error, or asynchronicity, aka DoLoop w/ an entry point.

(I realize that in practice 158 -> 78 will typically be asynchronous, but that
isn't a hard/documented guarantee of URLRequest's API)

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:152: fetch_queue_.front(), IDLE, this,
NULL);
Can this fail?

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:22: #include
"net/url_request/url_request_context.h"
You don't need this because of the forward declaration, right?

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:58: // Actually dispatch the request for the
next dictionary.
Can you expand this comment, particularly in light of the comments on 54-55?

Broadly speaking, I think it'd be great to take on a class-level comment on
lines 29/30 that describes a little bit about this class and how it
implements/what it implements, as well as some of the novelty (e.g. backoffs)
and limitations (as discussed). Then, these sorts of comments aren't too bad.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:80: // Both of them can only be accessed on
the IO thread.
Well, this class is base::NonThreadSafe, so I would expect this class to be
single-threaded. In the URLFetcher case, URLFetcher handled thread-marshalling
and guarantees, but now that you're a URLRequest::Delegate, you're assumed to be
exclusively on the IO thread.

Which makes me think that there may need some broader clean-up/threading
guarantees of this class if you're moving from Fetcher to URLRequest.

End goal: This class is single-threaded, and can live on any thread.
For the bits that need to live on the IO thread, perhaps an inner ::Core to hide
these threading details (... much like URLFetcher)

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:88: // for URL A in a short period of time.
Please reword this comment without the use of "we". Avoiding pronouns often has
the effect of making things clearer, especially for non-native English speakers.

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
for discussion.

[Randy Smith (Not in Mondays), 2014-08-25 20:12:20]

On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> Unit tests that make sure backoff works as expected?

Ryan, a quick high level question while I work on your comments: Do you perceive
the backoff code as being needed?  The only reason it's there is because it was
there in url_fetcher_core.cc, and I (\noideadog{}) blindly copied it.  It's
sounding like you think it's one of the trickier parts of the implementation
(requesting unit tests, comments, calling it novel) and if it isn't needed, I'm
happy to drop-kick it.  But it looked like we had some sort of contract to
throttle URLRequests dispatched from inside chrome through the
URLRequestThrottlerManager, so I went with that.

> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> File net/base/sdch_dictionary_fetcher.cc (right):
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:69: dictionary_ = "";
> s/""/std::string()
> 
> or just dictionary_.clear()
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:77: request->Read(buffer_.get(),
> kBufferSize, &bytes_read);
> Read() returns a bool. Please check it before assuming bytes_read is valid.
> 
> We should be distinguishing an error from an EOF.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:97: dictionary_ +=
> std::string(buffer_->data(), bytes_read);
> dictionary_.append(), and avoid an unnecessary allocation+copy
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:108: dictionary_ = "";
> .clear()
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:143: StartURLRequest();
> DANGER: This potentially leads to large recursion.
> 
> Line 158 -> Line 78 -> line 110 -> line 143.
> 
> I would prefer to see this worked into an explicit state machine using the
//net
> idioms, so that we can have a single looping iteration of pending URLs until
> exhaustion, error, or asynchronicity, aka DoLoop w/ an entry point.
> 
> (I realize that in practice 158 -> 78 will typically be asynchronous, but that
> isn't a hard/documented guarantee of URLRequest's API)
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:152: fetch_queue_.front(), IDLE, this,
> NULL);
> Can this fail?
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> File net/base/sdch_dictionary_fetcher.h (right):
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:22: #include
> "net/url_request/url_request_context.h"
> You don't need this because of the forward declaration, right?
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:58: // Actually dispatch the request for
the
> next dictionary.
> Can you expand this comment, particularly in light of the comments on 54-55?
> 
> Broadly speaking, I think it'd be great to take on a class-level comment on
> lines 29/30 that describes a little bit about this class and how it
> implements/what it implements, as well as some of the novelty (e.g. backoffs)
> and limitations (as discussed). Then, these sorts of comments aren't too bad.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:80: // Both of them can only be accessed on
> the IO thread.
> Well, this class is base::NonThreadSafe, so I would expect this class to be
> single-threaded. In the URLFetcher case, URLFetcher handled thread-marshalling
> and guarantees, but now that you're a URLRequest::Delegate, you're assumed to
be
> exclusively on the IO thread.
> 
> Which makes me think that there may need some broader clean-up/threading
> guarantees of this class if you're moving from Fetcher to URLRequest.
> 
> End goal: This class is single-threaded, and can live on any thread.
> For the bits that need to live on the IO thread, perhaps an inner ::Core to
hide
> these threading details (... much like URLFetcher)
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:88: // for URL A in a short period of time.
> Please reword this comment without the use of "we". Avoiding pronouns often
has
> the effect of making things clearer, especially for non-native English
speakers.
> 
> See
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
> for discussion.

[Ryan Sleevi, 2014-08-26 00:32:41]

Sorry, missed this.

noideadog here too. +mmenke


On Mon, Aug 25, 2014 at 1:12 PM, <rdsmith@chromium.org> wrote:

> On 2014/08/25 20:00:01, Ryan Sleevi wrote:
>
>> Unit tests that make sure backoff works as expected?
>>
>
> Ryan, a quick high level question while I work on your comments: Do you
> perceive
> the backoff code as being needed?  The only reason it's there is because
> it was
> there in url_fetcher_core.cc, and I (\noideadog{}) blindly copied it.  It's
> sounding like you think it's one of the trickier parts of the
> implementation
> (requesting unit tests, comments, calling it novel) and if it isn't
> needed, I'm
> happy to drop-kick it.  But it looked like we had some sort of contract to
> throttle URLRequests dispatched from inside chrome through the
> URLRequestThrottlerManager, so I went with that.
>
>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc
>
>> File net/base/sdch_dictionary_fetcher.cc (right):
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc#newcode69
>
>> net/base/sdch_dictionary_fetcher.cc:69: dictionary_ = "";
>> s/""/std::string()
>>
>
>  or just dictionary_.clear()
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc#newcode77
>
>> net/base/sdch_dictionary_fetcher.cc:77: request->Read(buffer_.get(),
>> kBufferSize, &bytes_read);
>> Read() returns a bool. Please check it before assuming bytes_read is
>> valid.
>>
>
>  We should be distinguishing an error from an EOF.
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc#newcode97
>
>> net/base/sdch_dictionary_fetcher.cc:97: dictionary_ +=
>> std::string(buffer_->data(), bytes_read);
>> dictionary_.append(), and avoid an unnecessary allocation+copy
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc#newcode108
>
>> net/base/sdch_dictionary_fetcher.cc:108: dictionary_ = "";
>> .clear()
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc#newcode143
>
>> net/base/sdch_dictionary_fetcher.cc:143: StartURLRequest();
>> DANGER: This potentially leads to large recursion.
>>
>
>  Line 158 -> Line 78 -> line 110 -> line 143.
>>
>
>  I would prefer to see this worked into an explicit state machine using the
>>
> //net
>
>> idioms, so that we can have a single looping iteration of pending URLs
>> until
>> exhaustion, error, or asynchronicity, aka DoLoop w/ an entry point.
>>
>
>  (I realize that in practice 158 -> 78 will typically be asynchronous, but
>> that
>> isn't a hard/documented guarantee of URLRequest's API)
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.cc#newcode152
>
>> net/base/sdch_dictionary_fetcher.cc:152: fetch_queue_.front(), IDLE,
>> this,
>> NULL);
>> Can this fail?
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.h
>
>> File net/base/sdch_dictionary_fetcher.h (right):
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.h#newcode22
>
>> net/base/sdch_dictionary_fetcher.h:22: #include
>> "net/url_request/url_request_context.h"
>> You don't need this because of the forward declaration, right?
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.h#newcode58
>
>> net/base/sdch_dictionary_fetcher.h:58: // Actually dispatch the request
>> for
>>
> the
>
>> next dictionary.
>> Can you expand this comment, particularly in light of the comments on
>> 54-55?
>>
>
>  Broadly speaking, I think it'd be great to take on a class-level comment
>> on
>> lines 29/30 that describes a little bit about this class and how it
>> implements/what it implements, as well as some of the novelty (e.g.
>> backoffs)
>> and limitations (as discussed). Then, these sorts of comments aren't too
>> bad.
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.h#newcode80
>
>> net/base/sdch_dictionary_fetcher.h:80: // Both of them can only be
>> accessed on
>> the IO thread.
>> Well, this class is base::NonThreadSafe, so I would expect this class to
>> be
>> single-threaded. In the URLFetcher case, URLFetcher handled
>> thread-marshalling
>> and guarantees, but now that you're a URLRequest::Delegate, you're
>> assumed to
>>
> be
>
>> exclusively on the IO thread.
>>
>
>  Which makes me think that there may need some broader clean-up/threading
>> guarantees of this class if you're moving from Fetcher to URLRequest.
>>
>
>  End goal: This class is single-threaded, and can live on any thread.
>> For the bits that need to live on the IO thread, perhaps an inner ::Core
>> to
>>
> hide
>
>> these threading details (... much like URLFetcher)
>>
>
>
> https://codereview.chromium.org/495523003/diff/60001/net/
> base/sdch_dictionary_fetcher.h#newcode88
>
>> net/base/sdch_dictionary_fetcher.h:88: // for URL A in a short period of
>> time.
>> Please reword this comment without the use of "we". Avoiding pronouns
>> often
>>
> has
>
>> the effect of making things clearer, especially for non-native English
>>
> speakers.
>
>  See
>>
>
> https://groups.google.com/a/chromium.org/d/msg/chromium-
> dev/NH-S6KCkr2M/yv0z3bdFPqQJ
>
>> for discussion.
>>
>
>
>
> https://codereview.chromium.org/495523003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

rdsmith@chromium.org changed reviewers:
+ mmenke@chromium.org

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

I don't see Matt on the cc list, so my own +mmenke (for c#11,12).

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:69: dictionary_ = "";
On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> s/""/std::string()
> 
> or just dictionary_.clear()

Done.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:77: request->Read(buffer_.get(),
kBufferSize, &bytes_read);
On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> Read() returns a bool. Please check it before assuming bytes_read is valid.
> 
> We should be distinguishing an error from an EOF.

Done, but I'd like a bit more of a consult here.  Both
URLFetcherCore::ReadResponse() (which I don't completely trust) and
ResourceLoader::ReadMore() (which I'm more inclined to take as the canonical
"way it should be done") ignore the result of request_->Read().  ResourceLoader
has a comment 

  // No need to check the return value here as we'll detect errors by
  // inspecting the URLRequest's status.

I think the same logic applies here.  If EOF, the status will be success, and
bytes_read will be set (to zero) and we can trust it.  If error, the status will
indicate that, and we won't look at bytes_read.  So I think the code was correct
as written, and if you disagree I want to understand why because "If you don't
understand it, it's dangerous."

Having said that, if I was going to keep the code as written, I'd want to define
the "OnReadCompleted()" interface to guarantee that it ignores bytes_read if the
request is in error.   That's not the guarantee in the url_request.h header
file, so I've reset bytes_read to -1 in the case of an error, which fits that
API interface.  If that was what you were originally getting at with your
comment: SG, Done :-}.

(Also done in url_fetcher_core.cc.)

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:97: dictionary_ +=
std::string(buffer_->data(), bytes_read);
On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> dictionary_.append(), and avoid an unnecessary allocation+copy

Done.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:108: dictionary_ = "";
On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> .clear()

Done.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:152: fetch_queue_.front(), IDLE, this,
NULL);
On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> Can this fail?

The header is silent on the topic.  As I read the code, only on an OOM error (it
translates directly to wrapping a "new URLRequest" in a scoped_ptr). 
ResourceDispatcherHostImpl::BeginRequest() (which I tend to think of as the
canonical request creation point in chrome) assumes it can't fail.  So I think
if I'm doing it wrong, I have good company :-}.  Let me know what you'd like me
to do.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:22: #include
"net/url_request/url_request_context.h"
On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> You don't need this because of the forward declaration, right?

Don't need it anymore, actually; I'm using URLRequestContext directly.  Done.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:80: // Both of them can only be accessed on
the IO thread.
On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> Well, this class is base::NonThreadSafe, so I would expect this class to be
> single-threaded. In the URLFetcher case, URLFetcher handled thread-marshalling
> and guarantees, but now that you're a URLRequest::Delegate, you're assumed to
be
> exclusively on the IO thread.
> 
> Which makes me think that there may need some broader clean-up/threading
> guarantees of this class if you're moving from Fetcher to URLRequest.

I believe that this class is used exclusively on the IO thread, so there aren't
any such cleanups required. 

> End goal: This class is single-threaded, and can live on any thread.

So is "can live on any thread" actually a requirement?  It isn't from the SDCH
side--SdchFetchers will live exclusively on the IO thread.  If it is a
requirement, then yes, there's some cleanup, since the URLRequest::Delegate must
live on the IO thread.  But I'm not seeing why that's a requirement.

> For the bits that need to live on the IO thread, perhaps an inner ::Core to
hide
> these threading details (... much like URLFetcher)

I'm going to assume that what's going on is that I should have been more careful
when I copied this comment out of URLFetcherCore, and I'll fix the comment on
that basis.  If you think that more's required, please let me know.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:88: // for URL A in a short period of time.
On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> Please reword this comment without the use of "we". Avoiding pronouns often
has
> the effect of making things clearer, especially for non-native English
speakers.
> 
> See
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
> for discussion.

Done both here and in url_fetcher_core.h.

[Randy Smith (Not in Mondays), 2014-08-26 15:40:40]

On 2014/08/26 15:34:17, rdsmith wrote:
> I don't see Matt on the cc list, so my own +mmenke (for c#11,12).
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> File net/base/sdch_dictionary_fetcher.cc (right):
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:69: dictionary_ = "";
> On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > s/""/std::string()
> > 
> > or just dictionary_.clear()
> 
> Done.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:77: request->Read(buffer_.get(),
> kBufferSize, &bytes_read);
> On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > Read() returns a bool. Please check it before assuming bytes_read is valid.
> > 
> > We should be distinguishing an error from an EOF.
> 
> Done, but I'd like a bit more of a consult here.  Both
> URLFetcherCore::ReadResponse() (which I don't completely trust) and
> ResourceLoader::ReadMore() (which I'm more inclined to take as the canonical
> "way it should be done") ignore the result of request_->Read(). 
ResourceLoader
> has a comment 
> 
>   // No need to check the return value here as we'll detect errors by
>   // inspecting the URLRequest's status.
> 
> I think the same logic applies here.  If EOF, the status will be success, and
> bytes_read will be set (to zero) and we can trust it.  If error, the status
will
> indicate that, and we won't look at bytes_read.  So I think the code was
correct
> as written, and if you disagree I want to understand why because "If you don't
> understand it, it's dangerous."
> 
> Having said that, if I was going to keep the code as written, I'd want to
define
> the "OnReadCompleted()" interface to guarantee that it ignores bytes_read if
the
> request is in error.   That's not the guarantee in the url_request.h header
> file, so I've reset bytes_read to -1 in the case of an error, which fits that
> API interface.  If that was what you were originally getting at with your
> comment: SG, Done :-}.
> 
> (Also done in url_fetcher_core.cc.)
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:97: dictionary_ +=
> std::string(buffer_->data(), bytes_read);
> On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > dictionary_.append(), and avoid an unnecessary allocation+copy
> 
> Done.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:108: dictionary_ = "";
> On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > .clear()
> 
> Done.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.cc:152: fetch_queue_.front(), IDLE, this,
> NULL);
> On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > Can this fail?
> 
> The header is silent on the topic.  As I read the code, only on an OOM error
(it
> translates directly to wrapping a "new URLRequest" in a scoped_ptr). 
> ResourceDispatcherHostImpl::BeginRequest() (which I tend to think of as the
> canonical request creation point in chrome) assumes it can't fail.  So I think
> if I'm doing it wrong, I have good company :-}.  Let me know what you'd like
me
> to do.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> File net/base/sdch_dictionary_fetcher.h (right):
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:22: #include
> "net/url_request/url_request_context.h"
> On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > You don't need this because of the forward declaration, right?
> 
> Don't need it anymore, actually; I'm using URLRequestContext directly.  Done.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:80: // Both of them can only be accessed on
> the IO thread.
> On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > Well, this class is base::NonThreadSafe, so I would expect this class to be
> > single-threaded. In the URLFetcher case, URLFetcher handled
thread-marshalling
> > and guarantees, but now that you're a URLRequest::Delegate, you're assumed
to
> be
> > exclusively on the IO thread.
> > 
> > Which makes me think that there may need some broader clean-up/threading
> > guarantees of this class if you're moving from Fetcher to URLRequest.
> 
> I believe that this class is used exclusively on the IO thread, so there
aren't
> any such cleanups required. 
> 
> > End goal: This class is single-threaded, and can live on any thread.
> 
> So is "can live on any thread" actually a requirement?  It isn't from the SDCH
> side--SdchFetchers will live exclusively on the IO thread.  If it is a
> requirement, then yes, there's some cleanup, since the URLRequest::Delegate
must
> live on the IO thread.  But I'm not seeing why that's a requirement.
> 
> > For the bits that need to live on the IO thread, perhaps an inner ::Core to
> hide
> > these threading details (... much like URLFetcher)
> 
> I'm going to assume that what's going on is that I should have been more
careful
> when I copied this comment out of URLFetcherCore, and I'll fix the comment on
> that basis.  If you think that more's required, please let me know.
> 
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> net/base/sdch_dictionary_fetcher.h:88: // for URL A in a short period of time.
> On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > Please reword this comment without the use of "we". Avoiding pronouns often
> has
> > the effect of making things clearer, especially for non-native English
> speakers.
> > 
> > See
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
> > for discussion.
> 
> Done both here and in url_fetcher_core.h.

Arggh; didn't mean to send these comments.  Ryan, please hold off on reacting to
these responses until I upload the PS that contains the changes referred to :-}.

[Randy Smith (Not in Mondays), 2014-08-26 15:43:31]

On 2014/08/26 15:40:40, rdsmith wrote:
> On 2014/08/26 15:34:17, rdsmith wrote:
> > I don't see Matt on the cc list, so my own +mmenke (for c#11,12).
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > File net/base/sdch_dictionary_fetcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.cc:69: dictionary_ = "";
> > On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > > s/""/std::string()
> > > 
> > > or just dictionary_.clear()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.cc:77: request->Read(buffer_.get(),
> > kBufferSize, &bytes_read);
> > On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > > Read() returns a bool. Please check it before assuming bytes_read is
valid.
> > > 
> > > We should be distinguishing an error from an EOF.
> > 
> > Done, but I'd like a bit more of a consult here.  Both
> > URLFetcherCore::ReadResponse() (which I don't completely trust) and
> > ResourceLoader::ReadMore() (which I'm more inclined to take as the canonical
> > "way it should be done") ignore the result of request_->Read(). 
> ResourceLoader
> > has a comment 
> > 
> >   // No need to check the return value here as we'll detect errors by
> >   // inspecting the URLRequest's status.
> > 
> > I think the same logic applies here.  If EOF, the status will be success,
and
> > bytes_read will be set (to zero) and we can trust it.  If error, the status
> will
> > indicate that, and we won't look at bytes_read.  So I think the code was
> correct
> > as written, and if you disagree I want to understand why because "If you
don't
> > understand it, it's dangerous."
> > 
> > Having said that, if I was going to keep the code as written, I'd want to
> define
> > the "OnReadCompleted()" interface to guarantee that it ignores bytes_read if
> the
> > request is in error.   That's not the guarantee in the url_request.h header
> > file, so I've reset bytes_read to -1 in the case of an error, which fits
that
> > API interface.  If that was what you were originally getting at with your
> > comment: SG, Done :-}.
> > 
> > (Also done in url_fetcher_core.cc.)
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.cc:97: dictionary_ +=
> > std::string(buffer_->data(), bytes_read);
> > On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > > dictionary_.append(), and avoid an unnecessary allocation+copy
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.cc:108: dictionary_ = "";
> > On 2014/08/25 20:00:00, Ryan Sleevi wrote:
> > > .clear()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.cc:152: fetch_queue_.front(), IDLE, this,
> > NULL);
> > On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > > Can this fail?
> > 
> > The header is silent on the topic.  As I read the code, only on an OOM error
> (it
> > translates directly to wrapping a "new URLRequest" in a scoped_ptr). 
> > ResourceDispatcherHostImpl::BeginRequest() (which I tend to think of as the
> > canonical request creation point in chrome) assumes it can't fail.  So I
think
> > if I'm doing it wrong, I have good company :-}.  Let me know what you'd like
> me
> > to do.
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > File net/base/sdch_dictionary_fetcher.h (right):
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.h:22: #include
> > "net/url_request/url_request_context.h"
> > On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > > You don't need this because of the forward declaration, right?
> > 
> > Don't need it anymore, actually; I'm using URLRequestContext directly. 
Done.
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.h:80: // Both of them can only be accessed
on
> > the IO thread.
> > On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > > Well, this class is base::NonThreadSafe, so I would expect this class to
be
> > > single-threaded. In the URLFetcher case, URLFetcher handled
> thread-marshalling
> > > and guarantees, but now that you're a URLRequest::Delegate, you're assumed
> to
> > be
> > > exclusively on the IO thread.
> > > 
> > > Which makes me think that there may need some broader clean-up/threading
> > > guarantees of this class if you're moving from Fetcher to URLRequest.
> > 
> > I believe that this class is used exclusively on the IO thread, so there
> aren't
> > any such cleanups required. 
> > 
> > > End goal: This class is single-threaded, and can live on any thread.
> > 
> > So is "can live on any thread" actually a requirement?  It isn't from the
SDCH
> > side--SdchFetchers will live exclusively on the IO thread.  If it is a
> > requirement, then yes, there's some cleanup, since the URLRequest::Delegate
> must
> > live on the IO thread.  But I'm not seeing why that's a requirement.
> > 
> > > For the bits that need to live on the IO thread, perhaps an inner ::Core
to
> > hide
> > > these threading details (... much like URLFetcher)
> > 
> > I'm going to assume that what's going on is that I should have been more
> careful
> > when I copied this comment out of URLFetcherCore, and I'll fix the comment
on
> > that basis.  If you think that more's required, please let me know.
> > 
> >
>
https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
> > net/base/sdch_dictionary_fetcher.h:88: // for URL A in a short period of
time.
> > On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> > > Please reword this comment without the use of "we". Avoiding pronouns
often
> > has
> > > the effect of making things clearer, especially for non-native English
> > speakers.
> > > 
> > > See
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...
> > > for discussion.
> > 
> > Done both here and in url_fetcher_core.h.
> 
> Arggh; didn't mean to send these comments.  Ryan, please hold off on reacting
to
> these responses until I upload the PS that contains the changes referred to
:-}.

Ok, Ryan, feel free to respond to what I just uploaded, or hold off until I've
uploaded a complete response to your comments; I'll let you know when I've done
that.

Matt: Still (very) interested in your response to c#11/12; that'll determine
*how* I respond to the rest of Ryan's comments.

[mmenke, 2014-08-26 17:15:47]

On 2014/08/26 15:43:31, rdsmith wrote:
> Ok, Ryan, feel free to respond to what I just uploaded, or hold off until I've
> uploaded a complete response to your comments; I'll let you know when I've
done
> that.
> 
> Matt: Still (very) interested in your response to c#11/12; that'll determine
> *how* I respond to the rest of Ryan's comments.

Sorry, wasn't CCed for most of this CL.

So (without really looking at this CL) it seems to me there two possible reasons
for backoff, assuming we don't retry failed dictionary requests automatically:
1)  We don't want to keep on requesting the same dictionary, if a network
request fails.
2)  We don't want to have a ton of dictionary requests interfere with other
network activity.

We may also want to delay dictionary requests until the network is idle, but
that's more of a delay, rather than backoff.  There's also a concern about use
of bandwidth while on cellular (Or power on mobile), but backoff doesn't address
either of those issues, either.

Backoff is a very bad solution to 1) - we shouldn't use backoff on potentially
good dictionary URLs, because all requests on one site keep referencing a bad
dictionary.  If we're concerned about that case, we should probably cache a
specific dictionary request failed bit, and eventually time that out.  I don't
think there's a real need for backoff there - we request the dictionary no more
than once per visit to that site, anyways, and if it's giving us a bad
dictionary, we don't want to degrade other sites, but really shouldn't worry
about accommodating them beyond that.  I'd say an arbitrary fixed timeout for
that should be fine, or could even just not fetch again until browser restart,
though that may be a bit too draconian.

I'm skeptical about whether 2) is a real problem, except in the presence of 1). 
I suppose you could imagine cases where there are a bunch of requests each using
a different dictionary, but it just doesn't seem likely, unless some site makes
a new dictionary for a large set of its resources.  I'd argue that a better
solution would either be waiting until the network was mostly idle, or just
dropping dictionary requests when there are too many of them.  We'll pick them
up again on the next visit, anyways.

Since we're at a layer above the cache, this code also means that we're
throttling requests that could potentially be served entirely out of the cache,
which also seems kinda weird (We do this elsewhere as well, but it seems
particularly weird here).


Am I missing anything?

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

On 2014/08/26 17:15:47, mmenke wrote:
> On 2014/08/26 15:43:31, rdsmith wrote:
> > Ok, Ryan, feel free to respond to what I just uploaded, or hold off until
I've
> > uploaded a complete response to your comments; I'll let you know when I've
> done
> > that.
> > 
> > Matt: Still (very) interested in your response to c#11/12; that'll determine
> > *how* I respond to the rest of Ryan's comments.
> 
> Sorry, wasn't CCed for most of this CL.
> 
> So (without really looking at this CL) it seems to me there two possible
reasons
> for backoff, assuming we don't retry failed dictionary requests automatically:
> 1)  We don't want to keep on requesting the same dictionary, if a network
> request fails.
> 2)  We don't want to have a ton of dictionary requests interfere with other
> network activity.
> 
> We may also want to delay dictionary requests until the network is idle, but
> that's more of a delay, rather than backoff.  There's also a concern about use
> of bandwidth while on cellular (Or power on mobile), but backoff doesn't
address
> either of those issues, either.
> 
> Backoff is a very bad solution to 1) - we shouldn't use backoff on potentially
> good dictionary URLs, because all requests on one site keep referencing a bad
> dictionary.  If we're concerned about that case, we should probably cache a
> specific dictionary request failed bit, and eventually time that out.  I don't
> think there's a real need for backoff there - we request the dictionary no
more
> than once per visit to that site, anyways, and if it's giving us a bad
> dictionary, we don't want to degrade other sites, but really shouldn't worry
> about accommodating them beyond that.  I'd say an arbitrary fixed timeout for
> that should be fine, or could even just not fetch again until browser restart,
> though that may be a bit too draconian.
> 
> I'm skeptical about whether 2) is a real problem, except in the presence of
1). 
> I suppose you could imagine cases where there are a bunch of requests each
using
> a different dictionary, but it just doesn't seem likely, unless some site
makes
> a new dictionary for a large set of its resources.  I'd argue that a better
> solution would either be waiting until the network was mostly idle, or just
> dropping dictionary requests when there are too many of them.  We'll pick them
> up again on the next visit, anyways.
> 
> Since we're at a layer above the cache, this code also means that we're
> throttling requests that could potentially be served entirely out of the
cache,
> which also seems kinda weird (We do this elsewhere as well, but it seems
> particularly weird here).
> 
> 
> Am I missing anything?

So your logic makes sense to me, but I want to make sure you're incorporating a
couple of pieces of information that you probably are aware of, but that you
didn't refer to in your response:
* The logic in question is lifted straight from URLFetcherCore, so don't assume
I had some plan in putting it in; I just wasn't clear on why it was there, so I
didn't want to take it out.  From what you're saying, it sounds like the
URLFetcher abuses being guarding against aren't really our uses.
* Not fetch again until browser restart is the current behavior
(SdchDictionaryFetcher::attempted_load_ is never expired); probably it should be
adapted, but I don't feel a strong urge to do so as part of this CL.  I'll make
a note to deal with it as part of dictionary expiration.  

However, I don't think either of those points is likely to affect your
judgement, so I'll yank out the backoff logic.  Thanks!

[mmenke, 2014-08-26 17:56:03]

On 2014/08/26 17:50:53, rdsmith wrote:
> On 2014/08/26 17:15:47, mmenke wrote:
> > On 2014/08/26 15:43:31, rdsmith wrote:
> > > Ok, Ryan, feel free to respond to what I just uploaded, or hold off until
> I've
> > > uploaded a complete response to your comments; I'll let you know when I've
> > done
> > > that.
> > > 
> > > Matt: Still (very) interested in your response to c#11/12; that'll
determine
> > > *how* I respond to the rest of Ryan's comments.
> > 
> > Sorry, wasn't CCed for most of this CL.
> > 
> > So (without really looking at this CL) it seems to me there two possible
> reasons
> > for backoff, assuming we don't retry failed dictionary requests
automatically:
> > 1)  We don't want to keep on requesting the same dictionary, if a network
> > request fails.
> > 2)  We don't want to have a ton of dictionary requests interfere with other
> > network activity.
> > 
> > We may also want to delay dictionary requests until the network is idle, but
> > that's more of a delay, rather than backoff.  There's also a concern about
use
> > of bandwidth while on cellular (Or power on mobile), but backoff doesn't
> address
> > either of those issues, either.
> > 
> > Backoff is a very bad solution to 1) - we shouldn't use backoff on
potentially
> > good dictionary URLs, because all requests on one site keep referencing a
bad
> > dictionary.  If we're concerned about that case, we should probably cache a
> > specific dictionary request failed bit, and eventually time that out.  I
don't
> > think there's a real need for backoff there - we request the dictionary no
> more
> > than once per visit to that site, anyways, and if it's giving us a bad
> > dictionary, we don't want to degrade other sites, but really shouldn't worry
> > about accommodating them beyond that.  I'd say an arbitrary fixed timeout
for
> > that should be fine, or could even just not fetch again until browser
restart,
> > though that may be a bit too draconian.
> > 
> > I'm skeptical about whether 2) is a real problem, except in the presence of
> 1). 
> > I suppose you could imagine cases where there are a bunch of requests each
> using
> > a different dictionary, but it just doesn't seem likely, unless some site
> makes
> > a new dictionary for a large set of its resources.  I'd argue that a better
> > solution would either be waiting until the network was mostly idle, or just
> > dropping dictionary requests when there are too many of them.  We'll pick
them
> > up again on the next visit, anyways.
> > 
> > Since we're at a layer above the cache, this code also means that we're
> > throttling requests that could potentially be served entirely out of the
> cache,
> > which also seems kinda weird (We do this elsewhere as well, but it seems
> > particularly weird here).
> > 
> > 
> > Am I missing anything?
> 
> So your logic makes sense to me, but I want to make sure you're incorporating
a
> couple of pieces of information that you probably are aware of, but that you
> didn't refer to in your response:
> * The logic in question is lifted straight from URLFetcherCore, so don't
assume
> I had some plan in putting it in; I just wasn't clear on why it was there, so
I
> didn't want to take it out.  From what you're saying, it sounds like the
> URLFetcher abuses being guarding against aren't really our uses.

Yes, that's exactly my feeling.

> * Not fetch again until browser restart is the current behavior
> (SdchDictionaryFetcher::attempted_load_ is never expired); probably it should
be
> adapted, but I don't feel a strong urge to do so as part of this CL.  I'll
make
> a note to deal with it as part of dictionary expiration.  
> 
> However, I don't think either of those points is likely to affect your
> judgement, so I'll yank out the backoff logic.  Thanks!

Indeed, neither affects what I said (And attempted_load_ not expiring exactly
addresses my first potential concern).

[Randy Smith (Not in Mondays), 2014-09-02 19:35:03]

Ryan: I'm working on some simple unit tests for SdchDictionaryFetcher, but while
I do that I'd like your feedback on my responses to your comments.  Most of that
is inline (below and in c#14), but the key question is around DoLoop() versus
not.

tl;dr: I feel as if DoLoop() is more complexity than this problem requires *and*
I don't see how it solves the recursion issue.  I've uploaded a DoLoop
implementation in PS7, and a sledgehammer fix for the recursion issue in PS8. 
My two key questions are:
* What did I miss in the DoLoop implementation around handling the recursion?
* Given whatever you imagine the DoLoop implementation would look like if I
hadn't missed it, do you still think the rewrite is worth it?

Thanks much in advance.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.cc:143: StartURLRequest();
On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> DANGER: This potentially leads to large recursion.
> 
> Line 158 -> Line 78 -> line 110 -> line 143.
> 
> I would prefer to see this worked into an explicit state machine using the
//net
> idioms, so that we can have a single looping iteration of pending URLs until
> exhaustion, error, or asynchronicity, aka DoLoop w/ an entry point.
> 
> (I realize that in practice 158 -> 78 will typically be asynchronous, but that
> isn't a hard/documented guarantee of URLRequest's API)

See top level comments; we need to hash a bit here.

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
File net/base/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/495523003/diff/60001/net/base/sdch_dictionary...
net/base/sdch_dictionary_fetcher.h:58: // Actually dispatch the request for the
next dictionary.
On 2014/08/25 20:00:01, Ryan Sleevi wrote:
> Can you expand this comment, particularly in light of the comments on 54-55?
> 
> Broadly speaking, I think it'd be great to take on a class-level comment on
> lines 29/30 that describes a little bit about this class and how it
> implements/what it implements, as well as some of the novelty (e.g. backoffs)
> and limitations (as discussed). Then, these sorts of comments aren't too bad.

Done; let me know what you think.

[Ryan Sleevi, 2014-09-02 20:04:20]

So, your sledgehammer totally works, but at an understandable cost. As you
noted, your DoLoop implementation doesn't really solve the issue either, because
it's got the same issue.

So we're clear, the fundamental issue is whether or not
URLRequest::Start() is allowed to (synchronously) return information. From both
the header and the implementation, the answer seems to be "Yes, it is", with it
long being noted the trouble this can cause for callers (c.f.
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
)

A simple solution for this - in either the DoLoop or the sledgehammer case -
would be to have a bool flag that you could set whether or not you're
synchronously processing.

Consider the DoLoop case

bool in_loop_;
(default in_loop_ = false)

int DoLoop(...) {
  base::AutoReset<bool> auto_reset_in_loop(&in_loop_, true);
  do {
     ...
  } while (...)
}

void SomeDelegateMethodICantRemember(...) {
  // do stuff
  if (in_loop_) {
    return;
  }
  return DoLoop(...);
}

void StartSomeRequest(...) {
  next_state_ = some_next_state;
  url_request->Start();
  if (stuff was done via SomeDelegateMethodBlah) {
    return OK;
  }
  return ERR_IO_PENDING;
}

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

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:83: }
This doesn't seem right. Normally we pass the previous return value into the
call (DoDispatch/DoRead/DoComplete) and it can decide how to handle the result
for the previous state. This swallows all errors before the state transitions
are allowed to handle them.

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:124: return ERR_IO_PENDING;               
// TODO(rdsmith): Is this correct?
I guess that's the question for URLRequest::Delegate / URLFetcher::Delegate,
whether it's guaranteed to asynchronously call the Delegate. This was the issue
I was alluding to in the previous remark - that your safety here is contingent
upon how the delegate methods get invoked.

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:176: DoLoop(OK);
Right, this has the same issue, in that it's contingent upon how the
URLRequest::Delegate invokes things.

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

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:43: // so this object will be destroyed
before either |*manager| or |*context|.
"The current implementation guarantees this" is likely a comment representing a
layering violation (nobody is guaranteeing this comment is kept up to date, nor
that these invariants will always hold)

[Randy Smith (Not in Mondays), 2014-09-02 23:34:33]

Ok, I think I'm ready for another round.  As noted offline, you've convinced me
around the DoLoop().  I've implemented what feels to me to be a reasonably clean
communications method between OnResponseStarted() and DoDispatchRequest().  I've
also added some unit tests.  Let me know what you think.

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

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:83: }
On 2014/09/02 20:04:18, Ryan Sleevi wrote:
> This doesn't seem right. Normally we pass the previous return value into the
> call (DoDispatch/DoRead/DoComplete) and it can decide how to handle the result
> for the previous state. This swallows all errors before the state transitions
> are allowed to handle them.

Done.

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:124: return ERR_IO_PENDING;               
// TODO(rdsmith): Is this correct?
On 2014/09/02 20:04:18, Ryan Sleevi wrote:
> I guess that's the question for URLRequest::Delegate / URLFetcher::Delegate,
> whether it's guaranteed to asynchronously call the Delegate. This was the
issue
> I was alluding to in the previous remark - that your safety here is contingent
> upon how the delegate methods get invoked.

I'm afraid I don't follow.  At least in the future, it seems like we should be
prepared at this point to handle either a synchronous or an asynchronous
starting.  If OnRequestStarted() was called synchronously i response to the
Start() call, I want to return from this routine with whatever status that call
was passed.  If it was called asynchronously, I want to return with
ERR_IO_PENDING.  I've made the assumption in the code I've modified this too
that current_request_->status().error() will be correct in either the
synchronous or the asynchronous case; is that correct?  If not, how should I
figure out whether or not the call was made synchronously or asynchronously?

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:176: DoLoop(OK);
On 2014/09/02 20:04:18, Ryan Sleevi wrote:
> Right, this has the same issue, in that it's contingent upon how the
> URLRequest::Delegate invokes things.

Yep, but again, I really don't think I should be coding assumptions about that
into this code if I can avoid it.

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

https://codereview.chromium.org/495523003/diff/120001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:43: // so this object will be destroyed
before either |*manager| or |*context|.
On 2014/09/02 20:04:19, Ryan Sleevi wrote:
> "The current implementation guarantees this" is likely a comment representing
a
> layering violation (nobody is guaranteeing this comment is kept up to date,
nor
> that these invariants will always hold)

Hmmm.  From my perspective, the normative part of the comment is the first
sentence, which I think is a fine specification of an interface contract and not
a layering violation.  I agree that the second sentence refers to things beyond
the ken of this class, and so I've removed it.  I don't see any layering
violations in the code.  Does removing that comment address your concern?

[Ryan Sleevi, 2014-09-04 01:14:21]

I'll need to think more about the SdchManager virtualization. I have a little
bit of an antibody response, but I think you'd find any comments I left now at
odds with comments I left in this round :)

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:116: // request on initial dispatch.
Avoid "we" in comments when possible (see
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
)

"|rv| is ignored, as the result from the previous request doesn't affect the
next request."

or something

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:142: // We're waiting to be gotten back to
through OnResponseStarted().
Avoid "we" in comments

// While this seems like it would cause an infinite loop in the
STATE_REQUEST_STARTED phase,
// it's the responsibility of OnResponseStarted() to advance to the next state.
This can either
// happen synchronously (when Start() is called in DoDispatchRequest), in which
case, this
// code will never be reached, or it may be called asynchronously. In the
asynchronous case,
// it will update next_state_ and the loop will be restarted, so return
ERR_IO_PENDING.


Or something that explains the subtlety we talked about here, since it's
non-obvious without that context.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:180: // If we haven't had an error, add the
dictionary.
// If the dictionary was successfully fetched, add it to the list of available
dictionaries.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:200: if (in_loop_)
// If OnResponseStarted was synchronously called, then DoLoop is still running.
In this case,
// just advance to the next state.
// Otherwise, if it was called asynchronously by the URLRequest, then restart
the state machine loop.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:216: return;
ditto comments above

Why is next_state_ not needed to be updated here?

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:33: // TODO).  It tracks all requests, only
attempting to fetch each dictionary
anal-retentive nit: once space, not two, between sentences.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:67: virtual void OnReadCompleted(URLRequest*
request, int bytes_read) OVERRIDE;
I argue with pkasting a lot about this, whether or not it's acceptable to 'hide'
methods like this.

If you feel strongly about it, fine, but I tend to prefer to keep the same
visibility as the base class, since by virtue of public inheritance, these
methods are exposed.

That is, just because they're private, someone can always

URLRequestDelegate* fetcher = somesdchdictionaryfetcher;
fetcher->OnResponseStarted(...) stuff

(Yes, I know no one will, but that's the idea behind it)

If you really did want to keep it private, you have to double-indirect. Namely,
SdchDictionaryFetcher is NOT a URLRequest::Delegate, but it HAS a
URLRequest::Delegate subclass (that's private), that it's friended

private:
  class Inner;
  friend class Inner;

  void OnResponseStarted(...);
  void OnReadCompleted(...);  // no virtual OR override

class Fetcher::Inner : public URLRequest::Delegate {
  explicit Inner(Fetcher* fetcher);

  virtual void OnResponseStarted(URLRequest* request) OVERRIDE {
    fetcher_->OnResponseStarted(request);
  }
  virtual void OnReadCompleted(...) OVERRIDE {
    fetcher_->OnReadCompleted(...);
  }
};


The above pattern is good not just for breaking visibility issues, but it also
helps solve encapsulation issues (e.g. "what if" URLRequest::Delegate has some
method added that conflicts with another interface here? What if it becomes a
ref-counted type? etc)

The compiler is generally smart enough to optimize this into (effectively) the
same code (your vtable for your Inner-as-a-Delegate just pops the member var
after the vtable offset and passes it into the concrete Fetcher class as the
first arg / 'this' pointer, as opposed to having the Fetcher-as-a-Delegate,
which... pops the this pointer after the vtable offset and passes it to the
concrete Fetcher class as the first arg / 'this' pointer)

In order of preference:
1) Prefer the above
2) Make these two methods public, since it's a lie to call them private even if
you want them to be
3) Do nothing. I feel strongly about this pattern, but there's enough vocal
disagreement from some (but not all) that it's not a fight worth having if you
don't agree :)

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher_unittest.cc (right):

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher_unittest.cc:33:
filter->AddHostnameHandler("http", "top.level.domain",
Use an RFC 2606 reserved TLD - .test is likely best here.

[Randy Smith (Not in Mondays), 2014-09-04 17:13:55]

Thanks for the comments!  I'll get to work on incorporating them.  One quick
question in the meantime:

On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> I'll need to think more about the SdchManager virtualization. I have a little
> bit of an antibody response, but I think you'd find any comments I left now at
> odds with comments I left in this round :)

The obvious solution is to create an AB SdchFetcher::Delegate C from which
SdchManager inherits that has the interfaces (and only the interfaces) needed by
Sdch*Fetcher.  Then I can just mock that class in the unit test.  Would that be
better?

> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> File net/base/sdch_dictionary_fetcher.cc (right):
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.cc:116: // request on initial dispatch.
> Avoid "we" in comments when possible (see
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
> )
> 
> "|rv| is ignored, as the result from the previous request doesn't affect the
> next request."
> 
> or something
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.cc:142: // We're waiting to be gotten back to
> through OnResponseStarted().
> Avoid "we" in comments
> 
> // While this seems like it would cause an infinite loop in the
> STATE_REQUEST_STARTED phase,
> // it's the responsibility of OnResponseStarted() to advance to the next
state.
> This can either
> // happen synchronously (when Start() is called in DoDispatchRequest), in
which
> case, this
> // code will never be reached, or it may be called asynchronously. In the
> asynchronous case,
> // it will update next_state_ and the loop will be restarted, so return
> ERR_IO_PENDING.
> 
> 
> Or something that explains the subtlety we talked about here, since it's
> non-obvious without that context.
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.cc:180: // If we haven't had an error, add
the
> dictionary.
> // If the dictionary was successfully fetched, add it to the list of available
> dictionaries.
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.cc:200: if (in_loop_)
> // If OnResponseStarted was synchronously called, then DoLoop is still
running.
> In this case,
> // just advance to the next state.
> // Otherwise, if it was called asynchronously by the URLRequest, then restart
> the state machine loop.
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.cc:216: return;
> ditto comments above
> 
> Why is next_state_ not needed to be updated here?
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> File net/base/sdch_dictionary_fetcher.h (right):
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.h:33: // TODO).  It tracks all requests, only
> attempting to fetch each dictionary
> anal-retentive nit: once space, not two, between sentences.
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher.h:67: virtual void
OnReadCompleted(URLRequest*
> request, int bytes_read) OVERRIDE;
> I argue with pkasting a lot about this, whether or not it's acceptable to
'hide'
> methods like this.
> 
> If you feel strongly about it, fine, but I tend to prefer to keep the same
> visibility as the base class, since by virtue of public inheritance, these
> methods are exposed.
> 
> That is, just because they're private, someone can always
> 
> URLRequestDelegate* fetcher = somesdchdictionaryfetcher;
> fetcher->OnResponseStarted(...) stuff
> 
> (Yes, I know no one will, but that's the idea behind it)
> 
> If you really did want to keep it private, you have to double-indirect.
Namely,
> SdchDictionaryFetcher is NOT a URLRequest::Delegate, but it HAS a
> URLRequest::Delegate subclass (that's private), that it's friended
> 
> private:
>   class Inner;
>   friend class Inner;
> 
>   void OnResponseStarted(...);
>   void OnReadCompleted(...);  // no virtual OR override
> 
> class Fetcher::Inner : public URLRequest::Delegate {
>   explicit Inner(Fetcher* fetcher);
> 
>   virtual void OnResponseStarted(URLRequest* request) OVERRIDE {
>     fetcher_->OnResponseStarted(request);
>   }
>   virtual void OnReadCompleted(...) OVERRIDE {
>     fetcher_->OnReadCompleted(...);
>   }
> };
> 
> 
> The above pattern is good not just for breaking visibility issues, but it also
> helps solve encapsulation issues (e.g. "what if" URLRequest::Delegate has some
> method added that conflicts with another interface here? What if it becomes a
> ref-counted type? etc)
> 
> The compiler is generally smart enough to optimize this into (effectively) the
> same code (your vtable for your Inner-as-a-Delegate just pops the member var
> after the vtable offset and passes it into the concrete Fetcher class as the
> first arg / 'this' pointer, as opposed to having the Fetcher-as-a-Delegate,
> which... pops the this pointer after the vtable offset and passes it to the
> concrete Fetcher class as the first arg / 'this' pointer)
> 
> In order of preference:
> 1) Prefer the above
> 2) Make these two methods public, since it's a lie to call them private even
if
> you want them to be
> 3) Do nothing. I feel strongly about this pattern, but there's enough vocal
> disagreement from some (but not all) that it's not a fight worth having if you
> don't agree :)
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> File net/base/sdch_dictionary_fetcher_unittest.cc (right):
> 
>
https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
> net/base/sdch_dictionary_fetcher_unittest.cc:33:
> filter->AddHostnameHandler("http", "top.level.domain",
> Use an RFC 2606 reserved TLD - .test is likely best here.

[Randy Smith (Not in Mondays), 2014-09-04 18:52:57]

Comments incorporated or responded to.

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:116: // request on initial dispatch.
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> Avoid "we" in comments when possible

Right, sorry, still training myself out of that habit and I sometimes slip.

> (see
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
> )
> 
> "|rv| is ignored, as the result from the previous request doesn't affect the
> next request."
> 
> or something

Done.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:142: // We're waiting to be gotten back to
through OnResponseStarted().
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> Avoid "we" in comments
> 
> // While this seems like it would cause an infinite loop in the
> STATE_REQUEST_STARTED phase,
> // it's the responsibility of OnResponseStarted() to advance to the next
state.
> This can either
> // happen synchronously (when Start() is called in DoDispatchRequest), in
which
> case, this
> // code will never be reached, or it may be called asynchronously. In the
> asynchronous case,
> // it will update next_state_ and the loop will be restarted, so return
> ERR_IO_PENDING.
> 
> 
> Or something that explains the subtlety we talked about here, since it's
> non-obvious without that context.

I did some text that made sense to me (same meaning as what you have).

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:180: // If we haven't had an error, add the
dictionary.
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> // If the dictionary was successfully fetched, add it to the list of available
> dictionaries.

Done.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:200: if (in_loop_)
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> // If OnResponseStarted was synchronously called, then DoLoop is still
running.
> In this case,
> // just advance to the next state.
> // Otherwise, if it was called asynchronously by the URLRequest, then restart
> the state machine loop.

If you ask me again, I'll do this, but it feels like overkill to me.  The
concept of advancing states had better be pretty clear to anyone reading this
code; if it isn't, then I don't think a random comment here will help (I could
do a top of file comment explaining state machines?).  I'm happy to put in
something explaining the loop guard.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:216: return;
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> ditto comments above

ditto response above.

> Why is next_state_ not needed to be updated here?

I'm afraid that in digging to answer your question, I've come up with a question
for you.  A close reading of url_request.h says to me that a call to Start()
always results in an OnResponseStarted() callback, but a call to Read() only
results in an OnReadCompleted() callback if the read is asynchronous.  Does that
match your understanding?  It surprised me a little when I engaged with your
question.  

Having said that, I think the answer is the same either way.  All Do*() methods
are required to specify their next state, because of the next_state_ =
STATE_NONE; at the beginning of the loop.  Callbacks only need to specify states
when they're transitioning, and reads completing don't transition states.  

If OnReadCompleted() was guaranteed to be called, I might shift the checking for
EOF and transition to STATE_REQUEST_COMPLETE into it (invariant: state
transitions occur at the end of an operation, including if it's asynchronous)
but given that it isn't guaranteed to be called, I'm disinclined to duplicate
the EOF checking; let it happen on the next round around the guitar.

Of course, now I start engaging with what comments are necessary, and if there's
any confusion about the fact that the state transition is occurring in one
On*Completed() method but not in the other (which there obviously is, given your
question :-}).   I've tossed a note in OnReadCompleted() about it, but I'd also
be willing to refactor so that the transition *is* in OnReadCompleted() and I
call OnReadCompleted() from DoRead() in the sync case.  If I do that, the price
will be a snarky comment about the inconsistency of URLRequest's interface :-}.

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:33: // TODO).  It tracks all requests, only
attempting to fetch each dictionary
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> anal-retentive nit: once space, not two, between sentences.

Apologies for getting a-r back at you, but could you give me a style guide
reference?  The web indicates that there's more debate about this than I thought
(http://en.wikipedia.org/wiki/Sentence_spacing), but that's not how I was taught
to write sentences.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:67: virtual void OnReadCompleted(URLRequest*
request, int bytes_read) OVERRIDE;
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> I argue with pkasting a lot about this, whether or not it's acceptable to
'hide'
> methods like this.
> 
> If you feel strongly about it, fine, but I tend to prefer to keep the same
> visibility as the base class, since by virtue of public inheritance, these
> methods are exposed.
> 
> That is, just because they're private, someone can always
> 
> URLRequestDelegate* fetcher = somesdchdictionaryfetcher;
> fetcher->OnResponseStarted(...) stuff
> 
> (Yes, I know no one will, but that's the idea behind it)
> 
> If you really did want to keep it private, you have to double-indirect.
Namely,
> SdchDictionaryFetcher is NOT a URLRequest::Delegate, but it HAS a
> URLRequest::Delegate subclass (that's private), that it's friended
> 
> private:
>   class Inner;
>   friend class Inner;
> 
>   void OnResponseStarted(...);
>   void OnReadCompleted(...);  // no virtual OR override
> 
> class Fetcher::Inner : public URLRequest::Delegate {
>   explicit Inner(Fetcher* fetcher);
> 
>   virtual void OnResponseStarted(URLRequest* request) OVERRIDE {
>     fetcher_->OnResponseStarted(request);
>   }
>   virtual void OnReadCompleted(...) OVERRIDE {
>     fetcher_->OnReadCompleted(...);
>   }
> };
> 
> 
> The above pattern is good not just for breaking visibility issues, but it also
> helps solve encapsulation issues (e.g. "what if" URLRequest::Delegate has some
> method added that conflicts with another interface here? What if it becomes a
> ref-counted type? etc)
> 
> The compiler is generally smart enough to optimize this into (effectively) the
> same code (your vtable for your Inner-as-a-Delegate just pops the member var
> after the vtable offset and passes it into the concrete Fetcher class as the
> first arg / 'this' pointer, as opposed to having the Fetcher-as-a-Delegate,
> which... pops the this pointer after the vtable offset and passes it to the
> concrete Fetcher class as the first arg / 'this' pointer)
> 
> In order of preference:
> 1) Prefer the above
> 2) Make these two methods public, since it's a lie to call them private even
if
> you want them to be
> 3) Do nothing. I feel strongly about this pattern, but there's enough vocal
> disagreement from some (but not all) that it's not a fight worth having if you
> don't agree :)

Wow. Ok, will do.

(I personally prefer the style as written, and while I acknowledge the virtues
of the approach you sketch out above, I consider it too much extra boilerplate
to be worth it.  However, the feelings are weak in this one; so I'll do it your
way on this CL.)

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
File net/base/sdch_dictionary_fetcher_unittest.cc (right):

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher_unittest.cc:33:
filter->AddHostnameHandler("http", "top.level.domain",
On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> Use an RFC 2606 reserved TLD - .test is likely best here.

Done, along with some refactoring out of common strings.

[Randy Smith (Not in Mondays), 2014-09-04 18:54:14]

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:67: virtual void OnReadCompleted(URLRequest*
request, int bytes_read) OVERRIDE;
On 2014/09/04 18:52:57, rdsmith wrote:
> On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> > I argue with pkasting a lot about this, whether or not it's acceptable to
> 'hide'
> > methods like this.
> > 
> > If you feel strongly about it, fine, but I tend to prefer to keep the same
> > visibility as the base class, since by virtue of public inheritance, these
> > methods are exposed.
> > 
> > That is, just because they're private, someone can always
> > 
> > URLRequestDelegate* fetcher = somesdchdictionaryfetcher;
> > fetcher->OnResponseStarted(...) stuff
> > 
> > (Yes, I know no one will, but that's the idea behind it)
> > 
> > If you really did want to keep it private, you have to double-indirect.
> Namely,
> > SdchDictionaryFetcher is NOT a URLRequest::Delegate, but it HAS a
> > URLRequest::Delegate subclass (that's private), that it's friended
> > 
> > private:
> >   class Inner;
> >   friend class Inner;
> > 
> >   void OnResponseStarted(...);
> >   void OnReadCompleted(...);  // no virtual OR override
> > 
> > class Fetcher::Inner : public URLRequest::Delegate {
> >   explicit Inner(Fetcher* fetcher);
> > 
> >   virtual void OnResponseStarted(URLRequest* request) OVERRIDE {
> >     fetcher_->OnResponseStarted(request);
> >   }
> >   virtual void OnReadCompleted(...) OVERRIDE {
> >     fetcher_->OnReadCompleted(...);
> >   }
> > };
> > 
> > 
> > The above pattern is good not just for breaking visibility issues, but it
also
> > helps solve encapsulation issues (e.g. "what if" URLRequest::Delegate has
some
> > method added that conflicts with another interface here? What if it becomes
a
> > ref-counted type? etc)
> > 
> > The compiler is generally smart enough to optimize this into (effectively)
the
> > same code (your vtable for your Inner-as-a-Delegate just pops the member var
> > after the vtable offset and passes it into the concrete Fetcher class as the
> > first arg / 'this' pointer, as opposed to having the Fetcher-as-a-Delegate,
> > which... pops the this pointer after the vtable offset and passes it to the
> > concrete Fetcher class as the first arg / 'this' pointer)
> > 
> > In order of preference:
> > 1) Prefer the above
> > 2) Make these two methods public, since it's a lie to call them private even
> if
> > you want them to be
> > 3) Do nothing. I feel strongly about this pattern, but there's enough vocal
> > disagreement from some (but not all) that it's not a fight worth having if
you
> > don't agree :)
> 
> Wow. Ok, will do.
> 
> (I personally prefer the style as written, and while I acknowledge the virtues
> of the approach you sketch out above, I consider it too much extra boilerplate
> to be worth it.  However, the feelings are weak in this one; so I'll do it
your
> way on this CL.)

Sorry, I realized this was ambiguous.  I've made the methods public.

[Ryan Sleevi, 2014-09-04 19:05:30]

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:200: if (in_loop_)
On 2014/09/04 18:52:57, rdsmith wrote:
> On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> > // If OnResponseStarted was synchronously called, then DoLoop is still
> running.
> > In this case,
> > // just advance to the next state.
> > // Otherwise, if it was called asynchronously by the URLRequest, then
restart
> > the state machine loop.
> 
> If you ask me again, I'll do this, but it feels like overkill to me.  The
> concept of advancing states had better be pretty clear to anyone reading this
> code; if it isn't, then I don't think a random comment here will help (I could
> do a top of file comment explaining state machines?).  I'm happy to put in
> something explaining the loop guard.  

I'm specifically trying to explain the subtlety of why you're short-circuiting,
and how it relates to the overall operation of the loop.

The goal is for both the comments here, DoRequestStarted, and OnReadCompleted to
point to each other and provide a complete narrative of the subtlety, of which
there is some, precisely because of URLRequest's sync/async weirdness.

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:216: return;
On 2014/09/04 18:52:57, rdsmith wrote:
> On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> > ditto comments above
> 
> ditto response above.
> 
> > Why is next_state_ not needed to be updated here?
> 
> I'm afraid that in digging to answer your question, I've come up with a
question
> for you.  A close reading of url_request.h says to me that a call to Start()
> always results in an OnResponseStarted() callback, but a call to Read() only
> results in an OnReadCompleted() callback if the read is asynchronous.  Does
that
> match your understanding?  It surprised me a little when I engaged with your
> question.  

I don't know. mmenke would/should.

> 
> Having said that, I think the answer is the same either way.  All Do*()
methods
> are required to specify their next state, because of the next_state_ =
> STATE_NONE; at the beginning of the loop.  Callbacks only need to specify
states
> when they're transitioning, and reads completing don't transition states.  

Reasonable reply :)

> 
> If OnReadCompleted() was guaranteed to be called, I might shift the checking
for
> EOF and transition to STATE_REQUEST_COMPLETE into it (invariant: state
> transitions occur at the end of an operation, including if it's asynchronous)
> but given that it isn't guaranteed to be called, I'm disinclined to duplicate
> the EOF checking; let it happen on the next round around the guitar.
> 
> Of course, now I start engaging with what comments are necessary, and if
there's
> any confusion about the fact that the state transition is occurring in one
> On*Completed() method but not in the other (which there obviously is, given
your
> question :-}).   I've tossed a note in OnReadCompleted() about it, but I'd
also
> be willing to refactor so that the transition *is* in OnReadCompleted() and I
> call OnReadCompleted() from DoRead() in the sync case.  If I do that, the
price
> will be a snarky comment about the inconsistency of URLRequest's interface
:-}.

Indeed, I was wanting to call attention to the inconsistency, since it grabbed
my attention, and encourage you to document if appropriate.

[mmenke, 2014-09-04 19:34:24]

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

https://codereview.chromium.org/495523003/diff/180001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:216: return;
On 2014/09/04 19:05:30, Ryan Sleevi wrote:
> On 2014/09/04 18:52:57, rdsmith wrote:
> > On 2014/09/04 01:14:21, Ryan Sleevi wrote:
> > > ditto comments above
> > 
> > ditto response above.
> > 
> > > Why is next_state_ not needed to be updated here?
> > 
> > I'm afraid that in digging to answer your question, I've come up with a
> question
> > for you.  A close reading of url_request.h says to me that a call to Start()
> > always results in an OnResponseStarted() callback, but a call to Read() only
> > results in an OnReadCompleted() callback if the read is asynchronous.  Does
> that
> > match your understanding?  It surprised me a little when I engaged with your
> > question.  
> 
> I don't know. mmenke would/should.

Yes, Start doesn't have a return value, so always returns the response via a
callback.

I'm not sure if it's possible for OnResponseStarted to be called synchronously /
re-entrantly, during the call to Start().  It's not possible for
HttpNetworkTransactions, or GetAllCookiesForURLAsync, to return synchronously,
but I don't see code in URLRequest / URLRequestJob that ensures this.  So
non-HTTP/HTTPS requests, or requests that don't go through the cookie store
(Because of load flags, or lack of a cookie store) and are served out of the
in-memory cache may theoretically be able to do that.

[Randy Smith (Not in Mondays), 2014-09-04 19:48:13]

Ryan: I'm not sure if your last comments meant you were satisfied with the
changes I had made to the comments or not :-}.  I've tweaked them a bit more,
but I'm considering the ball to be in your court at this point.  (Do let me know
if you want the SdchFetcher::Delegate refactor.)

[Ryan Sleevi, 2014-09-04 19:54:24]

On 2014/09/04 19:48:13, rdsmith wrote:
> Ryan: I'm not sure if your last comments meant you were satisfied with the
> changes I had made to the comments or not :-}.  I've tweaked them a bit more,
> but I'm considering the ball to be in your court at this point.  (Do let me
know
> if you want the SdchFetcher::Delegate refactor.)

To be clearer, I was not satisfied. I was stating what I would like to see
accomplished, but the exact extent of how that happened was up to you.

The issue was that non-trivial code didn't have sufficient context around it to
explain it's non-trivialness. That alone requires comments :)

Your new comments address this, so thanks.

Regarding the Fetcher::Delegate refactor, that's a nice to have, but it's a nit.
You made the URLRequest::Delegate methods public, which was the primary concern.
The rest is just ponies and sparkles and long-term hygiene :)

[Ryan Sleevi, 2014-09-04 20:06:21]

Ok, never mind, I see you were talking about the circular dependency between
Fetcher and Manager, and the need to make AddDictionary virtual for testing.

That is, the current design is 'broken', in that it forces a circular dependency

Manager depends on Fetcher (the abstract interface)
DictionaryFetcher (the concrete implementation) depends on Manager

In fact, I'm not even sure how one could ever implement Fetcher (the abstract)
without taking on a dependency on Manager when implementing the concrete.

So this does argue that we should break that dependency graph with inversion,
such that the Manager gives the Fetcher a way to communicate the added
dictionaries (and possibly error status), since that's key to DictionaryFetcher
being able to fulfill the Fetcher interface.

Note the implicit dependency via SdchManager::SdchErrorRecovery, which also
seems it would need to go through the delegate interface.

I realize this CL is getting larger than you expected or anticipated, but hey :)


So I also note below some instances of double spaces. My quick and simple regex
picks up 1060 instances of single spaces between sentences in comments, while
only 235 instances of double spaces. It'd be great to drive that number down
(and not just because I lurve me some MLA/APA stylistic goodness, but also
because typewriters, lulz). It'd be great that as you're shifting the code you
can take a stab at tweaking the comments, but if you feel opposed to doing that
here, I'm fine with an LGTM.

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

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:46: // Avoid pushing duplicate copy onto
queue.  We may fetch this url again later
here

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:93: // state machine loop will handle the
state transition.  Otherwise,
here

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:114: // state machine loop will handle the
state transition.  Otherwise,
here

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:180: // (called in DoDispatchRequest(),
above).  If that callback did not
here

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

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:34: // once.
nit: single spaces

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:90: // trying to load more than once).  In
addition, some dictionaries prove
Weird double space here that doesn't occur on 92.

You could make line 92 a double space just to annoy me, but at least be
consistent, or you could make the double spaces single :)

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

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_manager.h...
net/base/sdch_manager.h:313: // to use to decompreses data that arrived as SDCH
encoded content.  Check to
here

[Ryan Sleevi, 2014-09-04 20:07:27]

Er, I forgot that was the phrase that pays. To be explicit: I do think we should
do the Fetcher/Delegate split. I'm happy if you're willing to do that in the
next CL, but I don't want to let the virtual dtor bake, and I'd prefer we fix it
before/during than after.

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

Ryan: Having done all this work, I'm now no longer sure it was what you were
looking for; I think I assumed you had a negative reaction to my test strategy
similar to the one I did, and so fixed that :-}.   What I've done here (in
addition to a lot of nits, mostly change spaces after sentences) is created an
SdchFetcher::Delegate class and made SdchManager inherit from it.  This allows
sdch_filter_unittest.cc to be written without any reference to SdchManager
(though it still includes the header file, since that's where SdchFetcher is
defined).  That means that I'm not inheriting from SdchManager in order to mock
out AddSdchDictionary(), but it doesn't change anything else.  

If this doesn't address your concern, are you willing to be a bit more explicit
about that concern?  To respond to one comment in your comments in case it's
relevant: I don't agree that the original structure had a circular dependency. 
SdchDictionaryFetcher inheriting from SdchFetcher and depending on SdchManager,
and SdchManager depending on SdchFetcher seems to me a perfectly fine way of
handling the architectural layering, and I don't consider A inheriting from B to
mean that B is dependent on A (which is the only place I can see a potential
circular dependency).  So if you want a change based on an existing circular
dependency, you should put a bit more verbiage into explaining that dependency
to me.

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

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:46: // Avoid pushing duplicate copy onto
queue.  We may fetch this url again later
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> here

Done.

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:93: // state machine loop will handle the
state transition.  Otherwise,
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> here

Done.

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:114: // state machine loop will handle the
state transition.  Otherwise,
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> here

Done.

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.cc:180: // (called in DoDispatchRequest(),
above).  If that callback did not
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> here

Done.

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

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:34: // once.
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> nit: single spaces

Done.

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_dictionar...
net/base/sdch_dictionary_fetcher.h:90: // trying to load more than once).  In
addition, some dictionaries prove
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> Weird double space here that doesn't occur on 92.
> 
> You could make line 92 a double space just to annoy me, but at least be
> consistent, or you could make the double spaces single :)

Done :-} (double -> single; you got me with the analysis of what's already in
the code.)

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

https://codereview.chromium.org/495523003/diff/240001/net/base/sdch_manager.h...
net/base/sdch_manager.h:313: // to use to decompreses data that arrived as SDCH
encoded content.  Check to
On 2014/09/04 20:06:20, Ryan Sleevi wrote:
> here

You skipped several others earlier in the file--if you're going to make me
change things for consistency, no need to restrict yourself to only the code I
haven't edited that shows up for context on Rietveld :-}.  

(I searched for a period followed by two spaces in all files in net/ involved in
this CL; I presume that covers the bases as far as you're concerned, but let me
know if I'm wrong.  I left one ".  " in profile_imple_io_data.cc alone because
it had nothing to do with the code I was touching.)

[Ryan Sleevi, 2014-09-08 19:29:54]

Thanks Randy. You actually picked up on the circular dependency I was talking
about.

Namely that SdchManager depends on SdchFetcher (to implement the logic), and the
only possible way to implement SdchFetcher correctly (aka SdchDictionaryFetcher)
depended on SdchManager.

Now that you have the SdchFetcher::Delegate interface, SdchManager now depends
on SdchFetcher (as a class member) and on SdchFetcher::Delegate (as a base
class), but SdchFetcher (and more concretely, SdchDictionaryFetcher) no longer
depends on SdchManager.

So LGTM, mod comment nit.

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

https://codereview.chromium.org/495523003/diff/270001/net/base/sdch_manager.h...
net/base/sdch_manager.h:49: // Deliver the fetched dictionary back to the
consumer.
Documentation:

// Called whenever the SdchFetcher has successfully retrieved a
// dictionary. |dictionary_text| contains the body of the dictionary
// retrieved from |dictionary_url|.


That is, in the case of a Delegate interface, you're trying to document either
when an implementation can expect the call, or what a caller expects of the
implementation.

[Randy Smith (Not in Mondays), 2014-09-08 20:03:29]

Fair enough.  Thanks for the thorough (:-}) review!

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

https://codereview.chromium.org/495523003/diff/270001/net/base/sdch_manager.h...
net/base/sdch_manager.h:49: // Deliver the fetched dictionary back to the
consumer.
On 2014/09/08 19:29:54, Ryan Sleevi wrote:
> Documentation:
> 
> // Called whenever the SdchFetcher has successfully retrieved a
> // dictionary. |dictionary_text| contains the body of the dictionary
> // retrieved from |dictionary_url|.
> 
> 
> That is, in the case of a Delegate interface, you're trying to document either
> when an implementation can expect the call, or what a caller expects of the
> implementation.

Good point.  Done.

[Randy Smith (Not in Mondays), 2014-09-08 20:03:45]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-09-08 20:05:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rdsmith@chromium.org/495523003/290001

[commit-bot: I haz the power, 2014-09-08 20:17:31]

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

[commit-bot: I haz the power, 2014-09-08 20:17:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[mmenke, 2014-09-08 20:34:22]

On 2014/09/08 20:17:32, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
>   win_chromium_compile_dbg on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

profiles/ LGTM

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

The CQ bit was checked by rdsmith@chromium.org

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

The CQ bit was unchecked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2014-09-09 19:01:10]

Ryan, FYI: PS18 tweaked test infrastructure added earlier to make debugging a
try job failure easier, and fixed that failure.  I don't think anything requires
a re-review (it's test infrastructure you didn't comment on when it went in),
but giving you a heads up JIC.
.

[Randy Smith (Not in Mondays), 2014-09-09 19:53:41]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-09-09 19:58:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rdsmith@chromium.org/495523003/330001

[commit-bot: I haz the power, 2014-09-09 20:40:19]

Committed patchset #18 (id:330001) as f0a2e41195e5e6efad55f96e5b2dae0a194c2dbc

[commit-bot: I haz the power, 2014-09-10 03:55:22]

Patchset 18 (id:??) landed as
https://crrev.com/1df5b71300297a83efb39b8666ba0ff44369947c
Cr-Commit-Position: refs/heads/master@{#294002}