Register application connections under their post-redirect URL.

Register application connections under their post-redirect URL.

R=davemoore@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/df23d13b8217d04fcf8dfc9e3f1cf452b7a71c2a

[Aaron Boodman, 2015-02-23 23:38:02]

Patchset #1 (id:1) has been deleted

[Aaron Boodman, 2015-02-23 23:38:07]

Patchset #1 (id:20001) has been deleted

[Aaron Boodman, 2015-02-24 19:24:58]

Patchset #1 (id:40001) has been deleted

[Aaron Boodman, 2015-02-24 19:27:02]

aa@chromium.org changed reviewers:
+ davemoore@chromium.org

[Aaron Boodman, 2015-02-24 19:27:02]

Not sure how to test this. Suggestions welcome.

[DaveMoore, 2015-02-24 20:42:59]

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
File shell/application_manager/fetcher.h (right):

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
shell/application_manager/fetcher.h:43: // Returns the original URL that was
fetched.
Wouldn't it be better to just have one method on fetcher called GetURL()? The
local one would always return the requested one and the network implementation
would return the redirect url if there was one, otherwise the requested.

[Aaron Boodman, 2015-02-24 21:48:25]

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
File shell/application_manager/fetcher.h (right):

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
shell/application_manager/fetcher.h:43: // Returns the original URL that was
fetched.
On 2015/02/24 20:42:59, DaveMoore wrote:
> Wouldn't it be better to just have one method on fetcher called GetURL()? The
> local one would always return the requested one and the network implementation
> would return the redirect url if there was one, otherwise the requested.

Whether a redirect happened is important information -- it affects what
ApplicationManager does. Current CL encodes this information in whether
GetRedirectURL() returns a non-empty URL. It could also be encoded in a separate
bit ... but I kinda prefer it as it is.

[DaveMoore, 2015-02-24 22:36:43]

lgtm

[qsr, 2015-02-25 14:50:27]

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

[qsr, 2015-02-25 14:50:27]

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
File shell/application_manager/network_fetcher.cc (right):

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
shell/application_manager/network_fetcher.cc:53: return
GURL(response_->redirect_url);
Did you test this?

The doc for those fields are:

  // These fields are set to non-NULL if this response corresponds to a
  // redirect.  Call the |FollowRedirect| method on the URLLoader instance to
  // follow this redirect.
  string? redirect_method;
  string? redirect_url;

 So my understanding is that if you ask the url loader to follow redirect, it
will handle those by himself, and those fields would always be null. If you want
to use those, you need to handle redirect by yourself (and as you said, I think
this is exactly what we should do anyway)

[Aaron Boodman, 2015-02-25 18:43:08]

I ended up just adding proper support for redirects in this CL as that was
trivial now.

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
File shell/application_manager/network_fetcher.cc (right):

https://codereview.chromium.org/951643002/diff/60001/shell/application_manage...
shell/application_manager/network_fetcher.cc:53: return
GURL(response_->redirect_url);
On 2015/02/25 14:50:27, qsr wrote:
> Did you test this?
> 
> The doc for those fields are:
> 
>   // These fields are set to non-NULL if this response corresponds to a
>   // redirect.  Call the |FollowRedirect| method on the URLLoader instance to
>   // follow this redirect.
>   string? redirect_method;
>   string? redirect_url;
> 
>  So my understanding is that if you ask the url loader to follow redirect, it
> will handle those by himself, and those fields would always be null. If you
want
> to use those, you need to handle redirect by yourself (and as you said, I
think
> this is exactly what we should do anyway)

Ah, you are right. Fixed.

[Aaron Boodman, 2015-02-25 20:42:26]

The CQ bit was checked by aa@chromium.org

[Aaron Boodman, 2015-02-25 20:42:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
davemoore@chromium.org
Link to the patchset: https://codereview.chromium.org/951643002/#ps120001
(title: "cleanup")

[commit-bot: I haz the power, 2015-02-25 20:45:16]

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

[commit-bot: I haz the power, 2015-02-25 20:45:16]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Aaron Boodman, 2015-02-25 21:43:27]

Committed patchset #4 (id:120001) manually as
df23d13b8217d04fcf8dfc9e3f1cf452b7a71c2a (presubmit successful).

[qsr, 2015-02-26 10:41:57]

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

https://codereview.chromium.org/951643002/diff/120001/shell/application_manag...
shell/application_manager/application_manager.cc:241: // And around we go
again... Whee!
Is there any advantage to try to use the url_loader FollowRedirect method? Given
that it will greatly increase the complexity, I'm not sure it is worth it, but
just asking.

https://codereview.chromium.org/951643002/diff/120001/shell/application_manag...
shell/application_manager/application_manager.cc:289: cleanup_behavior,
base::Passed(fetcher.Pass())));
I think this is an undefined behavior. You have no guarantee on the order of ->
and .Pass() on fetcher.