propagate proxy auth info into the browser context.

propagate proxy auth info into the browser context.

BUG=658625, 255258
TEST=Manual

Review-Url: https://codereview.chromium.org/2644713003
Cr-Commit-Position: refs/heads/master@{#444855}
Committed: https://chromium.googlesource.com/chromium/src/+/9514fbd507c6ffb769209244dc083abfc7879a5b

[xiaoyinh(OOO Sep 11-29), 2017-01-18 22:26:12]

Description was changed from

==========
propagate proxy auth info into the browser context.

BUG=658625,255258
TEST=Manual
==========

to

==========
propagate proxy auth info into the browser context.

BUG=658625,255258
TEST=Manual
==========

[xiaoyinh(OOO Sep 11-29), 2017-01-18 22:26:12]

xiaoyinh@chromium.org changed reviewers:
+ nharper@chromium.org, xiyuan@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-01-18 22:26:23]

The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 22:27:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-01-18 22:35:21]

The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 22:35:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-01-18 22:36:40]

xiyuan@, nharper@, please take a look!
I've tested proxy authentication over ethernet and wifi, both seems to be
working.

[commit-bot: I haz the power, 2017-01-18 23:16:18]

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

[commit-bot: I haz the power, 2017-01-18 23:16:19]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-01-18 23:34:09]

Cool. lgtm from my side.

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:120: // the mail
profile to make sure all subsystems of Chrome can access the network
Could you help to update the comment since you are here?

  // .... entered in the login profile in
  // the mail profile ...

=>
  // ... entered in the login profile *to*
  // the *main* profile...

[nharper, 2017-01-19 00:42:18]

I guess it lgtm.

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:135:
content::StoragePartition* signin_partition = login::GetSigninPartition();
Is there a reason for doing this here instead of replacing the
ProfileHelper::GetSigninProfile() call at
TransferContextAuthenticationsOnIOThread's callsite with this? I don't know what
the implications of that would be.

[xiaoyinh(OOO Sep 11-29), 2017-01-19 01:32:08]

xiaoyinh@chromium.org changed reviewers:
+ alemate@google.com

[xiaoyinh(OOO Sep 11-29), 2017-01-19 01:32:08]

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:120: // the mail
profile to make sure all subsystems of Chrome can access the network
On 2017/01/18 23:34:08, xiyuan wrote:
> Could you help to update the comment since you are here?
> 
>   // .... entered in the login profile in
>   // the mail profile ...
> 
> =>
>   // ... entered in the login profile *to*
>   // the *main* profile...

Done.

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:135:
content::StoragePartition* signin_partition = login::GetSigninPartition();
On 2017/01/19 00:42:17, nharper wrote:
> Is there a reason for doing this here instead of replacing the
> ProfileHelper::GetSigninProfile() call at
> TransferContextAuthenticationsOnIOThread's callsite with this? I don't know
what
> the implications of that would be.

Thanks for the review. I just tried the normal login flow(without proxy), and it
didn't go through this function. I'm not sure though if
TransferContextAuthenticationsOnIOThread is only used to transfer proxy auth
data. If that's the case, we probably don't need
ProfileHelper::GetSigninProfile() anymore. Initially, I just don't want  to
break existing auth flow(if any) that are propagated though
ProfileHelper::GetSigninProfile().

xiyuan@, alemate@ might know better the history and usage of GetSigninProfile()
vs GetSigninPartition();

[xiaoyinh(OOO Sep 11-29), 2017-01-19 01:33:16]

The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 01:33:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 02:12:30]

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

[commit-bot: I haz the power, 2017-01-19 02:12:31]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-01-19 17:20:26]

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:135:
content::StoragePartition* signin_partition = login::GetSigninPartition();
On 2017/01/19 01:32:08, xiaoyinh wrote:
> On 2017/01/19 00:42:17, nharper wrote:
> > Is there a reason for doing this here instead of replacing the
> > ProfileHelper::GetSigninProfile() call at
> > TransferContextAuthenticationsOnIOThread's callsite with this? I don't know
> what
> > the implications of that would be.
> 
> Thanks for the review. I just tried the normal login flow(without proxy), and
it
> didn't go through this function. I'm not sure though if
> TransferContextAuthenticationsOnIOThread is only used to transfer proxy auth
> data. If that's the case, we probably don't need
> ProfileHelper::GetSigninProfile() anymore. Initially, I just don't want  to
> break existing auth flow(if any) that are propagated though
> ProfileHelper::GetSigninProfile().
> 
> xiyuan@, alemate@ might know better the history and usage of
GetSigninProfile()
> vs GetSigninPartition();

The Gaia is now served in webview and GetSigninPartition() should handle that.
However, there is still a couple of things use the signin profile directly: the
iframe in ToS page and captive portal window on the login screen. It is probably
safer to copy both.

However, it is a good point to call GetSigninPartition() on the callsite of
TransferContextAuthenticationsOnIOThread.
TransferContextAuthenticationsOnIOThread as the name indicates runs on the IO
thread. And GetSigninPartition() is not thread-safe and probably does not expect
to be called on IO thread. Can we change that?

[xiaoyinh(OOO Sep 11-29), 2017-01-19 19:26:50]

