Show Captcha dialog.

Show Captcha dialog.
Refactor ImageDownloader/ImageDecoder into a separate classes.

Resources submitted in http://codereview.chromium.org/2856003

BUG= http://crosbug.com/3486
TEST=Select "New User" screen, enter new account for this device. Enter incorrect password 3 times. After captcha is presented and entered login should succeed.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=49965

[sky, 2010-06-14 17:38:46]

http://codereview.chromium.org/2729020/diff/5001/6004
File chrome/browser/chromeos/image_decoder.h (right):

http://codereview.chromium.org/2729020/diff/5001/6004#newcode17
chrome/browser/chromeos/image_decoder.h:17: class ImageDecoder : public
UtilityProcessHost::Client {
Are we only going to use this and image_downloader during login? If so, move
them into the login directory.

http://codereview.chromium.org/2729020/diff/5001/6006
File chrome/browser/chromeos/image_downloader.h (right):

http://codereview.chromium.org/2729020/diff/5001/6006#newcode28
chrome/browser/chromeos/image_downloader.h:28: // It's a reference counted
object, so destructor is private.
This object isn't reference counted, which likely means you leak it now.

http://codereview.chromium.org/2729020/diff/5001/6009
File chrome/browser/chromeos/login/captcha_view.h (right):

http://codereview.chromium.org/2729020/diff/5001/6009#newcode42
chrome/browser/chromeos/login/captcha_view.h:42: ~CaptchaView() {}
virtual

http://codereview.chromium.org/2729020/diff/5001/6009#newcode46
chrome/browser/chromeos/login/captcha_view.h:46: MessageBoxFlags::DialogButton
button) const { return L""; }
std::wstring

[tfarina, 2010-06-14 20:23:02]

Some style comments below.

-Thiago

http://codereview.chromium.org/2729020/diff/5001/6004
File chrome/browser/chromeos/image_decoder.h (right):

http://codereview.chromium.org/2729020/diff/5001/6004#newcode23
chrome/browser/chromeos/image_decoder.h:23: protected:
please add a blank line above.

http://codereview.chromium.org/2729020/diff/5001/6004#newcode35
chrome/browser/chromeos/image_decoder.h:35: ~ImageDecoder() {}
virtual ?

http://codereview.chromium.org/2729020/diff/5001/6004#newcode37
chrome/browser/chromeos/image_decoder.h:37: // UtilityProcessHost::Client:
// Overridden from ...

http://codereview.chromium.org/2729020/diff/5001/6009
File chrome/browser/chromeos/login/captcha_view.h (right):

http://codereview.chromium.org/2729020/diff/5001/6009#newcode1
chrome/browser/chromeos/login/captcha_view.h:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/2729020/diff/5001/6009#newcode10
chrome/browser/chromeos/login/captcha_view.h:10: //#include "base/string16.h"
remove this "comment"?

http://codereview.chromium.org/2729020/diff/5001/6009#newcode11
chrome/browser/chromeos/login/captcha_view.h:11: #include "googleurl/src/gurl.h"
sort this alphabetical.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode16
chrome/browser/chromeos/login/captcha_view.h:16: 
remove this blank line

http://codereview.chromium.org/2729020/diff/5001/6009#newcode21
chrome/browser/chromeos/login/captcha_view.h:21: }
// namespace views

http://codereview.chromium.org/2729020/diff/5001/6009#newcode44
chrome/browser/chromeos/login/captcha_view.h:44: // views::DialogDelegate
methods.
methods -> overrides.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode52
chrome/browser/chromeos/login/captcha_view.h:52: // views::WindowDelegate
method.
methods -> overrides.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode59
chrome/browser/chromeos/login/captcha_view.h:59: // views::Textfield::Controller
methods.
methods -> overrides

http://codereview.chromium.org/2729020/diff/5001/6010
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/2729020/diff/5001/6010#newcode34
chrome/browser/chromeos/login/existing_user_controller.cc:34: using
views::WidgetGtk;
please remove this, and just use views::WidgetGtk below.

http://codereview.chromium.org/2729020/diff/5001/6019
File chrome/browser/chromeos/login/user_image_downloader.cc (right):

http://codereview.chromium.org/2729020/diff/5001/6019#newcode81
chrome/browser/chromeos/login/user_image_downloader.cc:81: new
ImageDownloader(this, GURL(image_url), auth_token_);
Weird, hum, can't we initialize this statically instead?

[Nikita (slow), 2010-06-15 17:10:27]

I've uploaded resources in a separate CL

http://codereview.chromium.org/2729020/diff/5001/6004
File chrome/browser/chromeos/image_decoder.h (right):

