Fix so that login errors are shown from login/oobe screens....

Fix so that login errors are shown from login/oobe screens.
Split functionality of check for login failures to NetworkLoginObserver from NetworkMessageObserver.
BUG=chromium-os:8251
TEST=manually make sure the login error dialog pops up when you enter bad password.

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

[stevenjb, 2011-01-11 20:53:52]

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:1869: NetworkLoginObserver*
network_login_observer_;
Maybe use scoped_ptr<NetworkLoginObserver>?

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
File chrome/browser/chromeos/network_login_observer.cc (right):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:23: initialized_ = true;
nit: Rather than tracking an initiailized_ state, the "Refresh stored networks"
section of OnNetworkManagerChanged() could be moved to a separate private method
and called instead of OnNetworkManagerChanged() here.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:34: if (browser &&
browser->type() != Browser::TYPE_NORMAL) {
FindBrowserWithType() searches in order of last activation, so we should be able
to skip the GetLastActive() call and always use FindBrowserWithType().

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:44: return;
nit: return statement is unnecessary.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:95:
wifi_networks_[wifi->service_path()] = new WifiNetwork(*wifi);
We are doing a lot of unnecessary copying of WifiNetwork in both
NetworkLoginObserver and NetworkMessageObserver when we only need to save error_
and state_. It's a small optimization, but given how often this could happen, it
might be worth the effort. I will leave it up to you.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
File chrome/browser/chromeos/network_message_observer.cc (left):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_message_observer.cc:129: continue;
Don't we still need this check?

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_message_observer.cc:141: 
If we remove this check here, we might end up displaying a notification as well
as a dialog. This actually seems like better behavior, but you might want to run
it by Kan.

[Charlie Lee, 2011-01-11 22:52:06]

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:1869: NetworkLoginObserver*
network_login_observer_;
On 2011/01/11 20:53:52, Steven Bennetts wrote:
> Maybe use scoped_ptr<NetworkLoginObserver>?

Done.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
File chrome/browser/chromeos/network_login_observer.cc (right):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:23: initialized_ = true;
On 2011/01/11 20:53:52, Steven Bennetts wrote:
> nit: Rather than tracking an initiailized_ state, the "Refresh stored
networks"
> section of OnNetworkManagerChanged() could be moved to a separate private
method
> and called instead of OnNetworkManagerChanged() here.

Done.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:34: if (browser &&
browser->type() != Browser::TYPE_NORMAL) {
FindBrowserWithType takes in a profile which we get from the browser object. So
we can't call it without getting an initial browser.

I copied this code from elsewhere, so i think it's the right way to do it.

On 2011/01/11 20:53:52, Steven Bennetts wrote:
> FindBrowserWithType() searches in order of last activation, so we should be
able
> to skip the GetLastActive() call and always use FindBrowserWithType().

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:44: return;
On 2011/01/11 20:53:52, Steven Bennetts wrote:
> nit: return statement is unnecessary.

Done.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:95:
wifi_networks_[wifi->service_path()] = new WifiNetwork(*wifi);
On 2011/01/11 20:53:52, Steven Bennetts wrote:
> We are doing a lot of unnecessary copying of WifiNetwork in both
> NetworkLoginObserver and NetworkMessageObserver when we only need to save
error_
> and state_. It's a small optimization, but given how often this could happen,
it
> might be worth the effort. I will leave it up to you.

Done.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
File chrome/browser/chromeos/network_message_observer.cc (left):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_message_observer.cc:129: continue;
No, because wifi_old->connecting() below means that it wasn't previously failed

On 2011/01/11 20:53:52, Steven Bennetts wrote:
> Don't we still need this check?

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_message_observer.cc:141: 
Let me think about this. For this change, I don't want to change the behavior.

On 2011/01/11 20:53:52, Steven Bennetts wrote:
> If we remove this check here, we might end up displaying a notification as
well
> as a dialog. This actually seems like better behavior, but you might want to
run
> it by Kan.

[stevenjb, 2011-01-12 01:34:52]

LGTM if we decide we want to show a notification along with the dialog on a bad
passphrase error.

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
File chrome/browser/chromeos/network_login_observer.cc (right):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_login_observer.cc:34: if (browser &&
browser->type() != Browser::TYPE_NORMAL) {
I forgot to add that we would need to use ProfileManager::GetDefaultProfile()
instead of b->profile().

I searched through the code and it looks like this is something we ought to
clean up consistently throughout, so I am fine with leaving it as-is for now.


On 2011/01/11 22:52:06, Charlie Lee wrote:
> FindBrowserWithType takes in a profile which we get from the browser object.
So
> we can't call it without getting an initial browser.
> 
> I copied this code from elsewhere, so i think it's the right way to do it.
> 
> On 2011/01/11 20:53:52, Steven Bennetts wrote:
> > FindBrowserWithType() searches in order of last activation, so we should be
> able
> > to skip the GetLastActive() call and always use FindBrowserWithType().
>

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
File chrome/browser/chromeos/network_message_observer.cc (left):

http://codereview.chromium.org/6027012/diff/27001/chrome/browser/chromeos/net...
chrome/browser/chromeos/network_message_observer.cc:129: continue;
Ah, right, OK.

On 2011/01/11 22:52:06, Charlie Lee wrote:
> No, because wifi_old->connecting() below means that it wasn't previously
failed
> 
> On 2011/01/11 20:53:52, Steven Bennetts wrote:
> > Don't we still need this check?
>