Enabling access notifications for login wizard

Enabling access notifications for login wizard


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

[dmazzoni, 2010-09-22 21:20:27]

http://codereview.chromium.org/3442011/diff/33001/34002
File chrome/browser/chromeos/login/existing_user_view.h (right):

http://codereview.chromium.org/3442011/diff/33001/34002#newcode26
chrome/browser/chromeos/login/existing_user_view.h:26:
WizardAccessibilityHelper::accelerator, false, true, true)) {}
How about if WizardAccessibilityHelper::accelerator is the actual
views::Accelerator object, rather than just the key.

That would cut down on duplication - if we wanted to change the set of modifier
keys we'd have to change it in more than one place as it is now.

http://codereview.chromium.org/3442011/diff/33001/34016
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3442011/diff/33001/34016#newcode43
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:43: for
(std::map<views::View*, ViewDetails*>::iterator iter =
Use STLDeleteValues(&views_buffer_) from stl_util-inl.h

[Chaitanya, 2010-09-22 22:43:09]

http://codereview.chromium.org/3442011/diff/33001/34002
File chrome/browser/chromeos/login/existing_user_view.h (right):

http://codereview.chromium.org/3442011/diff/33001/34002#newcode26
chrome/browser/chromeos/login/existing_user_view.h:26:
WizardAccessibilityHelper::accelerator, false, true, true)) {}
On 2010/09/22 21:20:27, Dominic Mazzoni wrote:
> How about if WizardAccessibilityHelper::accelerator is the actual
> views::Accelerator object, rather than just the key.
> 
> That would cut down on duplication - if we wanted to change the set of
modifier
> keys we'd have to change it in more than one place as it is now.
> 

Done.

