Move logic in UserImageScreen from UserImageScreenHandler

Move logic in UserImageScreen from UserImageScreenHandler

R=ygorshenin@chromium.org
BUG=231331
TBR=ben@chromium.org (API usage adjustements in chrome/browser/automation)

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

[Denis Kuznetsov (DE-MUC), 2013-04-17 14:49:16]

self-review

https://codereview.chromium.org/14063017/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/14063017/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/user_image_screen.cc:66: if (actor_)
{}

https://codereview.chromium.org/14063017/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/user_image_screen.cc:251: 
empty line

https://codereview.chromium.org/14063017/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/screens/user_image_screen.h (right):

https://codereview.chromium.org/14063017/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/screens/user_image_screen.h:64: 
empty line

[Nikita (slow), 2013-04-17 20:28:04]

lgtm

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider_chromeos.cc:393:
image_screen->OnImageSelected("", image_type);
Why this call is changed?

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.cc:241: bool
UserImageScreen::profile_picture_absent() {
these seem like simple getters so could be moved to header.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen_actor.h (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen_actor.h:66: virtual void
OnProfileImageAbsent() {}
style guide doesn't allow having virtual function definition in header.
~UserImageScreenActor() already breaks that rule.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:21: #include
"ui/views/widget/widget.h"
nit: not needed

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:102: if
(page_is_ready()) {
nit: drop {}

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:108: if
(page_is_ready()) {
nit: drop {}

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:147: if
(!net::DataURL::Parse(GURL(image_url), &mime_type, &charset, &raw_data))
Should this code be also moved to screen?

[Denis Kuznetsov (DE-MUC), 2013-04-18 02:57:50]

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider_chromeos.cc:393:
image_screen->OnImageSelected("", image_type);
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> Why this call is changed?
Because screen does not have now separate methods for selecting
profile/camera/default image, it has one generic method.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider_chromeos.cc:393:
image_screen->OnImageSelected("", image_type);
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> Why this call is changed?

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.cc:241: bool
UserImageScreen::profile_picture_absent() {
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> these seem like simple getters so could be moved to header.

They are virtual, so they can not be in header.
Anyway, they will be gone once we get a right "init" stage for screens (they're
needed only for init stage that is still in handler).

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:147: if
(!net::DataURL::Parse(GURL(image_url), &mime_type, &charset, &raw_data))
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> Should this code be also moved to screen?
It depends on how we treat it - is passing image in URL a part of JS<->handler
interaction just like wrapping things in base::Value (in this case this code
belongs to handler), or it is a part of higher logic, in this case it belongs to
screen code.

[Denis Kuznetsov (DE-MUC), 2013-04-18 03:00:16]

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/automation/...
chrome/browser/automation/testing_automation_provider_chromeos.cc:393:
image_screen->OnImageSelected("", image_type);
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> Why this call is changed?
And it should actually be followed by image_screen->OnImageAccepted() call.

[Denis Kuznetsov (DE-MUC), 2013-04-18 08:08:29]

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen_actor.h (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen_actor.h:66: virtual void
OnProfileImageAbsent() {}
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> style guide doesn't allow having virtual function definition in header.
> ~UserImageScreenActor() already breaks that rule.

Done.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:21: #include
"ui/views/widget/widget.h"
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> nit: not needed

Done.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:102: if
(page_is_ready()) {
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:108: if
(page_is_ready()) {
On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> nit: drop {}

Done.

[commit-bot: I haz the power, 2013-04-18 08:09:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/14063017/13001

[ygorshenin1, 2013-04-18 08:12:29]

lgtm with nits

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.cc:136: uma_index =
GetDefaultImageHistogramValue(selected_image_);
Add "break" here.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.cc:241: bool
UserImageScreen::profile_picture_absent() {
As they're virtual, their names must be in CamelCase.

On 2013/04/18 02:57:50, Denis Kuznetsov wrote:
> On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> > these seem like simple getters so could be moved to header.
> 
> They are virtual, so they can not be in header.
> Anyway, they will be gone once we get a right "init" stage for screens
(they're
> needed only for init stage that is still in handler).

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen.h (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.h:10: #include
"chrome/browser/chromeos/camera_detector.h"
Move to .cc

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.h:15: #include
"chrome/common/url_constants.h"
Why do we need this?

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.h:16: #include
"content/public/browser/browser_thread.h"
Move to .cc

[Denis Kuznetsov (DE-MUC), 2013-04-18 08:48:45]

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.cc:241: bool
UserImageScreen::profile_picture_absent() {
On 2013/04/18 08:12:30, ygorshenin1 wrote:
> As they're virtual, their names must be in CamelCase.
> 
> On 2013/04/18 02:57:50, Denis Kuznetsov wrote:
> > On 2013/04/17 20:28:04, Nikita Kostylev wrote:
> > > these seem like simple getters so could be moved to header.
> > 
> > They are virtual, so they can not be in header.
> > Anyway, they will be gone once we get a right "init" stage for screens
> (they're
> > needed only for init stage that is still in handler).
> 
They will be gone in couple of days.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/screens/user_image_screen.h (right):

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.h:10: #include
"chrome/browser/chromeos/camera_detector.h"
On 2013/04/18 08:12:30, ygorshenin1 wrote:
> Move to .cc

Done.

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.h:15: #include
"chrome/common/url_constants.h"
On 2013/04/18 08:12:30, ygorshenin1 wrote:
> Why do we need this?

profile_picture_data_url_(chrome::kAboutBlankURL)

https://codereview.chromium.org/14063017/diff/3001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/screens/user_image_screen.h:16: #include
"content/public/browser/browser_thread.h"
On 2013/04/18 08:12:30, ygorshenin1 wrote:
> Move to .cc

Done.

[commit-bot: I haz the power, 2013-04-18 11:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/14063017/27001

[commit-bot: I haz the power, 2013-04-18 14:19:04]

Change committed as 194917