Add a hook for capturing a user profile photo and saving it to file and local state.

Add a hook for capturing a user profile photo and saving it to file and local state.

BUG=chromium-os:16729
TEST=PyAuto test


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

[dennis_jeffrey, 2011-07-29 20:10:06]

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers.h:963: // and returns
the status to automation provider
nit: add period at end of sentence

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:365:
//TODO(frankf): Currently, we do not destroy TakePhotoDialog
nit: add space after the '//', and add a period at the end of the sentence in
the next line below

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:378:
AutomationJSONReply(automation_, reply_message_.release()).SendError("Capture
failure");
line too long; 80 chars max

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:388:
DCHECK(user_manager);
instead of a DCHECK, maybe we could send an informative error reply message
instead (same at line 391 below), since DCHECKs are only used in Debug builds.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:396:
AutomationJSONReply(automation_, reply_message_.release()).SendSuccess(NULL);
Rather than returning success here, shouldn't we wait to make sure that the
OnCaptureSuccess event occurs first?  Or are we guaranteed that it occurs before
we get to this point?

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:1013: 
remove this blank line

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:1023:
take_photo_dialog);
fit this on the previous line

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/login/t...
File chrome/browser/chromeos/login/take_photo_view.h (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/login/t...
chrome/browser/chromeos/login/take_photo_view.h:71: // For automation purposes
nit: add period at end of line.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/options...
File chrome/browser/chromeos/options/take_photo_dialog.h (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/take_photo_dialog.h:62: // to receive
notification for capture success/failure
nit: add period at end of sentence

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
File chrome/test/functional/chromeos_prefs.py (right):

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
chrome/test/functional/chromeos_prefs.py:23: k_user_images =
ChromeosPrefsTest.k_user_images
nit: maybe we can avoid defining these two variables and just use
"ChromeosPrefsTest.k_logged_in_users" and "ChromeosPrefsTest.k_user_images"
wherever necessary.  Same comment for lnes 41-42 below.

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
chrome/test/functional/chromeos_prefs.py:52: self.assertTrue(expected_photo_name
in current_image_path)
I recommend adding a "msg='...'" parameter to this assertion that prints a
descriptive error message in the event this assertion fails.  Same at line 36
above.

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
chrome/test/functional/chromeos_prefs.py:57: 
nit: remove this blank line

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py...
chrome/test/pyautolib/pyauto.py:3612: 
Maybe mention here where the expected file is created and what its name is.

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py...
chrome/test/pyautolib/pyauto.py:3617: return
self._GetResultFromJSONRequest(cmd_dict)
Is something important actually returned here?  The sample test doesn't seem to
check for a return value, and there's not "Returns:" section in the docstring
above to describe what is returned.

[frankf, 2011-07-29 21:50:40]

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers.h:963: // and returns
the status to automation provider
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> nit: add period at end of sentence

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:365:
//TODO(frankf): Currently, we do not destroy TakePhotoDialog
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> nit: add space after the '//', and add a period at the end of the sentence in
> the next line below

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:378:
AutomationJSONReply(automation_, reply_message_.release()).SendError("Capture
failure");
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> line too long; 80 chars max

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:388:
DCHECK(user_manager);
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> instead of a DCHECK, maybe we could send an informative error reply message
> instead (same at line 391 below), since DCHECKs are only used in Debug builds.

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/autom...
chrome/browser/automation/automation_provider_observers_chromeos.cc:396:
AutomationJSONReply(automation_, reply_message_.release()).SendSuccess(NULL);
We reach this point only when we execute PhotoCaptureObserver::OnCaptureSuccess.


On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> Rather than returning success here, shouldn't we wait to make sure that the
> OnCaptureSuccess event occurs first?  Or are we guaranteed that it occurs
before
> we get to this point?

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:1013: 
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> remove this blank line

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:1023:
take_photo_dialog);
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> fit this on the previous line

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/login/t...
File chrome/browser/chromeos/login/take_photo_view.h (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/login/t...
chrome/browser/chromeos/login/take_photo_view.h:71: // For automation purposes
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> nit: add period at end of line.

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/options...
File chrome/browser/chromeos/options/take_photo_dialog.h (right):

http://codereview.chromium.org/7523063/diff/1/chrome/browser/chromeos/options...
chrome/browser/chromeos/options/take_photo_dialog.h:62: // to receive
notification for capture success/failure
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> nit: add period at end of sentence

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
File chrome/test/functional/chromeos_prefs.py (right):

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
chrome/test/functional/chromeos_prefs.py:23: k_user_images =
ChromeosPrefsTest.k_user_images
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> nit: maybe we can avoid defining these two variables and just use
> "ChromeosPrefsTest.k_logged_in_users" and "ChromeosPrefsTest.k_user_images"
> wherever necessary.  Same comment for lnes 41-42 below.

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
chrome/test/functional/chromeos_prefs.py:52: self.assertTrue(expected_photo_name
in current_image_path)
Yes, that's very useful.

On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> I recommend adding a "msg='...'" parameter to this assertion that prints a
> descriptive error message in the event this assertion fails.  Same at line 36
> above.

http://codereview.chromium.org/7523063/diff/1/chrome/test/functional/chromeos...
chrome/test/functional/chromeos_prefs.py:57: 
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> nit: remove this blank line

Done.

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py...
chrome/test/pyautolib/pyauto.py:3612: 
I've added more info here. The exact path to file is in local state.

On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> Maybe mention here where the expected file is created and what its name is.

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py...
chrome/test/pyautolib/pyauto.py:3617: return
self._GetResultFromJSONRequest(cmd_dict)
No there is no return value. However, all the functions have this statement
regardless of whether they're returning anything. 
On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> Is something important actually returned here?  The sample test doesn't seem
to
> check for a return value, and there's not "Returns:" section in the docstring
> above to describe what is returned.

[dennis_jeffrey, 2011-07-29 23:11:46]

LGTM

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523063/diff/1/chrome/test/pyautolib/pyauto.py...
chrome/test/pyautolib/pyauto.py:3617: return
self._GetResultFromJSONRequest(cmd_dict)
On 2011/07/29 21:50:40, frankf wrote:
> No there is no return value. However, all the functions have this statement
> regardless of whether they're returning anything. 
> On 2011/07/29 20:10:06, dennis_jeffrey wrote:
> > Is something important actually returned here?  The sample test doesn't seem
> to
> > check for a return value, and there's not "Returns:" section in the
docstring
> > above to describe what is returned.
> 

I think it's kind of inconsistent in this file.  For example, "SetReleaseTrack"
above doesn't have the return.  I think it's ok as is - just seems unnecessary
to have the return.

[Nirnimesh, 2011-07-30 10:32:31]

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:30: #include
"chrome/browser/chromeos/options/take_photo_dialog.h"
Including the .h might not be necessary.

Just forward declaring TakePhotoDialog and TakePhotoView might work

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:395: const
chromeos::UserManager::User& user = user_manager->logged_in_user();
It's possible to reach here if user_manager == NULL. The AutomationJSONReply
above responds back to the client but does not return from this function.

You should 'delete this; return;' in all the error conditions.

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:1014: new
chromeos::TakePhotoDialog(NULL);
who deletes this object?

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/take_photo_view.cc (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/take_photo_view.cc:292: DCHECK(sender ==
snapshot_button_);
You'd need to get owners review from someone in
chrome/browser/chromeos/login/OWNERS

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
File chrome/test/functional/chromeos_prefs.py (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:14: k_logged_in_users = 'LoggedInUsers'
I don't think python constants need to have a k_ prefix

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:17: default_image_prefix = 'default:'
nah.. move it into testDefaultUserImage

image1 = u'default:4'	
image2 = u'default:5'

... is fine I think

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:47:
self.RestartBrowser(clear_profile=False)
Should you verify before restarting the browser too?

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:54: msg='Captured user photo was not
set in preferences.')
Can you also verify the file on disk?

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3611: """Captures user profile photo.
Append: on chromeos.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3613: This is done by driving the UI. The
profile photo is saved in a file
which UI. be specific.
Prefs profile photo capture UI

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3614: and its path is set in the local state.
append: preferences

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3615: 
Should the user be logged in? mention it so

[frankf, 2011-08-01 19:31:09]

@Anton: Please take a look at the change to take_photo_view and perhaps
take_photo_dialog

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:30: #include
"chrome/browser/chromeos/options/take_photo_dialog.h"
I don't think you can forward declare a nested class (TakePhotoDialog::Observer)

On 2011/07/30 10:32:31, Nirnimesh wrote:
> Including the .h might not be necessary.
> 
> Just forward declaring TakePhotoDialog and TakePhotoView might work

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers_chromeos.cc
(right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers_chromeos.cc:395: const
chromeos::UserManager::User& user = user_manager->logged_in_user();
On 2011/07/30 10:32:31, Nirnimesh wrote:
> It's possible to reach here if user_manager == NULL. The AutomationJSONReply
> above responds back to the client but does not return from this function.
> 
> You should 'delete this; return;' in all the error conditions.

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:1014: new
chromeos::TakePhotoDialog(NULL);
The UI is not destroyed (This is documented in the Observer's destructor). Right
now, restarting the browser does the job. This helps in debugging when running
the individual test case since we can switch to the browser screen and see the
TakePhotoDialog.

On 2011/07/30 10:32:31, Nirnimesh wrote:
> who deletes this object?

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
File chrome/test/functional/chromeos_prefs.py (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:14: k_logged_in_users = 'LoggedInUsers'
This was to match variable name in user_manager.cc

On 2011/07/30 10:32:31, Nirnimesh wrote:
> I don't think python constants need to have a k_ prefix

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:17: default_image_prefix = 'default:'
On 2011/07/30 10:32:31, Nirnimesh wrote:
> nah.. move it into testDefaultUserImage
> 
> image1 = u'default:4'	
> image2 = u'default:5'
> 
> ... is fine I think

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:47:
self.RestartBrowser(clear_profile=False)
On 2011/07/30 10:32:31, Nirnimesh wrote:
> Should you verify before restarting the browser too?

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/functional/chrom...
chrome/test/functional/chromeos_prefs.py:54: msg='Captured user photo was not
set in preferences.')
On 2011/07/30 10:32:31, Nirnimesh wrote:
> Can you also verify the file on disk?

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto.py
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3611: """Captures user profile photo.
On 2011/07/30 10:32:31, Nirnimesh wrote:
> Append: on chromeos.

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3613: This is done by driving the UI. The
profile photo is saved in a file
On 2011/07/30 10:32:31, Nirnimesh wrote:
> which UI. be specific.
> Prefs profile photo capture UI

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3614: and its path is set in the local state.
On 2011/07/30 10:32:31, Nirnimesh wrote:
> append: preferences

Done.

http://codereview.chromium.org/7523063/diff/6002/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyauto.py:3615: 
On 2011/07/30 10:32:31, Nirnimesh wrote:
> Should the user be logged in? mention it so

Done.

[whywhat, 2011-08-02 15:17:02]

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/take_photo_view.h (right):

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/take_photo_view.h:72: virtual void
ButtonPressed();
Shouldn't be virtual.
Should be private with PhotoCaptureObserver being friend.
Should be named FlipCapturingState or smth like that to better reflect what it
does since it's no longer a handler for button being pressed.

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/opti...
File chrome/browser/chromeos/options/take_photo_dialog.h (right):

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/opti...
chrome/browser/chromeos/options/take_photo_dialog.h:65: virtual void
OnCaptureSuccess(
Could you add comments about each method. When exactly it is called.

[whywhat, 2011-08-02 15:17:35]

LGTM

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:970: virtual void
OnCaptureSuccess(
OVERRIDE should be added to each method prototype.

[Nirnimesh, 2011-08-02 19:18:20]

LGTM. Please use the trybots before committing.

[frankf, 2011-08-03 01:41:48]

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/automation/au...
File chrome/browser/automation/automation_provider_observers.h (right):

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/automation/au...
chrome/browser/automation/automation_provider_observers.h:970: virtual void
OnCaptureSuccess(
On 2011/08/02 15:17:35, whywhat wrote:
> OVERRIDE should be added to each method prototype.

Done.

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/take_photo_view.h (right):

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/take_photo_view.h:72: virtual void
ButtonPressed();
On 2011/08/02 15:17:02, whywhat wrote:
> Shouldn't be virtual.
> Should be private with PhotoCaptureObserver being friend.
> Should be named FlipCapturingState or smth like that to better reflect what it
> does since it's no longer a handler for button being pressed.

Done.

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/opti...
File chrome/browser/chromeos/options/take_photo_dialog.h (right):

http://codereview.chromium.org/7523063/diff/5005/chrome/browser/chromeos/opti...
chrome/browser/chromeos/options/take_photo_dialog.h:65: virtual void
OnCaptureSuccess(
On 2011/08/02 15:17:02, whywhat wrote:
> Could you add comments about each method. When exactly it is called.

Done.

[commit-bot: I haz the power, 2011-08-03 21:01:17]

Change committed as 95310