http://codereview.chromium.org/3442011/diff/33001/34016
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3442011/diff/33001/34016#newcode43
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:43: for
(std::map<views::View*, ViewDetails*>::iterator iter =
On 2010/09/22 21:20:27, Dominic Mazzoni wrote:
> Use STLDeleteValues(&views_buffer_) from stl_util-inl.h

Done.

[dmazzoni, 2010-09-23 16:27:51]

http://codereview.chromium.org/3442011/diff/11003/53017
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3442011/diff/11003/53017#newcode29
chrome/browser/chromeos/login/wizard_accessibility_helper.h:29:
ViewDetails(Profile* profile, bool enabled) :
I don't think it's necessary to store the profile for each view.

In the context of the wizard, there's only going to be one profile. We just need
to save it once.

I think instead of ViewDetails and the map from View to ViewDetails you could
just keep a single set<View*> of views that have been created but haven't yet
been made accessible.

[oshima, 2010-09-23 17:06:53]

http://codereview.chromium.org/3442011/diff/11003/53001
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53001#newcode116
chrome/browser/chromeos/login/existing_user_view.cc:116: void
ExistingUserView::ViewHierarchyChanged(bool is_add, View* parent,
move all arguments to the next single line.

http://codereview.chromium.org/3442011/diff/11003/53001#newcode120
chrome/browser/chromeos/login/existing_user_view.cc:120:
ProfileManager::GetDefaultProfile());
this get executed everytime a view is added to the hierachy this belongs. Is
that what you want? (same for other ViewHierachyChanged)

http://codereview.chromium.org/3442011/diff/11003/53002
File chrome/browser/chromeos/login/existing_user_view.h (right):

http://codereview.chromium.org/3442011/diff/11003/53002#newcode9
chrome/browser/chromeos/login/existing_user_view.h:9: #include
"chrome/browser/chromeos/login/wizard_accessibility_helper.h"
it's probably better to move this and constructor to .cc to reduce dependency on
header.

http://codereview.chromium.org/3442011/diff/11003/53002#newcode41
chrome/browser/chromeos/login/existing_user_view.h:41: virtual void
ViewHierarchyChanged(bool is_add, View* parent, View* child);
don't you have to say views::View?

http://codereview.chromium.org/3442011/diff/11003/53002#newcode51
chrome/browser/chromeos/login/existing_user_view.h:51:
DISALLOW_COPY_AND_ASSIGN(ExistingUserView);
move this to private: (or if there is a reason, write a comment)

http://codereview.chromium.org/3442011/diff/11003/53003
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53003#newcode345
chrome/browser/chromeos/login/network_selection_view.cc:345: void
NetworkSelectionView::ViewHierarchyChanged(bool is_add, View* parent,
same here

http://codereview.chromium.org/3442011/diff/11003/53004
File chrome/browser/chromeos/login/network_selection_view.h (right):

http://codereview.chromium.org/3442011/diff/11003/53004#newcode74
chrome/browser/chromeos/login/network_selection_view.h:74: 
remove extra file

http://codereview.chromium.org/3442011/diff/11003/53007
File chrome/browser/chromeos/login/password_changed_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53007#newcode69
chrome/browser/chromeos/login/password_changed_view.cc:69:
ProfileManager::GetDefaultProfile());
this will be called multiple times. Is it safe?
Can this be inside (is_add && child==this)?

http://codereview.chromium.org/3442011/diff/11003/53013
File chrome/browser/chromeos/login/user_image_view.h (right):

http://codereview.chromium.org/3442011/diff/11003/53013#newcode65
chrome/browser/chromeos/login/user_image_view.h:65: 
remove empty line

http://codereview.chromium.org/3442011/diff/11003/53017
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3442011/diff/11003/53017#newcode27
chrome/browser/chromeos/login/wizard_accessibility_helper.h:27: class
ViewDetails {
[Only if you want to keep this class]

looks like this is internal to Helper class. Can you use forward declaration and
move this to .cc?

http://codereview.chromium.org/3442011/diff/11003/53017#newcode34
chrome/browser/chromeos/login/wizard_accessibility_helper.h:34: ~ViewDetails()
{}
virtual (for style rather than correct-ness)

http://codereview.chromium.org/3442011/diff/11003/53017#newcode40
chrome/browser/chromeos/login/wizard_accessibility_helper.h:40: bool
view_enabled;
private: DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/3442011/diff/11003/53017#newcode75
chrome/browser/chromeos/login/wizard_accessibility_helper.h:75:
~WizardAccessibilityHelper();
virtual

[Chaitanya, 2010-09-23 19:34:38]

http://codereview.chromium.org/3442011/diff/11003/53001
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53001#newcode116
chrome/browser/chromeos/login/existing_user_view.cc:116: void
ExistingUserView::ViewHierarchyChanged(bool is_add, View* parent,
On 2010/09/23 17:06:54, oshima wrote:
> move all arguments to the next single line.

Done.

http://codereview.chromium.org/3442011/diff/11003/53001#newcode120
chrome/browser/chromeos/login/existing_user_view.cc:120:
ProfileManager::GetDefaultProfile());
Not sending profile anymore. GetDefaultProfile is now done in the helper.

On 2010/09/23 17:06:54, oshima wrote:
> this get executed everytime a view is added to the hierachy this belongs. Is
> that what you want? (same for other ViewHierachyChanged)

http://codereview.chromium.org/3442011/diff/11003/53002
File chrome/browser/chromeos/login/existing_user_view.h (right):

http://codereview.chromium.org/3442011/diff/11003/53002#newcode9
chrome/browser/chromeos/login/existing_user_view.h:9: #include
"chrome/browser/chromeos/login/wizard_accessibility_helper.h"
On 2010/09/23 17:06:54, oshima wrote:
> it's probably better to move this and constructor to .cc to reduce dependency
on
> header.

Done.

http://codereview.chromium.org/3442011/diff/11003/53002#newcode41
chrome/browser/chromeos/login/existing_user_view.h:41: virtual void
ViewHierarchyChanged(bool is_add, View* parent, View* child);
On 2010/09/23 17:06:54, oshima wrote:
> don't you have to say views::View?

Done.

http://codereview.chromium.org/3442011/diff/11003/53002#newcode51
chrome/browser/chromeos/login/existing_user_view.h:51:
DISALLOW_COPY_AND_ASSIGN(ExistingUserView);
On 2010/09/23 17:06:54, oshima wrote:
> move this to private: (or if there is a reason, write a comment)

Done.

http://codereview.chromium.org/3442011/diff/11003/53003
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53003#newcode345
chrome/browser/chromeos/login/network_selection_view.cc:345: void
NetworkSelectionView::ViewHierarchyChanged(bool is_add, View* parent,
On 2010/09/23 17:06:54, oshima wrote:
> same here

Done.

http://codereview.chromium.org/3442011/diff/11003/53004
File chrome/browser/chromeos/login/network_selection_view.h (right):

http://codereview.chromium.org/3442011/diff/11003/53004#newcode74
chrome/browser/chromeos/login/network_selection_view.h:74: 
On 2010/09/23 17:06:54, oshima wrote:
> remove extra file

Done.

http://codereview.chromium.org/3442011/diff/11003/53007
File chrome/browser/chromeos/login/password_changed_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53007#newcode69
chrome/browser/chromeos/login/password_changed_view.cc:69:
ProfileManager::GetDefaultProfile());
See comment for exising_user_view.cc

On 2010/09/23 17:06:54, oshima wrote:
> this will be called multiple times. Is it safe?
> Can this be inside (is_add && child==this)?
>

http://codereview.chromium.org/3442011/diff/11003/53013
File chrome/browser/chromeos/login/user_image_view.h (right):

http://codereview.chromium.org/3442011/diff/11003/53013#newcode65
chrome/browser/chromeos/login/user_image_view.h:65: 
On 2010/09/23 17:06:54, oshima wrote:
> remove empty line

Done.

http://codereview.chromium.org/3442011/diff/11003/53017
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3442011/diff/11003/53017#newcode27
chrome/browser/chromeos/login/wizard_accessibility_helper.h:27: class
ViewDetails {
On 2010/09/23 17:06:54, oshima wrote:
> [Only if you want to keep this class]
> 
> looks like this is internal to Helper class. Can you use forward declaration
and
> move this to .cc?

Removed the class.

http://codereview.chromium.org/3442011/diff/11003/53017#newcode29
chrome/browser/chromeos/login/wizard_accessibility_helper.h:29:
ViewDetails(Profile* profile, bool enabled) :
On 2010/09/23 16:27:52, Dominic Mazzoni wrote:
> I don't think it's necessary to store the profile for each view.
> 
> In the context of the wizard, there's only going to be one profile. We just
need
> to save it once.
> 
> I think instead of ViewDetails and the map from View to ViewDetails you could
> just keep a single set<View*> of views that have been created but haven't yet
> been made accessible.
> 

Done.

http://codereview.chromium.org/3442011/diff/11003/53017#newcode34
chrome/browser/chromeos/login/wizard_accessibility_helper.h:34: ~ViewDetails()
{}
On 2010/09/23 17:06:54, oshima wrote:
> virtual (for style rather than correct-ness)

class removed.

http://codereview.chromium.org/3442011/diff/11003/53017#newcode40
chrome/browser/chromeos/login/wizard_accessibility_helper.h:40: bool
view_enabled;
On 2010/09/23 17:06:54, oshima wrote:
> private: DISALLOW_COPY_AND_ASSIGN

class removed.

[Chaitanya, 2010-09-23 23:37:51]

Fixed.

On 2010/09/23 16:27:51, Dominic Mazzoni wrote:
> http://codereview.chromium.org/3442011/diff/11003/53017
> File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):
> 
> http://codereview.chromium.org/3442011/diff/11003/53017#newcode29
> chrome/browser/chromeos/login/wizard_accessibility_helper.h:29:
> ViewDetails(Profile* profile, bool enabled) :
> I don't think it's necessary to store the profile for each view.
> 
> In the context of the wizard, there's only going to be one profile. We just
need
> to save it once.
> 
> I think instead of ViewDetails and the map from View to ViewDetails you could
> just keep a single set<View*> of views that have been created but haven't yet
> been made accessible.

[Nikita (slow), 2010-09-24 16:26:01]

Do we have an issue that's this CL belongs to?

[oshima, 2010-09-24 16:56:52]

http://codereview.chromium.org/3442011/diff/11003/53001
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/3442011/diff/11003/53001#newcode120
chrome/browser/chromeos/login/existing_user_view.cc:120:
ProfileManager::GetDefaultProfile());
You probably misread my comment? This gets called everytime something is added
to view, so MaybeEnableAccessibility may be called with "this" multiple times.
Or is it enough to add when child == this?

On 2010/09/23 19:34:38, Chaitanya wrote:
> Not sending profile anymore. GetDefaultProfile is now done in the helper.
> 
> On 2010/09/23 17:06:54, oshima wrote:
> > this get executed everytime a view is added to the hierachy this belongs. Is
> > that what you want? (same for other ViewHierachyChanged)
> 
>

http://codereview.chromium.org/3442011/diff/59002/61017
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3442011/diff/59002/61017#newcode22
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:22: app::VKEY_Z,
false, true, true);
no static/global object. can you change this to singleton or 
static pointer?

