Add code to initialize accessibility for UI in the ChromeOS startup wizard.

Add code to initialize accessibility for UI in the ChromeOS startup wizard.

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

[dmazzoni, 2010-08-19 18:28:29]

http://codereview.chromium.org/3159028/diff/14001/15002
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3159028/diff/14001/15002#newcode33
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:33: case
NotificationType::ACCESSIBILITY_CONTROL_FOCUSED:
Are you going to have this call Speak(info->name()) for now as a placeholder
implementation that actually does something?

http://codereview.chromium.org/3159028/diff/14001/15003
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3159028/diff/14001/15003#newcode20
chrome/browser/chromeos/login/wizard_accessibility_helper.h:20: class
WizardAccessibilityHandler : public NotificationObserver {
Move this to a separate source file, wizard_accessibility_helper.h.
WizardAccessibilityHandler will end up with a lot of code to handle the details
of speaking, so it'd be easier to keep it separate.

[oshima, 2010-08-19 20:53:25]

http://codereview.chromium.org/3159028/diff/14001/15002
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3159028/diff/14001/15002#newcode22
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:22: #include
<fstream>
move C++ system headers before our headers.

http://codereview.chromium.org/3159028/diff/14001/15002#newcode43
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:43: default:
is this expected? If this shouldn't happen, use NOTREACHED, or may be write to
log.

http://codereview.chromium.org/3159028/diff/14001/15002#newcode84
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:84: if
(g_browser_process->local_state()->GetBoolean(
if (g_browser_process != NULL &&
    g_browser_process->local_state()->GetBoolean()) {
}

http://codereview.chromium.org/3159028/diff/14001/15002#newcode98
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:98:
prefService->SavePersistentPrefs();
i guess you should use ScheduleSavePresistentPrefs?

http://codereview.chromium.org/3159028/diff/14001/15003
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3159028/diff/14001/15003#newcode14
chrome/browser/chromeos/login/wizard_accessibility_helper.h:14: #include
"views/view.h"
is it possible to use forward decl instead of including viwe.h?

[Chaitanya, 2010-08-19 22:34:40]

http://codereview.chromium.org/3159028/diff/14001/15002
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3159028/diff/14001/15002#newcode22
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:22: #include
<fstream>
Removed these includes.

On 2010/08/19 20:53:25, oshima wrote:
> move C++ system headers before our headers.

http://codereview.chromium.org/3159028/diff/14001/15002#newcode33
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:33: case
NotificationType::ACCESSIBILITY_CONTROL_FOCUSED:
On 2010/08/19 18:28:29, Dominic Mazzoni wrote:
> Are you going to have this call Speak(info->name()) for now as a placeholder
> implementation that actually does something?
> 

Done.

http://codereview.chromium.org/3159028/diff/14001/15002#newcode43
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:43: default:
On 2010/08/19 20:53:25, oshima wrote:
> is this expected? If this shouldn't happen, use NOTREACHED, or may be write to
> log.

Done.

http://codereview.chromium.org/3159028/diff/14001/15002#newcode84
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:84: if
(g_browser_process->local_state()->GetBoolean(
On 2010/08/19 20:53:25, oshima wrote:
> if (g_browser_process != NULL &&
>     g_browser_process->local_state()->GetBoolean()) {
> }

Done.

http://codereview.chromium.org/3159028/diff/14001/15002#newcode98
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:98:
prefService->SavePersistentPrefs();
On 2010/08/19 20:53:25, oshima wrote:
> i guess you should use ScheduleSavePresistentPrefs?

Done.

[oshima, 2010-08-19 22:45:08]

http://codereview.chromium.org/3159028/diff/39001/40005
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3159028/diff/39001/40005#newcode15
chrome/browser/chromeos/login/wizard_accessibility_helper.h:15: #include
"views/view.h"
can you use forward declaration instead?

[Chaitanya, 2010-08-20 17:45:43]

http://codereview.chromium.org/3159028/diff/14001/15003
File chrome/browser/chromeos/login/wizard_accessibility_helper.h (right):

http://codereview.chromium.org/3159028/diff/14001/15003#newcode14
chrome/browser/chromeos/login/wizard_accessibility_helper.h:14: #include
"views/view.h"
On 2010/08/19 20:53:25, oshima wrote:
> is it possible to use forward decl instead of including viwe.h?

Done.

http://codereview.chromium.org/3159028/diff/14001/15003#newcode20
chrome/browser/chromeos/login/wizard_accessibility_helper.h:20: class
WizardAccessibilityHandler : public NotificationObserver {
On 2010/08/19 18:28:29, Dominic Mazzoni wrote:
> Move this to a separate source file, wizard_accessibility_helper.h.
> WizardAccessibilityHandler will end up with a lot of code to handle the
details
> of speaking, so it'd be easier to keep it separate.
> 

Done.

[dmazzoni, 2010-08-20 17:51:59]

LGTM.

[oshima, 2010-08-20 17:57:25]

http://codereview.chromium.org/3159028/diff/45001/46003
File chrome/browser/chromeos/login/wizard_accessibility_handler.h (right):

http://codereview.chromium.org/3159028/diff/45001/46003#newcode10
chrome/browser/chromeos/login/wizard_accessibility_handler.h:10: #include
"chrome/common/notification_registrar.h"
Do you need this?

http://codereview.chromium.org/3159028/diff/45001/46004
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3159028/diff/45001/46004#newcode16
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:16: #include
"views/view.h"
I'm guessing this compile without view.h.

Can you do me a favor? following accessibility headers seems to have unnecessary
includes.

a) chrome/browser/views/accessible_view_helper.h includes
accessibility_event_router_views.h, which seems to be unnecessary.
b) accessibilty_event_router_views.h includes "views.h" and "root_view.h", which
probably not necessary and can move to .cc

don't have to be in this CL, but I'd like to fix it while we're at it.

[Chaitanya, 2010-08-20 20:16:24]

http://codereview.chromium.org/3159028/diff/45001/46003
File chrome/browser/chromeos/login/wizard_accessibility_handler.h (right):

http://codereview.chromium.org/3159028/diff/45001/46003#newcode10
chrome/browser/chromeos/login/wizard_accessibility_handler.h:10: #include
"chrome/common/notification_registrar.h"
On 2010/08/20 17:57:25, oshima wrote:
> Do you need this?

Done.

http://codereview.chromium.org/3159028/diff/45001/46004
File chrome/browser/chromeos/login/wizard_accessibility_helper.cc (right):

http://codereview.chromium.org/3159028/diff/45001/46004#newcode16
chrome/browser/chromeos/login/wizard_accessibility_helper.cc:16: #include
"views/view.h"
Fixed this.
Will make a separate CL for other fixes...
On 2010/08/20 17:57:25, oshima wrote:
> I'm guessing this compile without view.h.
> 
> Can you do me a favor? following accessibility headers seems to have
unnecessary
> includes.
> 
> a) chrome/browser/views/accessible_view_helper.h includes
> accessibility_event_router_views.h, which seems to be unnecessary.
> b) accessibilty_event_router_views.h includes "views.h" and "root_view.h",
which
> probably not necessary and can move to .cc
> 
> don't have to be in this CL, but I'd like to fix it while we're at it.

[oshima, 2010-08-20 20:59:24]

lgtm. make sure try jobs pass.

[Chaitanya, 2010-08-20 22:50:58]

On 2010/08/20 20:59:24, oshima wrote:
> lgtm. make sure try jobs pass.

I was registering the accessibility pref twice. It gets registered in
wizard_controller first. Removed the registration in preferences.cc.

[Chaitanya, 2010-08-25 02:36:50]

I believe the build failure on Win, and the flaky tests on linux_chromeos are
not related to this change. Can someone please confirm?

On 2010/08/20 22:50:58, Chaitanya wrote:
> On 2010/08/20 20:59:24, oshima wrote:
> > lgtm. make sure try jobs pass.
> 
> I was registering the accessibility pref twice. It gets registered in
> wizard_controller first. Removed the registration in preferences.cc.

[dmazzoni, 2010-08-25 02:38:32]

How many times have you run gcl try? If you think it's flakiness, run it 2
or 3 times to confirm.

- Dominic

On Tue, Aug 24, 2010 at 7:36 PM, <chaitanyag@chromium.org> wrote:

> I believe the build failure on Win, and the flaky tests on linux_chromeos
> are
> not related to this change. Can someone please confirm?
>
>
> On 2010/08/20 22:50:58, Chaitanya wrote:
>
>> On 2010/08/20 20:59:24, oshima wrote:
>> > lgtm. make sure try jobs pass.
>>
>
>  I was registering the accessibility pref twice. It gets registered in
>> wizard_controller first. Removed the registration in preferences.cc.
>>
>
>
>
> http://codereview.chromium.org/3159028/show
>

[sadrul-g, 2010-09-15 23:41:37]

> I was registering the accessibility pref twice. It gets registered in
> wizard_controller first. Removed the registration in preferences.cc.

This change seems to cause a crash on linux (with chromeos=1) when trying to get
to the Preferences dialog.

[Chaitanya, 2010-09-16 00:33:36]

I am taking a look at it now.

On Wed, Sep 15, 2010 at 4:41 PM, <sadrul@google.com> wrote:

> I was registering the accessibility pref twice. It gets registered in
>> wizard_controller first. Removed the registration in preferences.cc.
>>
>
> This change seems to cause a crash on linux (with chromeos=1) when trying
> to get
> to the Preferences dialog.
>
> http://codereview.chromium.org/3159028/show
>

[Charlie Lee, 2010-09-16 21:07:24]

This is crashing because this preference never gets registered if we just run
Chrome for ChromeOS by itself on linux. What was the reason for moving the
registration for this preference from preferences.cc to wizard_controller?

On 2010/09/16 00:33:36, Chaitanya wrote:
> I am taking a look at it now.
> 
> On Wed, Sep 15, 2010 at 4:41 PM, <mailto:sadrul@google.com> wrote:
> 
> > I was registering the accessibility pref twice. It gets registered in
> >> wizard_controller first. Removed the registration in preferences.cc.
> >>
> >
> > This change seems to cause a crash on linux (with chromeos=1) when trying
> > to get
> > to the Preferences dialog.
> >
> > http://codereview.chromium.org/3159028/show
> >
>

[Chaitanya, 2010-09-16 21:35:36]

Issue 3402009 fixes this. I will submit as soon as I get an LGTM
thanks!

On Thu, Sep 16, 2010 at 2:07 PM, <chocobo@chromium.org> wrote:

> This is crashing because this preference never gets registered if we just
> run
> Chrome for ChromeOS by itself on linux. What was the reason for moving the
> registration for this preference from preferences.cc to wizard_controller?
>
>
> On 2010/09/16 00:33:36, Chaitanya wrote:
>
>> I am taking a look at it now.
>>
>
>  On Wed, Sep 15, 2010 at 4:41 PM, <mailto:sadrul@google.com> wrote:
>>
>
>  > I was registering the accessibility pref twice. It gets registered in
>> >> wizard_controller first. Removed the registration in preferences.cc.
>> >>
>> >
>> > This change seems to cause a crash on linux (with chromeos=1) when
>> trying
>> > to get
>> > to the Preferences dialog.
>> >
>> > http://codereview.chromium.org/3159028/show
>> >
>>
>
>
>
>
> http://codereview.chromium.org/3159028/show
>