[cros] Initial WebRTC-enabled implementation of user image picker on OOBE.

[cros] Initial WebRTC-enabled implementation of user image picker on OOBE.


BUG=122764
TEST=Manual with --enable-media-source --enable-new-oobe --enable-html5-camera


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

[Ivan Korotkov, 2012-06-07 13:46:26]

PTAL

This is a prototype, lacking:
 (*) detection of camera stream loss
 (*) animated capture
 (*) camera pre-selection.

I'm planning to add same functionality to Preferences as well, and unify most of
the code in oobe_screen_user_image.js and options2/ to some shared source.

[Ivan Korotkov, 2012-06-07 13:47:02]

Also, please, disregard changes in UserManager, they're for testing.

[Nikita (slow), 2012-06-07 15:26:54]

http://codereview.chromium.org/10532048/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_manager_impl.cc:214: if
(command_line->HasSwitch(switches::kStubUser))
Please combine with condition above.
It makes sense to support stub user only if
(!base::chromeos::IsRunningOnChromeOS())

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:495: * .online: camera is
streaming video;
How online / live are different?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:505: display: block;
Please make sure that doesn't affect existing layout (with new camera being
disabled).

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:521: height: 220px;
Btw, why w-h is 220x220 when we capture 160x160?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:528: display: block;
nit: May as well add separate style that defines display: none; with old camera
type. Looks easier to cleanup.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:536: left: -16.6%;
-webkit-margin-start

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:547: left: 14px;
-webkit-margin-start

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:549: top: 14px;
-webkit-margin-before for consistency

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:578: display: block;
nit: Same here, makes sense to define "display: none;" as a part of old camera
type rule.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/oobe.js (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.js:266:
Oobe.getInstance().showScreen({'id': 'user-image'});
Remove: test change.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:35:
console.log('WebRTC: ' + isWebRTC);
Remove console output.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:390:
this.checkVideo_();
Not sure if this the right place for this check.

" $('user-image-stream').src = window.webkitURL.createObjectURL(stream);"
When we start OOBE / sign in screen this will be executed, right?

It makes sense to do camera check but defer initializing
$('user-image-stream').src till onBeforeShow?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:489: {width:
320, height: 320});
nit: Extract constants.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:519: var W =
video.videoWidth, H = video.videoHeight;
Split into 2 lines.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:519: var W =
video.videoWidth, H = video.videoHeight;
rename to width/height in lowercase

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:587:
//this.checkVideo_();
Remove or add a second part of the initialization (see comment above).

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:620:
checkVideo_: function() {
Rename to checkCameraPresence?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:759:
'setCameraPresent',
setCameraPresent_ is not defined for UserImageScreenWebRTCProto

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:7: #include
"base/command_line.h"
nit: sort

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:69:
localized_strings->SetString("userImagesGridSrc", "../user_images_grid.js");
Is this somekind of hack till code is refactored into a shared JS?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:252:
CheckCameraPresence();
Remove?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:263: // For
WebRTC, camera presence checked is done on JS side.
You don't need this check here as you CamerDetector wouldn't be started in the
first place.

http://codereview.chromium.org/10532048/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10532048/diff/1/chrome/common/chrome_switches....
chrome/common/chrome_switches.cc:1158: const char kStubUser[]                   
  = "stub-user";
Comment?

[Nikita (slow), 2012-06-09 09:23:43]

http://codereview.chromium.org/10532048/diff/8016/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10532048/diff/8016/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:539: const char kEnableHtml5Camera[]           
 = "enable-html5-camera";
Please flag to about:flags that would define 2 switches:
--enable-media-source --enable-html5-camera

[Nikita (slow), 2012-06-09 09:29:01]

http://codereview.chromium.org/10532048/diff/8016/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10532048/diff/8016/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:539: const char kEnableHtml5Camera[]           
 = "enable-html5-camera";
On 2012/06/09 09:23:43, Nikita Kostylev wrote:
> Please flag to about:flags that would define 2 switches:
> --enable-media-source --enable-html5-camera

Actually only --enable-html5-camera is now needed.

[Ivan Korotkov, 2012-06-09 14:19:01]

PTAL

*) Moved image decoding to sandboxed worker (not sure if needed anyway...)
*) Cleanup and addressed comments
*) Camera preselection

It's probably too errly to add --enable-html5-camera to about:flags until
there's support for chrome:// WebRTC whitelisting.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_manager_impl.cc:214: if
(command_line->HasSwitch(switches::kStubUser))
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Please combine with condition above.
> It makes sense to support stub user only if
> (!base::chromeos::IsRunningOnChromeOS())