The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 19:27:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-01-19 19:45:32]

On 2017/01/19 17:20:26, xiyuan wrote:
>
https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/existing_user_controller.cc (right):
> 
>
https://codereview.chromium.org/2644713003/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/existing_user_controller.cc:135:
> content::StoragePartition* signin_partition = login::GetSigninPartition();
> On 2017/01/19 01:32:08, xiaoyinh wrote:
> > On 2017/01/19 00:42:17, nharper wrote:
> > > Is there a reason for doing this here instead of replacing the
> > > ProfileHelper::GetSigninProfile() call at
> > > TransferContextAuthenticationsOnIOThread's callsite with this? I don't
know
> > what
> > > the implications of that would be.
> > 
> > Thanks for the review. I just tried the normal login flow(without proxy),
and
> it
> > didn't go through this function. I'm not sure though if
> > TransferContextAuthenticationsOnIOThread is only used to transfer proxy auth
> > data. If that's the case, we probably don't need
> > ProfileHelper::GetSigninProfile() anymore. Initially, I just don't want  to
> > break existing auth flow(if any) that are propagated though
> > ProfileHelper::GetSigninProfile().
> > 
> > xiyuan@, alemate@ might know better the history and usage of
> GetSigninProfile()
> > vs GetSigninPartition();
> 
> The Gaia is now served in webview and GetSigninPartition() should handle that.
> However, there is still a couple of things use the signin profile directly:
the
> iframe in ToS page and captive portal window on the login screen. It is
probably
> safer to copy both.
> 
> However, it is a good point to call GetSigninPartition() on the callsite of
> TransferContextAuthenticationsOnIOThread.
> TransferContextAuthenticationsOnIOThread as the name indicates runs on the IO
> thread. And GetSigninPartition() is not thread-safe and probably does not
expect
> to be called on IO thread. Can we change that?

Done.

[commit-bot: I haz the power, 2017-01-19 19:56:45]

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

[commit-bot: I haz the power, 2017-01-19 19:56:46]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-01-19 20:33:42]

https://codereview.chromium.org/2644713003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2644713003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:125:
net::URLRequestContextGetter* webview_context_getter) {
nit: move |webview_context_getter| before |browser_process_context_getter| so
that input args precede output args.

https://codereview.chromium.org/2644713003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:339: task =
base::Bind(&TransferContextAuthenticationsOnIOThread,
Can we only do base::Bind in one place?

e.g.
  content::StoragePartition* signin_partition = login::GetSigninPartition();
  scoped_refptr<net::URLRequestContextGetter> webview_context_getter;
  if (signin_partition)
    webview_context_getter = ignin_partition->GetURLRequestContext();

  content::BrowserThread::PostDelayedTask(...

[xiaoyinh(OOO Sep 11-29), 2017-01-19 21:13:09]

The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 21:13:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiaoyinh(OOO Sep 11-29), 2017-01-19 21:14:10]

https://codereview.chromium.org/2644713003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/2644713003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:125:
net::URLRequestContextGetter* webview_context_getter) {
On 2017/01/19 20:33:42, xiyuan wrote:
> nit: move |webview_context_getter| before |browser_process_context_getter| so
> that input args precede output args.

Done.

https://codereview.chromium.org/2644713003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:339: task =
base::Bind(&TransferContextAuthenticationsOnIOThread,
On 2017/01/19 20:33:42, xiyuan wrote:
> Can we only do base::Bind in one place?
> 
> e.g.
>   content::StoragePartition* signin_partition = login::GetSigninPartition();
>   scoped_refptr<net::URLRequestContextGetter> webview_context_getter;
>   if (signin_partition)
>     webview_context_getter = ignin_partition->GetURLRequestContext();
> 
>   content::BrowserThread::PostDelayedTask(...

Thanks! Changed.

[xiyuan, 2017-01-19 21:21:16]

lgtm++

[commit-bot: I haz the power, 2017-01-19 21:44:23]

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

[commit-bot: I haz the power, 2017-01-19 21:44:25]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-01-19 21:55:26]

The CQ bit was checked by xiaoyinh@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-01-19 21:55:27]

The patchset sent to the CQ was uploaded after l-g-t-m from nharper@chromium.org
Link to the patchset: https://codereview.chromium.org/2644713003/#ps80001
(title: "comments from xiyuan@")

[commit-bot: I haz the power, 2017-01-19 21:56:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 22:07:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484862926864210,
"parent_rev": "86866d174733b3372662376b3ecfc937727442d2", "commit_rev":
"9514fbd507c6ffb769209244dc083abfc7879a5b"}

[commit-bot: I haz the power, 2017-01-19 22:07:44]

Description was changed from

==========
propagate proxy auth info into the browser context.

BUG=658625,255258
TEST=Manual
==========

to

==========
propagate proxy auth info into the browser context.

BUG=658625,255258
TEST=Manual

Review-Url: https://codereview.chromium.org/2644713003
Cr-Commit-Position: refs/heads/master@{#444855}
Committed:
https://chromium.googlesource.com/chromium/src/+/9514fbd507c6ffb769209244dc08...
==========

[commit-bot: I haz the power, 2017-01-19 22:07:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9514fbd507c6ffb769209244dc08...