Enable Mojo to download from file URLs

Mostly useful for testing.

Added default implementations for methods in net::NetworkDelegate to make it easier to implement, and more likely for implementations to be correct.

BUG=310297
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231489

[Aaron Boodman, 2013-10-24 06:08:33]

After Darin's email today about how to run a little localhost for testing, I'm
not sure this is still necessary. Let me know what you think.

[darin (slow to review), 2013-10-24 06:13:25]

This kind of thing still seems useful to me.

https://codereview.chromium.org/38303002/diff/40001/mojo/DEPS
File mojo/DEPS (right):

https://codereview.chromium.org/38303002/diff/40001/mojo/DEPS#newcode5
mojo/DEPS:5: "+net",
Adam said he was trying to hide the net dependency inside the mojo/loader/
directory.

I should have also added a comment in this DEPS file saying that anything you
add here you have to consider subtracting from mojo/public/DEPS :-P

[abarth-chromium, 2013-10-24 14:26:46]

Maybe it's not worth trying to keep the |net| dependencies separate.  I was
thinking we'd want to move the loader into a separate service from the shell,
but we can defer that work...

If we want to keep the dependencies separate, we can move network_delegate.cc
into mojo/loader and just always return |true| from OnCanAccessFile (likely with
a TODO comment).

Either way is fine with me.

[abarth-chromium, 2013-10-24 16:25:22]

https://codereview.chromium.org/38303002/diff/40001/mojo/DEPS
File mojo/DEPS (right):

https://codereview.chromium.org/38303002/diff/40001/mojo/DEPS#newcode5
mojo/DEPS:5: "+net",
LGTM if you move this +net line into mojo/shell/DEPS so that it doesn't leak
into mojo/public.

If you do decide to introduce the net dependency to mojo/shell, we should just
move all the files from mojo/loader into mojo/shell and worry about sorting them
out later.  We can move the files around in a separate CL.

[Aaron Boodman, 2013-10-24 17:53:13]

+willchan for net/

I moved the net dependency down into mojo/shell. Per abarth's suggestion, I will
move loader into shell, but in a separate CL.

[Aaron Boodman, 2013-10-24 21:48:34]

s/willchan/akalin

[willchan no longer on Chromium, 2013-10-24 22:03:36]

Matt's probably more appropriate here. Sorry I didn't redirect sooner, I was in
meetings all day. I already synced up with Matt about this and he's generally in
favor, although he oughta review the default implementations you've inserted
here to make sure they're correct.

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
net/base/network_delegate.cc:132: return net::OK;
no need for net:: prefix everywhere here, since it's already in the net
namescape

[mmenke, 2013-10-24 22:46:23]

I'll look at this tomorrow, but we should have a couple unit tests making sure
the default behavior doesn't break anything.  I'll make specific suggestions
tomorrow.

[akalin, 2013-10-25 13:29:59]

On 2013/10/24 22:46:23, mmenke wrote:
> I'll look at this tomorrow, but we should have a couple unit tests making sure
> the default behavior doesn't break anything.  I'll make specific suggestions
> tomorrow.

Can you add a comment in the CL description about why you're adding default
implementations for network_delegate.h?

[abarth-chromium, 2013-10-25 13:36:01]

On 2013/10/25 13:29:59, akalin wrote:
> Can you add a comment in the CL description about why you're adding default
> implementations for network_delegate.h?

In case Aaron hasn't gotten into the office yet, my understanding is that the
reason is to make it easier to use net.  Mojo currently has a very simple
embedding of net.  We need to provide implementations of these pure virtual
functions.  We could either implement all these defaults in the mojo directory
or we could put them in net where they could be used by other embedders of net
in the future.

[akalin, 2013-10-25 13:42:45]

On 2013/10/25 13:36:01, abarth wrote:
> On 2013/10/25 13:29:59, akalin wrote:
> > Can you add a comment in the CL description about why you're adding default
> > implementations for network_delegate.h?
> 
> In case Aaron hasn't gotten into the office yet, my understanding is that the
> reason is to make it easier to use net.  Mojo currently has a very simple
> embedding of net.  We need to provide implementations of these pure virtual
> functions.  We could either implement all these defaults in the mojo directory
> or we could put them in net where they could be used by other embedders of net
> in the future.

