Removing last usage of default request context. Fixing up login_utils to not need it by allowing ac…

Removing last usage of default request context. Fixing up login_utils to not need it by adding a notification so that login_utils only accesses it when the request context getter has been initialized.

BUG=125292, 127693
TEST=unittest, manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145969

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146610

[willchan no longer on Chromium, 2012-06-28 23:28:18]

Thanks for trying to fix this! I'd prefer not to expose HasMainRequestContext()
if possible. Is there a reason that this is necessary?

It looks to me like the LoginUtils stuff is trying to preconnect if the default
profile has already been initialized (and thus has the main
URLRequestContextGetter). Can this be called before the profile has been
initialized? I think a better solution would be not to preconnect until the
URLRequestContextGetter becomes available. I'd prefer that when the
URLRequestContextGetter becomes available, we pass it directly to this
LoginUtils stuff. Otherwise, it's also acceptable to listen for the notification
that the URLRequestContextGetter is available
(NOTIFY_DEFAULT_REQUEST_CONTEXT_AVAILABLE). I don't like indirect notifications
as much, but that's still preferable to the current state (which I'm glad you're
fixing).

[rpetterson, 2012-06-29 00:49:41]

Nikita, can you answer the question about calling it before the profile has been
initialized?

Will, the problem I'm seeing is that this actually gets called after the request
context has been deleted. So if we just call "GetRequestContext" then we end up
creating a new one at a time when that won't work because we're shutting down.
By adding an accessor to HasMainRequestContext(), I can check if it already
exists. An alternative I thought of was to pass "create" to GetRequestContext,
but I didn't want to propagate that through the code.

What about a separate function GetRequestContextWithoutCreating()? This wouldn't
require exposing HasMainRequestContext and would move the check for that
internal to ProfileImpl. I think that might actually be a cleaner, less invasive
method.

On 2012/06/28 23:28:18, willchan wrote:
> Thanks for trying to fix this! I'd prefer not to expose
HasMainRequestContext()
> if possible. Is there a reason that this is necessary?
> 
> It looks to me like the LoginUtils stuff is trying to preconnect if the
default
> profile has already been initialized (and thus has the main
> URLRequestContextGetter). Can this be called before the profile has been
> initialized? I think a better solution would be not to preconnect until the
> URLRequestContextGetter becomes available. I'd prefer that when the
> URLRequestContextGetter becomes available, we pass it directly to this
> LoginUtils stuff. Otherwise, it's also acceptable to listen for the
notification
> that the URLRequestContextGetter is available
> (NOTIFY_DEFAULT_REQUEST_CONTEXT_AVAILABLE). I don't like indirect
notifications
> as much, but that's still preferable to the current state (which I'm glad
you're
> fixing).

[Nikita (slow), 2012-06-29 17:06:04]

On 2012/06/29 00:49:41, rpetterson wrote:
> Nikita, can you answer the question about calling it before the profile has
been
> initialized?

No, PrewarmAuthentication() is only supposed to be called after default profile
is initialized.
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...

[rpetterson, 2012-06-29 21:54:41]

Updated with the notification way of doing it.

[willchan no longer on Chromium, 2012-06-29 21:57:31]

LGTM, thanks for the cleanup!

[rpetterson, 2012-06-29 22:10:03]

Adding thakis for chrome/common owners approval.

[Nico, 2012-06-29 22:11:31]

lgtm

[commit-bot: I haz the power, 2012-06-29 22:43:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10657009/18002

[commit-bot: I haz the power, 2012-06-29 23:01:47]

Try job failure for 10657009-18002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Nikita (slow), 2012-07-02 05:33:44]

lgtm

[commit-bot: I haz the power, 2012-07-10 20:42:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10657009/40002

[commit-bot: I haz the power, 2012-07-10 22:35:52]

Change committed as 145969

[rpetterson, 2012-07-10 23:57:04]

Removing last usage of default request context. Fixing up login_utils to not
need it by adding a notification so that login_utils only accesses it when the
request context getter has been initialized. 

BUG=125292, 127693
TEST=unittest, manual 

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145969

[rpetterson, 2012-07-12 20:38:39]

Nikita, I had to add a bit more to the login_utils. PTAL?

[Nikita (slow), 2012-07-13 12:08:11]

lgtm

http://codereview.chromium.org/10657009/diff/17025/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/10657009/diff/17025/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils.cc:244: if (g_browser_process) {
What were those tests when this failed?

[rpetterson, 2012-07-13 16:43:48]

On 2012/07/13 12:08:11, Nikita Kostylev wrote:
> lgtm
> 
>
http://codereview.chromium.org/10657009/diff/17025/chrome/browser/chromeos/lo...
> File chrome/browser/chromeos/login/login_utils.cc (right):
> 
>
http://codereview.chromium.org/10657009/diff/17025/chrome/browser/chromeos/lo...
> chrome/browser/chromeos/login/login_utils.cc:244: if (g_browser_process) {
> What were those tests when this failed?

BrowserGuestSessionNavigatorTest.Disposition_Settings_UseIncognitoWindow
LoginImplUtils::Get() is called from SetUpCommandLine after g_browser_process
has been set to NULL  (InProcessBrowserTest::SetUp()).

[commit-bot: I haz the power, 2012-07-13 16:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10657009/17025

[commit-bot: I haz the power, 2012-07-13 17:37:45]

Try job failure for 10657009-17025 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-13 17:52:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/10657009/17025

[commit-bot: I haz the power, 2012-07-13 19:16:20]

Change committed as 146610