I actually don't need this flag anymore, removed.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:495: * .online: camera is
streaming video;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> How online / live are different?

.online means that camera is working (i.e. present and initialized)
.live means that camera is in live mode (shown to user); when user takes a
picture, streaming is not stopped (camera remains .online but no longer .live
and is hidden)

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:505: display: block;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Please make sure that doesn't affect existing layout (with new camera being
> disabled).

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:528: display: block;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> nit: May as well add separate style that defines display: none; with old
camera
> type. Looks easier to cleanup.

It should already be display: none in base style (so that it's only shown camera
is live), why add another style for old type?

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:536: left: -16.6%;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> -webkit-margin-start

No need in RTL support as this is symmetric (left == right)

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:547: left: 14px;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> -webkit-margin-start

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:549: top: 14px;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> -webkit-margin-before for consistency

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe.css:578: display: block;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> nit: Same here, makes sense to define "display: none;" as a part of old camera
> type rule.

Same reasoning

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:35:
console.log('WebRTC: ' + isWebRTC);
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Remove console output.

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:390:
this.checkVideo_();
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Not sure if this the right place for this check.
> 
> " $('user-image-stream').src = window.webkitURL.createObjectURL(stream);"
> When we start OOBE / sign in screen this will be executed, right?
> 
> It makes sense to do camera check but defer initializing
> $('user-image-stream').src till onBeforeShow?

Right, fixed.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:489: {width:
320, height: 320});
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> nit: Extract constants.

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:519: var W =
video.videoWidth, H = video.videoHeight;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Split into 2 lines.

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:519: var W =
video.videoWidth, H = video.videoHeight;
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> rename to width/height in lowercase

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:587:
//this.checkVideo_();
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Remove or add a second part of the initialization (see comment above).

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:759:
'setCameraPresent',
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> setCameraPresent_ is not defined for UserImageScreenWebRTCProto

It's not a problem as long as it doesn't get called from WebUI

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:7: #include
"base/command_line.h"
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> nit: sort

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:69:
localized_strings->SetString("userImagesGridSrc", "../user_images_grid.js");
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Is this somekind of hack till code is refactored into a shared JS?

Obsolete hack, removed.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:252:
CheckCameraPresence();
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Remove?

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:263: // For
WebRTC, camera presence checked is done on JS side.
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> You don't need this check here as you CamerDetector wouldn't be started in the
> first place.

Done.

http://codereview.chromium.org/10532048/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10532048/diff/1/chrome/common/chrome_switches....
chrome/common/chrome_switches.cc:1158: const char kStubUser[]                   
  = "stub-user";
On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> Comment?

Removed

[Ivan Korotkov, 2012-06-09 16:04:49]

If not available, camera is now polled every N secs on image screen.
N == 3 is ok or maybe 1 sec?..

[Ivan Korotkov, 2012-06-09 16:27:51]

Probably the last thing for this CL: added camera liveness check (will be
removed if stream stops for some reason).

[ygorshenin1, 2012-06-12 22:19:28]

http://codereview.chromium.org/10532048/diff/34/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/34/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:22: #include
"googleurl/src/gurl.h"
Sort, please.

http://codereview.chromium.org/10532048/diff/34/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:209:
DCHECK_EQ(mime_type, "image/png");
nit: put expected value before actual.

http://codereview.chromium.org/10532048/diff/34/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/10532048/diff/34/chrome/common/chrome_switches...
chrome/common/chrome_switches.h:313: extern const char kStubUser[];
As you removed kStubUser declaration, maybe it makes sense to remove it's
definition?

[Nikita (slow), 2012-06-13 10:28:56]

On 2012/06/09 16:04:49, Ivan Korotkov wrote:
> If not available, camera is now polled every N secs on image screen.
> N == 3 is ok or maybe 1 sec?..

3 seconds seems fine.

[Ivan Korotkov, 2012-06-13 14:02:23]

http://codereview.chromium.org/10532048/diff/34/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/34/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:22: #include
"googleurl/src/gurl.h"
On 2012/06/12 22:19:28, ygorshenin1 wrote:
> Sort, please.

Done.

http://codereview.chromium.org/10532048/diff/34/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:209:
DCHECK_EQ(mime_type, "image/png");
On 2012/06/12 22:19:28, ygorshenin1 wrote:
> nit: put expected value before actual.

Done.