I see! In that case, I'd ask that you'd add comments to network_delegate.h
documenting the default behavior (for functions that return values; I guess one
can just say that all void-returning default implementations simply do nothing).

[mmenke, 2013-10-25 17:59:14]

My suggestions for tests:

Normal request, with body.
Redirect.
Auth required (Should fail).
file:// request for a valid file we have access to (Should fail)
Maybe a request that has cookies set, followed by a request that should send
them?

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
net/base/network_delegate.cc:227: void
NetworkDelegate::NotifyRequestWaitStateChange(const URLRequest& request,
These should be up with the other notify functions, please move them (Know that
they were incorrectly below OnCanEnablePrivacyMode before, but think it's a
minor enough cleanup that including it in this CL is fine).

[Aaron Boodman, 2013-10-25 21:13:00]

On 2013/10/25 13:29:59, akalin wrote:
> On 2013/10/24 22:46:23, mmenke wrote:
> > I'll look at this tomorrow, but we should have a couple unit tests making
sure
> > the default behavior doesn't break anything.  I'll make specific suggestions
> > tomorrow.
> 
> Can you add a comment in the CL description about why you're adding default
> implementations for network_delegate.h?

Done.

[Aaron Boodman, 2013-10-25 21:13:22]

On 2013/10/25 13:42:45, akalin wrote:
> On 2013/10/25 13:36:01, abarth wrote:
> > On 2013/10/25 13:29:59, akalin wrote:
> > > Can you add a comment in the CL description about why you're adding
default
> > > implementations for network_delegate.h?
> > 
> > In case Aaron hasn't gotten into the office yet, my understanding is that
the
> > reason is to make it easier to use net.  Mojo currently has a very simple
> > embedding of net.  We need to provide implementations of these pure virtual
> > functions.  We could either implement all these defaults in the mojo
directory
> > or we could put them in net where they could be used by other embedders of
net
> > in the future.
> 
> I see! In that case, I'd ask that you'd add comments to network_delegate.h
> documenting the default behavior (for functions that return values; I guess
one
> can just say that all void-returning default implementations simply do
nothing).

Done.

[Aaron Boodman, 2013-10-25 21:19:49]

ptal

I believe I have addressed everything except mmenke's suggestions for unit
tests.

I agree there should be unit tests that exercise the various methods of
NetworkDelegate. It's not clear to me how much that is tested currently.

But I don't think there should be unit tests specifically testing the default
values returned by those methods. This would basically be testing that a
function which is hard-coded to return 0 does in fact return 0. That's not the
important thing - the important thing is what when 0 is returned URLRequest does
foo, but when it returns 1, URLRequest does bar.

https://codereview.chromium.org/38303002/diff/40001/mojo/DEPS
File mojo/DEPS (right):

https://codereview.chromium.org/38303002/diff/40001/mojo/DEPS#newcode5
mojo/DEPS:5: "+net",
On 2013/10/24 16:25:23, abarth wrote:
> LGTM if you move this +net line into mojo/shell/DEPS so that it doesn't leak
> into mojo/public.
> 
> If you do decide to introduce the net dependency to mojo/shell, we should just
> move all the files from mojo/loader into mojo/shell and worry about sorting
them
> out later.  We can move the files around in a separate CL.

As we discussed, I moved the net include rule down into mojo/shell. A future
change will move loader into shell.

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
net/base/network_delegate.cc:132: return net::OK;
On 2013/10/24 22:03:36, willchan wrote:
> no need for net:: prefix everywhere here, since it's already in the net
> namescape

Done.

[Aaron Boodman, 2013-10-25 21:21:41]

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/38303002/diff/140001/net/base/network_delegat...
net/base/network_delegate.cc:227: void
NetworkDelegate::NotifyRequestWaitStateChange(const URLRequest& request,
On 2013/10/25 17:59:14, mmenke wrote:
> These should be up with the other notify functions, please move them (Know
that
> they were incorrectly below OnCanEnablePrivacyMode before, but think it's a
> minor enough cleanup that including it in this CL is fine).

Done.

[Aaron Boodman, 2013-10-28 20:15:32]

On Fri, Oct 25, 2013 at 2:19 PM, <aa@chromium.org> wrote:

> I agree there should be unit tests that exercise the various methods of
> NetworkDelegate. It's not clear to me how much that is tested currently.
>
> But I don't think there should be unit tests specifically testing the
> default
> values returned by those methods. This would basically be testing that a
> function which is hard-coded to return 0 does in fact return 0. That's not
> the
> important thing - the important thing is what when 0 is returned
> URLRequest does
> foo, but when it returns 1, URLRequest does bar.


ping.

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

[mmenke, 2013-10-28 20:22:59]

On 2013/10/28 20:15:32, Aaron Boodman wrote:
> On Fri, Oct 25, 2013 at 2:19 PM, <mailto:aa@chromium.org> wrote:
> 
> > I agree there should be unit tests that exercise the various methods of
> > NetworkDelegate. It's not clear to me how much that is tested currently.
> >
> > But I don't think there should be unit tests specifically testing the
> > default
> > values returned by those methods. This would basically be testing that a
> > function which is hard-coded to return 0 does in fact return 0. That's not
> > the
> > important thing - the important thing is what when 0 is returned
> > URLRequest does
> > foo, but when it returns 1, URLRequest does bar.
> 
> 
> ping.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry, heavily loaded with reviews today (And had an interview as well).  I'll
get to this tomorrow.

[willchan no longer on Chromium, 2013-10-28 20:45:33]

Aaron pinged me about this so I took a quick look. It looks like the default
implementations are all effectively declarative (return OK or no-op), and it's
not useful to directly unit test NetworkDelegate itself. What is important to
test is the interaction between the network stack and the NetworkDelegate, which
should be done in url_request_unittest.cc (or wherever the entry point is) using
a mock NetworkDelegate.

I have not investigated whether or not any of these entry points have the
relevant testing. If they do not currently have coverage, I told aa@ offline
that I don't mind imposing some minimal tax burden on developers to fix that,
but would not be comfortable if it were too "steep". I don't have time to check
that out though.

[mmenke, 2013-10-28 20:53:46]

On 2013/10/28 20:45:33, willchan wrote:
> Aaron pinged me about this so I took a quick look. It looks like the default
> implementations are all effectively declarative (return OK or no-op), and it's
> not useful to directly unit test NetworkDelegate itself. What is important to
> test is the interaction between the network stack and the NetworkDelegate,
which
> should be done in url_request_unittest.cc (or wherever the entry point is)
using
> a mock NetworkDelegate.
> 
> I have not investigated whether or not any of these entry points have the
> relevant testing. If they do not currently have coverage, I told aa@ offline
> that I don't mind imposing some minimal tax burden on developers to fix that,
> but would not be comfortable if it were too "steep". I don't have time to
check
> that out though.

My general concern is that if the default NetworkDelegate behavior changes and
something breaks badly, then there's nothing to catch the regression.  I'm not
concerned about the functions that do nothing, but some of the others
(Particularly OnCanAccessFile) make me nervous.

[Aaron Boodman, 2013-10-28 20:55:52]

On Mon, Oct 28, 2013 at 1:53 PM, <mmenke@chromium.org> wrote:

> On 2013/10/28 20:45:33, willchan wrote:
>
>> Aaron pinged me about this so I took a quick look. It looks like the
>> default
>> implementations are all effectively declarative (return OK or no-op), and
>> it's
>> not useful to directly unit test NetworkDelegate itself. What is
>> important to
>> test is the interaction between the network stack and the NetworkDelegate,
>>
> which
>
>> should be done in url_request_unittest.cc (or wherever the entry point is)
>>
> using
>
>> a mock NetworkDelegate.
>>
>
>  I have not investigated whether or not any of these entry points have the
>> relevant testing. If they do not currently have coverage, I told aa@offline
>> that I don't mind imposing some minimal tax burden on developers to fix
>> that,
>> but would not be comfortable if it were too "steep". I don't have time to
>>
> check
>
>> that out though.
>>
>
> My general concern is that if the default NetworkDelegate behavior changes
> and
> something breaks badly, then there's nothing to catch the regression.  I'm
> not
> concerned about the functions that do nothing, but some of the others
> (Particularly OnCanAccessFile) make me nervous.
>

That concern exists even today. The call sites have to handle the situation
of a NULL delegate. If anything, changing this improves the situation
because in a code review, the reviewer can clearly see that a default
behavior is being tested.

- a

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

[willchan no longer on Chromium, 2013-10-28 20:58:01]

I'm trying to understand what can break. Can you elaborate? My understanding is
every existing implementation of NetworkDelegate already explicitly specifies
behavior. Providing a default will not break any of them.

Your concern seems more valid for changing a default in the future, once it
becomes possible for NetworkDelegate implementations that rely on a default.
Does that sound right?

[mmenke, 2013-10-28 20:59:30]

On 2013/10/28 20:58:01, willchan wrote:
> I'm trying to understand what can break. Can you elaborate? My understanding
is
> every existing implementation of NetworkDelegate already explicitly specifies
> behavior. Providing a default will not break any of them.
> 
> Your concern seems more valid for changing a default in the future, once it
> becomes possible for NetworkDelegate implementations that rely on a default.
> Does that sound right?

Yes - my concern is solely about future regressions.  I have no concern about
the current behavior.

[mmenke, 2013-10-28 21:01:05]

On 2013/10/28 20:55:52, Aaron Boodman wrote:
> On Mon, Oct 28, 2013 at 1:53 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > On 2013/10/28 20:45:33, willchan wrote:
> >
> >> Aaron pinged me about this so I took a quick look. It looks like the
> >> default
> >> implementations are all effectively declarative (return OK or no-op), and
> >> it's
> >> not useful to directly unit test NetworkDelegate itself. What is
> >> important to
> >> test is the interaction between the network stack and the NetworkDelegate,
> >>
> > which
> >
> >> should be done in url_request_unittest.cc (or wherever the entry point is)
> >>
> > using
> >
> >> a mock NetworkDelegate.
> >>
> >
> >  I have not investigated whether or not any of these entry points have the
> >> relevant testing. If they do not currently have coverage, I told aa@offline
> >> that I don't mind imposing some minimal tax burden on developers to fix
> >> that,
> >> but would not be comfortable if it were too "steep". I don't have time to
> >>
> > check
> >
> >> that out though.
> >>
> >
> > My general concern is that if the default NetworkDelegate behavior changes
> > and
> > something breaks badly, then there's nothing to catch the regression.  I'm
> > not
> > concerned about the functions that do nothing, but some of the others
> > (Particularly OnCanAccessFile) make me nervous.
> >
> 
> That concern exists even today. The call sites have to handle the situation
> of a NULL delegate. If anything, changing this improves the situation
> because in a code review, the reviewer can clearly see that a default
> behavior is being tested.

I'm not following, unless you're proposing to do away with handling the NULL
delegate case (Which I do think would be reasonable in conjunction to this CL,
but this CL does not do).

[willchan no longer on Chromium, 2013-10-28 21:02:21]

If that's the case, do you fundamentally disagree with providing default
implementations? Or only for specific APIs where you want to make sure the
embedder explicitly specifies behavior for certain delegate callbacks? Or is it
just a concern and we'll have to deal with it if/when we want to change default
behavior in the future (and hope that our various embedders have the appropriate
end to end test coverage to catch any bugs that flipping defaults may
introduce)?

[mmenke, 2013-10-28 21:06:32]

On 2013/10/28 21:02:21, willchan wrote:
> If that's the case, do you fundamentally disagree with providing default
> implementations? Or only for specific APIs where you want to make sure the
> embedder explicitly specifies behavior for certain delegate callbacks? Or is
it
> just a concern and we'll have to deal with it if/when we want to change
default
> behavior in the future (and hope that our various embedders have the
appropriate
> end to end test coverage to catch any bugs that flipping defaults may
> introduce)?

I don't fundamentally disagree with default implementations, but if the default
implementations are non-empty, I think it's reasonable to test the default
implementation, particularly when they make security decisions.  My general
opinion is that if "trivial" code changes and break something, and there's
nothing there that will notice, that's a problem.  Depending on non-NULL,
untested, default implementation behavior seems like a problem to me.  Perhaps
I'm just overly paranoid.

[willchan no longer on Chromium, 2013-10-28 21:10:42]

No, I think that's a reasonable stance. But to clarify, I don't think a
network_delegate_unittest.cc is useful here. Since testing that an interface
function returns true/false is not useful IMO. What's interesting is the
interaction between the network stack and the delegate interface, which is
primarily in URLRequest, and therefore should be tested in
url_request_unittest.cc.

[mmenke, 2013-10-28 21:14:52]

On 2013/10/28 21:10:42, willchan wrote:
> No, I think that's a reasonable stance. But to clarify, I don't think a
> network_delegate_unittest.cc is useful here. Since testing that an interface
> function returns true/false is not useful IMO. What's interesting is the
> interaction between the network stack and the delegate interface, which is
> primarily in URLRequest, and therefore should be tested in
> url_request_unittest.cc.

I agree completely, that's exactly the lines I was thinking along.  Sorry for
any confusion if I gave the impression I wanted a network_delegate_unittest.

I think existing tests could probably just be copy and pasted, for the most
part, with just the delegate changed.

[Aaron Boodman, 2013-10-28 21:21:35]

From a quick scan, I think everything on NetworkDelegate is currently
tested except:

oncanaccessfile
oncanthrottlerequest
oncanenableprivacymode
onbeforesocketstreamconnect

I can look into adding tests for these methods, but I would prefer to do
that as part of a separate change.

I think that it is easy for people who are very familiar with an area of
code to fall into the trap of saying "ah, I should have done that when I
wrote it... can you just throw it in as long as you are in there?". In
reality, I have zero familiarity with the network stack. I would not want
to just copy and paste some tests and try and get it by the reviewer. I'd
want to take some time and do it right.

On the other hand, I could have just implemented a gigantic no-op version
of this interface, and that wouldn't have even shown up on your radar. But
that was not the right change.

- a


On Mon, Oct 28, 2013 at 2:14 PM, <mmenke@chromium.org> wrote:

> On 2013/10/28 21:10:42, willchan wrote:
>
>> No, I think that's a reasonable stance. But to clarify, I don't think a
>> network_delegate_unittest.cc is useful here. Since testing that an
>> interface
>> function returns true/false is not useful IMO. What's interesting is the
>> interaction between the network stack and the delegate interface, which is
>> primarily in URLRequest, and therefore should be tested in
>> url_request_unittest.cc.
>>
>
> I agree completely, that's exactly the lines I was thinking along.  Sorry
> for
> any confusion if I gave the impression I wanted a
> network_delegate_unittest.
>
> I think existing tests could probably just be copy and pasted, for the most
> part, with just the delegate changed.
>
>
https://codereview.chromium.**org/38303002/<https://codereview.chromium.org/3...
>

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

[willchan no longer on Chromium, 2013-10-28 21:41:25]

Great, I think that despite some initial confusion, Matt and I are in agreement
here. Matt, I defer to you on whether or not you want to accept Aaron's proposal
to split out the test changes into a separate CL. FWIW, I'm inclined to do so
for two reasons:
* Being nice to Aaron since it's blocking other CLs of his
* I trust Aaron to follow through with a followup CL. And since there's no
current behavior change (the tests would primarily protect against regressions
in future default behavior changes), it seems OK to have a short window in which
there may be missing test coverage here.

Cheers.

[mmenke, 2013-10-28 21:46:45]

On 2013/10/28 21:41:25, willchan wrote:
> Great, I think that despite some initial confusion, Matt and I are in
agreement
> here. Matt, I defer to you on whether or not you want to accept Aaron's
proposal
> to split out the test changes into a separate CL. FWIW, I'm inclined to do so
> for two reasons:
> * Being nice to Aaron since it's blocking other CLs of his
> * I trust Aaron to follow through with a followup CL. And since there's no
> current behavior change (the tests would primarily protect against regressions
> in future default behavior changes), it seems OK to have a short window in
which
> there may be missing test coverage here.
> 
> Cheers.

I'm fine with tests in another CL, or multiple CLs.  I do think we also want a
case of a request with a default delegate going through successfully, both with
and without a redirect - not terribly exciting, admittedly.

Also worth looking at existing delegates and removing functions identical to the
default values, but no need for Aaron to do that.

LGTM

[commit-bot: I haz the power, 2013-10-28 23:25:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/38303002/460001

[commit-bot: I haz the power, 2013-10-29 02:40:39]

Change committed as 231489