http://codereview.chromium.org/3442011/diff/59002/61017#newcode96
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:96: if
(view_tree->GetWidget() == NULL) {
I believe chromium prefers ! over == NULL, and no {} for single line if

http://codereview.chromium.org/3442011/diff/59002/61017#newcode109
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:109: if
(!view_exists) {
same here for {}

[Chaitanya, 2010-09-27 19:52:48]

On 2010/09/24 16:56:52, oshima wrote:
> http://codereview.chromium.org/3442011/diff/11003/53001
> File chrome/browser/chromeos/login/existing_user_view.cc (right):
> 
> http://codereview.chromium.org/3442011/diff/11003/53001#newcode120
> chrome/browser/chromeos/login/existing_user_view.cc:120:
> ProfileManager::GetDefaultProfile());
> You probably misread my comment? This gets called everytime something is added
> to view, so MaybeEnableAccessibility may be called with "this" multiple times.
> Or is it enough to add when child == this?

Done.

> 
> On 2010/09/23 19:34:38, Chaitanya wrote:
> > Not sending profile anymore. GetDefaultProfile is now done in the helper.
> > 
> > On 2010/09/23 17:06:54, oshima wrote:
> > > this get executed everytime a view is added to the hierachy this belongs.
Is
> > > that what you want? (same for other ViewHierachyChanged)
> > 
> >
> 
> http://codereview.chromium.org/3442011/diff/59002/61017
> File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):
> 
> http://codereview.chromium.org/3442011/diff/59002/61017#newcode22
> chrome/browser/chromeos/login/wizard_accessibility_helper.cc:22: app::VKEY_Z,
> false, true, true);
> no static/global object. can you change this to singleton or 
> static pointer?