http://codereview.chromium.org/10532048/diff/34/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/10532048/diff/34/chrome/common/chrome_switches...
chrome/common/chrome_switches.h:313: extern const char kStubUser[];
On 2012/06/12 22:19:28, ygorshenin1 wrote:
> As you removed kStubUser declaration, maybe it makes sense to remove it's
> definition?

Done.

[Nikita (slow), 2012-06-13 14:07:35]

http://codereview.chromium.org/10532048/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/10532048/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user_manager_impl.cc:214: if
(command_line->HasSwitch(switches::kStubUser))
On 2012/06/09 14:19:01, Ivan Korotkov wrote:
> On 2012/06/07 15:26:55, Nikita Kostylev wrote:
> > Please combine with condition above.
> > It makes sense to support stub user only if
> > (!base::chromeos::IsRunningOnChromeOS())
> 
> I actually don't need this flag anymore, removed.

Actually we have a bug to support such flag.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (left):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:359:
is_current_user_ephemeral_ = true;
This should probably be kept as is.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:43: var
CAMERA_CHECK_INTERVAL_MS = 1000;
3000ms

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:685:
this.cameraPresent = false;
$('user-image-preview').classList.remove('online'); ?

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:214:
image_decoder_ = new ImageDecoder(this, raw_data);
As discussed, should fix leaking of ImageDecoder + reset delegate_ when existing
image_decoder_ is discarded or UserImageScreenHandler is destroyed.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:253:
screen_->OnPhotoTaken(user_photo_);
As discussed, should fix leaking of ImageDecoder + reset delegate_ when existing
image_decoder_ is discarded or UserImageScreenHandler is destroyed.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:290:
screen_->OnPhotoTaken(user_photo_);
accept_photo_after_decoding_ = false;

[Nikita (slow), 2012-06-13 14:08:36]

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:253:
screen_->OnPhotoTaken(user_photo_);
Wrong comment. Correct one:

accept_photo_after_decoding_ = false;

[Ivan Korotkov, 2012-06-13 14:26:06]

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (left):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:359:
is_current_user_ephemeral_ = true;
On 2012/06/13 14:07:35, Nikita Kostylev wrote:
> This should probably be kept as is.

Done.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_user_image.js (right):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:43: var
CAMERA_CHECK_INTERVAL_MS = 1000;
On 2012/06/13 14:07:35, Nikita Kostylev wrote:
> 3000ms

Done.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_user_image.js:685:
this.cameraPresent = false;
On 2012/06/13 14:07:35, Nikita Kostylev wrote:
> $('user-image-preview').classList.remove('online'); ?

Done.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:214:
image_decoder_ = new ImageDecoder(this, raw_data);
On 2012/06/13 14:07:35, Nikita Kostylev wrote:
> As discussed, should fix leaking of ImageDecoder + reset delegate_ when
existing
> image_decoder_ is discarded or UserImageScreenHandler is destroyed.

Done.

http://codereview.chromium.org/10532048/diff/1020/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:290:
screen_->OnPhotoTaken(user_photo_);
On 2012/06/13 14:07:35, Nikita Kostylev wrote:
> accept_photo_after_decoding_ = false;

As discussed, this logic is the right one.

[Nikita (slow), 2012-06-13 14:32:59]

lgtm

http://codereview.chromium.org/10532048/diff/10038/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/10038/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:47:
UserImageScreenHandler::~UserImageScreenHandler() {
 if (image_decoder_.get())
   image_decoder_->set_delegate(NULL);

http://codereview.chromium.org/10532048/diff/10038/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:288:
image_decoder_.release();
image_decoder_ = NULL;

[Ivan Korotkov, 2012-06-13 14:40:55]

http://codereview.chromium.org/10532048/diff/10038/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/10532048/diff/10038/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:47:
UserImageScreenHandler::~UserImageScreenHandler() {
On 2012/06/13 14:32:59, Nikita Kostylev wrote:
>  if (image_decoder_.get())
>    image_decoder_->set_delegate(NULL);

Done.

http://codereview.chromium.org/10532048/diff/10038/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:288:
image_decoder_.release();
On 2012/06/13 14:32:59, Nikita Kostylev wrote:
> image_decoder_ = NULL;

Done.

[ygorshenin1, 2012-06-13 14:41:16]

lgtm

[commit-bot: I haz the power, 2012-06-13 17:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10532048/3008

[commit-bot: I haz the power, 2012-06-13 19:22:04]

Change committed as 141931