Change AuthenticatingURLLoader to be a URLLoaderInterceptor

Change AuthenticatingURLLoader to be a URLLoaderInterceptor

This CL changes AuthenticatingURLLoader to be a URLLoaderInterceptor rather
than a one-off interface whose implementation wrapped URLLoaders. Renaming of
various classes/files to more appropriate names will happen in a follow-up CL
to minimize churn.

It also removes the background loader used to load the
authenticating_url_loader app; it is no longer needed as the NetworkService's
URLLoaders are now being used directly again and can fetch the
authenticating_url_loader app.

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/8ca4d652c72233d89dd5c27fdf12810d96ebb7a2

[blundell, 2015-06-04 10:51:28]

blundell@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-06-04 12:25:31]

lgtm

https://codereview.chromium.org/1155283003/diff/80001/mojo/services/authentic...
File
mojo/services/authenticating_url_loader/public/interfaces/authenticating_url_loader_factory.mojom
(right):

https://codereview.chromium.org/1155283003/diff/80001/mojo/services/authentic...
mojo/services/authenticating_url_loader/public/interfaces/authenticating_url_loader_factory.mojom:21:
CreateURLLoaderInterceptorFactory(
I don't expect to need to create this more than once, so you can probably remove
the setter for the authentication service and passing it there.

https://codereview.chromium.org/1155283003/diff/80001/services/authenticating...
File services/authenticating_url_loader/authenticating_url_loader_interceptor.cc
(right):

https://codereview.chromium.org/1155283003/diff/80001/services/authenticating...
services/authenticating_url_loader/authenticating_url_loader_interceptor.cc:79:
StartRequest(BuildRequest(""));
This should probably return null when we do not add a token, to let the network
service continue its redirect chain.

https://codereview.chromium.org/1155283003/diff/80001/services/authenticating...
services/authenticating_url_loader/authenticating_url_loader_interceptor.cc:94:
StartRequest(BuildRequest(""));
This API is incomplete. It seems you need to be able to call follow redirect
here if you do not need to add a token.

[qsr, 2015-06-04 12:26:06]

Sorry, the l.g.t.m. was a mistake

[blundell, 2015-06-04 15:33:04]

Patchset #6 (id:100001) has been deleted

[blundell, 2015-06-04 15:36:36]

In addition to addressing comments, also moved the intelligence from the
AuthicatingURLLoaderFactoryImpl to the AuthenticatingURLLoaderInterceptorFactory
as discussed offline.

https://codereview.chromium.org/1155283003/diff/80001/mojo/services/authentic...
File
mojo/services/authenticating_url_loader/public/interfaces/authenticating_url_loader_factory.mojom
(right):

https://codereview.chromium.org/1155283003/diff/80001/mojo/services/authentic...
mojo/services/authenticating_url_loader/public/interfaces/authenticating_url_loader_factory.mojom:21:
CreateURLLoaderInterceptorFactory(
On 2015/06/04 12:25:31, qsr wrote:
> I don't expect to need to create this more than once, so you can probably
remove
> the setter for the authentication service and passing it there.

Done.

https://codereview.chromium.org/1155283003/diff/80001/services/authenticating...
File services/authenticating_url_loader/authenticating_url_loader_interceptor.cc
(right):

https://codereview.chromium.org/1155283003/diff/80001/services/authenticating...
services/authenticating_url_loader/authenticating_url_loader_interceptor.cc:79:
StartRequest(BuildRequest(""));
On 2015/06/04 12:25:31, qsr wrote:
> This should probably return null when we do not add a token, to let the
network
> service continue its redirect chain.

Done.

https://codereview.chromium.org/1155283003/diff/80001/services/authenticating...
services/authenticating_url_loader/authenticating_url_loader_interceptor.cc:94:
StartRequest(BuildRequest(""));
On 2015/06/04 12:25:31, qsr wrote:
> This API is incomplete. It seems you need to be able to call follow redirect
> here if you do not need to add a token.

Done.

[qsr, 2015-06-05 10:54:55]

https://codereview.chromium.org/1155283003/diff/120001/services/authenticatin...
File services/authenticating_url_loader/authenticating_url_loader_factory_impl.h
(right):

https://codereview.chromium.org/1155283003/diff/120001/services/authenticatin...
services/authenticating_url_loader/authenticating_url_loader_factory_impl.h:22:
: public AuthenticatingURLLoaderFactory {
This class doesn't seem to have any state in the end. You can probably merge it
with the application delegate class, no?

https://codereview.chromium.org/1155283003/diff/120001/services/authenticatin...
services/authenticating_url_loader/authenticating_url_loader_factory_impl.h:39:
std::map<GURL, std::string> cached_accounts_;
You do not seem to be using this anymore.

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
shell/application_manager/application_manager.cc:202:
!EndsWith(resolved_url.path(), "/authentication.mojo", true)) {
I think we should now be able to not need mojo:authenticating_url_loader to be
bundle with the shell. You can use the network service without an interceptor,
so we should be able to get this.

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
File shell/application_manager/application_manager.h (right):

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
shell/application_manager/application_manager.h:245:
mojo::AuthenticatingURLLoaderFactoryPtr url_loader_factory_;
I don't think you need to keep this as a member. You just create it and pass it
along.

[blundell, 2015-06-05 13:17:41]

Patchset #7 (id:140001) has been deleted

[blundell, 2015-06-05 13:24:17]

https://codereview.chromium.org/1155283003/diff/120001/services/authenticatin...
File services/authenticating_url_loader/authenticating_url_loader_factory_impl.h
(right):

https://codereview.chromium.org/1155283003/diff/120001/services/authenticatin...
services/authenticating_url_loader/authenticating_url_loader_factory_impl.h:22:
: public AuthenticatingURLLoaderFactory {
On 2015/06/05 10:54:55, qsr wrote:
> This class doesn't seem to have any state in the end. You can probably merge
it
> with the application delegate class, no?

The application delegate maps the |cached_tokens| maps per-origin in order to be
able to hand out the right set of cached tokens to this class based on the
connecting application's origin.

https://codereview.chromium.org/1155283003/diff/120001/services/authenticatin...
services/authenticating_url_loader/authenticating_url_loader_factory_impl.h:39:
std::map<GURL, std::string> cached_accounts_;
On 2015/06/05 10:54:55, qsr wrote:
> You do not seem to be using this anymore.

Done.

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
shell/application_manager/application_manager.cc:202:
!EndsWith(resolved_url.path(), "/authentication.mojo", true)) {
On 2015/06/05 10:54:55, qsr wrote:
> I think we should now be able to not need mojo:authenticating_url_loader to be
> bundle with the shell. You can use the network service without an interceptor,
> so we should be able to get this.

Done.

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
File shell/application_manager/application_manager.h (right):

https://codereview.chromium.org/1155283003/diff/120001/shell/application_mana...
shell/application_manager/application_manager.h:245:
mojo::AuthenticatingURLLoaderFactoryPtr url_loader_factory_;
On 2015/06/05 10:54:55, qsr wrote:
> I don't think you need to keep this as a member. You just create it and pass
it
> along.

Done.

[qsr, 2015-06-05 14:17:42]

LGTM wiht nit

https://codereview.chromium.org/1155283003/diff/160001/services/authenticatin...
File services/authenticating_url_loader/BUILD.gn (right):

https://codereview.chromium.org/1155283003/diff/160001/services/authenticatin...
services/authenticating_url_loader/BUILD.gn:26:
"//mojo/public/cpp/application:standalone",
Please keep using chromium env. The standalone env is not really production
ready.

[blundell, 2015-06-05 14:24:47]

https://codereview.chromium.org/1155283003/diff/160001/services/authenticatin...
File services/authenticating_url_loader/BUILD.gn (right):

https://codereview.chromium.org/1155283003/diff/160001/services/authenticatin...
services/authenticating_url_loader/BUILD.gn:26:
"//mojo/public/cpp/application:standalone",
On 2015/06/05 14:17:42, qsr wrote:
> Please keep using chromium env. The standalone env is not really production
> ready.

Done.

[blundell, 2015-06-05 14:41:38]

Committed patchset #8 (id:180001) manually as
8ca4d652c72233d89dd5c27fdf12810d96ebb7a2 (presubmit successful).