Simplify adding callbacks.

Simplify adding callbacks.

BUG=231331
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194563

[bartfab (slow), 2013-04-16 17:04:35]

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/base_screen_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler.h:14: #include
"base/values.h"
Where is this needed? The values used in this file are forward-declared in lines
21-23.

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler.h:116:
base::Bind(method, base::Unretained(static_cast<T*>(this))));
Argument alignment here and throughout the file:

1/ "name" and "base:Bind(...) should be aligned with each other.
2/ The indentation when breaking an argument list should be four spaces, not
two.

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:64: inline
void CallbackWrapper0(base::Callback<void()> callback,
#include "base/callback.h"

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:66:
DCHECK(args && args->empty());
Nit 1: #include "base/logging.h"
Nit 2: Split into two DCHECKs sot that if something fails, we know which one
failed.

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:73:
DCHECK(args && args->GetSize() == 1);
Nit 1: Split into two DCHECKs
Nit 2: Use DCHECK_EQ:

DCHECK(args);
DCHECK_EQ(1, args->GetSize());

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:85:
DCHECK(args && args->GetSize() == 2);
Nit 1: Split into two DCHECKs
Nit 2: Use DCHECK_EQ:

DCHECK(args);
DCHECK_EQ(2, args->GetSize());

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:98:
DCHECK(args && args->GetSize() == 3);
Nit 1: Split into two DCHECKs
Nit 2: Use DCHECK_EQ:

DCHECK(args);
DCHECK_EQ(3, args->GetSize());

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:114:
DCHECK(args && args->GetSize() == 4);
Nit 1: Split into two DCHECKs
Nit 2: Use DCHECK_EQ:

DCHECK(args);
DCHECK_EQ(4, args->GetSize());

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:14: class ListValue;
Here and in the other files touched by this CL: Remove the forward-declarations
where no longer needed.

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:66: void
HandleUpdateCurrentScreen(const std::string& screen);
#include <string>

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.cc:214:
std::string manager_username = gaia::SanitizeEmail(raw_manager_username);
Nit: const

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h
(right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h:39:
const std::string& managed_user_password,
Since you are fixing up includes anyway: #include <string>

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.h:31: void
HandleNetworkDropdownShow(const std::string& element_id,
#include <string>

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:51: void
HandleOnLanguageChanged(const std::string& locale);
#include <string>

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:11: #include
"base/values.h"
Nit: No longer needed.

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1049:
std::string sanitized_email = gaia::SanitizeEmail(typed_email);
Nit: const

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1062:
std::string sanitized_email = gaia::SanitizeEmail(email);
Nit: const

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.h:60: void
HandlePhotoTaken(const std::string& image_url);
#include <string>

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chromeos/login/wrong_hwid_screen_handler.cc
(right):

https://chromiumcodereview.appspot.com/14208014/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/chromeos/login/wrong_hwid_screen_handler.cc:7: #include
"base/values.h"
Nit: No longer used.

[ygorshenin1, 2013-04-16 17:52:43]

PTAL

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/base_screen_handler.h (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler.h:14: #include
"base/values.h"
On 2013/04/16 17:04:35, bartfab wrote:
> Where is this needed? The values used in this file are forward-declared in
lines
> 21-23.

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler.h:116:
base::Bind(method, base::Unretained(static_cast<T*>(this))));
On 2013/04/16 17:04:35, bartfab wrote:
> Argument alignment here and throughout the file:
> 
> 1/ "name" and "base:Bind(...) should be aligned with each other.
> 2/ The indentation when breaking an argument list should be four spaces, not
> two.

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:64: inline
void CallbackWrapper0(base::Callback<void()> callback,
On 2013/04/16 17:04:35, bartfab wrote:
> #include "base/callback.h"

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:66:
DCHECK(args && args->empty());
On 2013/04/16 17:04:35, bartfab wrote:
> Nit 1: #include "base/logging.h"
> Nit 2: Split into two DCHECKs sot that if something fails, we know which one
> failed.

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:66:
DCHECK(args && args->empty());
On 2013/04/16 17:04:35, bartfab wrote:
> Nit 1: #include "base/logging.h"
> Nit 2: Split into two DCHECKs sot that if something fails, we know which one
> failed.

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:73:
DCHECK(args && args->GetSize() == 1);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit 1: Split into two DCHECKs
> Nit 2: Use DCHECK_EQ:
> 
> DCHECK(args);
> DCHECK_EQ(1, args->GetSize());

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:85:
DCHECK(args && args->GetSize() == 2);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit 1: Split into two DCHECKs
> Nit 2: Use DCHECK_EQ:
> 
> DCHECK(args);
> DCHECK_EQ(2, args->GetSize());

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:98:
DCHECK(args && args->GetSize() == 3);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit 1: Split into two DCHECKs
> Nit 2: Use DCHECK_EQ:
> 
> DCHECK(args);
> DCHECK_EQ(3, args->GetSize());

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/base_screen_handler_utils.h:114:
DCHECK(args && args->GetSize() == 4);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit 1: Split into two DCHECKs
> Nit 2: Use DCHECK_EQ:
> 
> DCHECK(args);
> DCHECK_EQ(4, args->GetSize());

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:14: class ListValue;
On 2013/04/16 17:04:35, bartfab wrote:
> Here and in the other files touched by this CL: Remove the
forward-declarations
> where no longer needed.

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:66: void
HandleUpdateCurrentScreen(const std::string& screen);
On 2013/04/16 17:04:35, bartfab wrote:
> #include <string>

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.cc
(right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.cc:214:
std::string manager_username = gaia::SanitizeEmail(raw_manager_username);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit: const

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h
(right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h:39:
const std::string& managed_user_password,
On 2013/04/16 17:04:35, bartfab wrote:
> Since you are fixing up includes anyway: #include <string>

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:51: void
HandleOnLanguageChanged(const std::string& locale);
On 2013/04/16 17:04:35, bartfab wrote:
> #include <string>

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.cc:11: #include
"base/values.h"
On 2013/04/16 17:04:35, bartfab wrote:
> Nit: No longer needed.

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1049:
std::string sanitized_email = gaia::SanitizeEmail(typed_email);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit: const

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1062:
std::string sanitized_email = gaia::SanitizeEmail(email);
On 2013/04/16 17:04:35, bartfab wrote:
> Nit: const

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.h (right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.h:60: void
HandlePhotoTaken(const std::string& image_url);
On 2013/04/16 17:04:35, bartfab wrote:
> #include <string>

Done.

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/wrong_hwid_screen_handler.cc
(right):

https://codereview.chromium.org/14208014/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/wrong_hwid_screen_handler.cc:7: #include
"base/values.h"
On 2013/04/16 17:04:35, bartfab wrote:
> Nit: No longer used.

Done.

[ygorshenin1, 2013-04-16 18:59:06]

+ Nikita

[Denis Kuznetsov (DE-MUC), 2013-04-17 08:32:26]

https://codereview.chromium.org/14208014/diff/10001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/base_screen_handler.h (right):

https://codereview.chromium.org/14208014/diff/10001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/base_screen_handler.h:112: void
AddCallback(const std::string& name,
As now we have fast and nice way to unwrap parameters, it may be reasonable to
rename original non-unwrapped methods (e.g. AddRawCallback).
It can be done in separate CL though.

[bartfab (slow), 2013-04-17 08:50:05]

lgtm with a couple of minor nits.

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h
(right):

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h:83:
void HandleCheckLocallyManagedUserName(const string16& name);
Nit: #include "base/string16.h"

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.h:31: void
HandleNetworkDropdownShow(const std::string& element_id,
Nit: #include <string>

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/reset_screen_handler.h:14: class
DictionaryValue;
Nit: Can you remove the DictionaryValue as well? I don't see it being used
anywhere.

[ygorshenin1, 2013-04-17 10:06:34]

Thanks!

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/login/base_screen_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/base_screen_handler.h:112: void
AddCallback(const std::string& name,
Great idea, Done.

On 2013/04/17 08:32:26, Denis Kuznetsov wrote:
> As now we have fast and nice way to unwrap parameters, it may be reasonable to
> rename original non-unwrapped methods (e.g. AddRawCallback).
> It can be done in separate CL though.

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
File
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h
(right):

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/locally_managed_user_creation_screen_handler.h:83:
void HandleCheckLocallyManagedUserName(const string16& name);
On 2013/04/17 08:50:05, bartfab wrote:
> Nit: #include "base/string16.h"

Done.

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.h (right):

https://chromiumcodereview.appspot.com/14208014/diff/10001/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.h:31: void
HandleNetworkDropdownShow(const std::string& element_id,
On 2013/04/17 08:50:05, bartfab wrote:
> Nit: #include <string>

Done.

[ygorshenin1, 2013-04-17 10:25:06]

Committed patchset #3 manually as r194563 (presubmit successful).

[Nikita (slow), 2013-04-17 12:37:14]

lgtm