Changed to private static scoped_ptr.

> 
> http://codereview.chromium.org/3442011/diff/59002/61017#newcode96
> chrome/browser/chromeos/login/wizard_accessibility_helper.cc:96: if
> (view_tree->GetWidget() == NULL) {
> I believe chromium prefers ! over == NULL, and no {} for single line if

Done.

> 
> http://codereview.chromium.org/3442011/diff/59002/61017#newcode109
> chrome/browser/chromeos/login/wizard_accessibility_helper.cc:109: if
> (!view_exists) {
> same here for {}

Done.

[oshima, 2010-09-27 21:17:27]

lgtm

[dmazzoni, 2010-09-27 21:18:56]

LGTM too.

[Nikita (slow), 2010-09-29 14:10:22]

http://codereview.chromium.org/3442011/diff/67002/52005
File chrome/browser/chromeos/login/existing_user_view.cc (right):

http://codereview.chromium.org/3442011/diff/67002/52005#newcode113
chrome/browser/chromeos/login/existing_user_view.cc:113:
AddAccelerator(accel_enable_accessibility_);
I'm moving this call to ctor.
RecreateFields is called on each language change that registers accelerator
mupltiple time.

Any reason why AddAccelerator shouldn't be added in ctor?

[Nico, 2011-11-21 04:13:47]

http://codereview.chromium.org/3442011/diff/67002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3442011/diff/67002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:27: return
*(WizardAccessibilityHelper::accelerator_.get());
Why not just

  return views::Accelerator(app:VKEY_Z, false, true, true);

? I'm about to make that change; shout if that's wrong for some reason.