http://codereview.chromium.org/2729020/diff/5001/6004#newcode17
chrome/browser/chromeos/image_decoder.h:17: class ImageDecoder : public
UtilityProcessHost::Client {
On 2010/06/14 17:38:46, sky wrote:
> Are we only going to use this and image_downloader during login? If so, move
> them into the login directory.

Done.

http://codereview.chromium.org/2729020/diff/5001/6004#newcode23
chrome/browser/chromeos/image_decoder.h:23: protected:
On 2010/06/14 20:23:02, tfarina wrote:
> please add a blank line above.

Done.

http://codereview.chromium.org/2729020/diff/5001/6004#newcode35
chrome/browser/chromeos/image_decoder.h:35: ~ImageDecoder() {}
On 2010/06/14 20:23:02, tfarina wrote:
> virtual ?

Done.

http://codereview.chromium.org/2729020/diff/5001/6004#newcode37
chrome/browser/chromeos/image_decoder.h:37: // UtilityProcessHost::Client:
On 2010/06/14 20:23:02, tfarina wrote:
> // Overridden from ...

Done.

http://codereview.chromium.org/2729020/diff/5001/6006
File chrome/browser/chromeos/image_downloader.h (right):

http://codereview.chromium.org/2729020/diff/5001/6006#newcode28
chrome/browser/chromeos/image_downloader.h:28: // It's a reference counted
object, so destructor is private.
On 2010/06/14 17:38:46, sky wrote:
> This object isn't reference counted, which likely means you leak it now.

Done.

http://codereview.chromium.org/2729020/diff/5001/6009
File chrome/browser/chromeos/login/captcha_view.h (right):

http://codereview.chromium.org/2729020/diff/5001/6009#newcode1
chrome/browser/chromeos/login/captcha_view.h:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
On 2010/06/14 20:23:02, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode10
chrome/browser/chromeos/login/captcha_view.h:10: //#include "base/string16.h"
On 2010/06/14 20:23:02, tfarina wrote:
> remove this "comment"?

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode11
chrome/browser/chromeos/login/captcha_view.h:11: #include "googleurl/src/gurl.h"
On 2010/06/14 20:23:02, tfarina wrote:
> sort this alphabetical.

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode16
chrome/browser/chromeos/login/captcha_view.h:16: 
On 2010/06/14 20:23:02, tfarina wrote:
> remove this blank line

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode21
chrome/browser/chromeos/login/captcha_view.h:21: }
On 2010/06/14 20:23:02, tfarina wrote:
> // namespace views

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode42
chrome/browser/chromeos/login/captcha_view.h:42: ~CaptchaView() {}
On 2010/06/14 17:38:46, sky wrote:
> virtual

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode44
chrome/browser/chromeos/login/captcha_view.h:44: // views::DialogDelegate
methods.
On 2010/06/14 20:23:02, tfarina wrote:
> methods -> overrides.

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode46
chrome/browser/chromeos/login/captcha_view.h:46: MessageBoxFlags::DialogButton
button) const { return L""; }
On 2010/06/14 17:38:46, sky wrote:
> std::wstring

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode52
chrome/browser/chromeos/login/captcha_view.h:52: // views::WindowDelegate
method.
On 2010/06/14 20:23:02, tfarina wrote:
> methods -> overrides.

Done.

http://codereview.chromium.org/2729020/diff/5001/6009#newcode59
chrome/browser/chromeos/login/captcha_view.h:59: // views::Textfield::Controller
methods.
On 2010/06/14 20:23:02, tfarina wrote:
> methods -> overrides

It's implementation.

http://codereview.chromium.org/2729020/diff/5001/6010
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/2729020/diff/5001/6010#newcode34
chrome/browser/chromeos/login/existing_user_controller.cc:34: using
views::WidgetGtk;
On 2010/06/14 20:23:02, tfarina wrote:
> please remove this, and just use views::WidgetGtk below.

Done.

http://codereview.chromium.org/2729020/diff/5001/6019
File chrome/browser/chromeos/login/user_image_downloader.cc (right):

http://codereview.chromium.org/2729020/diff/5001/6019#newcode81
chrome/browser/chromeos/login/user_image_downloader.cc:81: new
ImageDownloader(this, GURL(image_url), auth_token_);
On 2010/06/14 20:23:02, tfarina wrote:
> Weird, hum, can't we initialize this statically instead?

Not really, since it's will be reference counted and created for a single image
download once we know that URL.

[sky, 2010-06-15 19:01:26]

http://codereview.chromium.org/2729020/diff/15001/16013
File chrome/browser/chromeos/login/image_downloader.cc (right):

http://codereview.chromium.org/2729020/diff/15001/16013#newcode27
chrome/browser/chromeos/login/image_downloader.cc:27: image_fetcher_.reset(new
URLFetcher(GURL(image_url), URLFetcher::GET, this));
Is this object getting deleted? URLFetcher's delegate isn't ref counted, so the
url fetcher won't release it when done. I suspect you don't need this object to
be ref counted, just delete it in OnURLFetchComplete.

[Nikita (slow), 2010-06-16 15:34:33]

http://codereview.chromium.org/2729020/diff/15001/16013
File chrome/browser/chromeos/login/image_downloader.cc (right):

http://codereview.chromium.org/2729020/diff/15001/16013#newcode27
chrome/browser/chromeos/login/image_downloader.cc:27: image_fetcher_.reset(new
URLFetcher(GURL(image_url), URLFetcher::GET, this));
On 2010/06/15 19:01:26, sky wrote:
> Is this object getting deleted? URLFetcher's delegate isn't ref counted, so
the
> url fetcher won't release it when done. I suspect you don't need this object
to
> be ref counted, just delete it in OnURLFetchComplete.

Done.

[sky, 2010-06-16 16:58:15]

LGTM