Adding accessibility setting to System options

landing chaitanya's patch

Adding accessibility setting to System options

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

[Chaitanya, 2010-05-25 23:15:19]

On 2010/05/25 00:22:18, Chaitanya wrote:
> 

ping?

[Chaitanya, 2010-05-26 17:56:40]

On 2010/05/25 23:15:19, Chaitanya wrote:
> On 2010/05/25 00:22:18, Chaitanya wrote:
> > 
> 
> ping?

Nicholas has Ok'd the UI. Does this look good otherwise?

[oshima, 2010-05-26 18:05:08]

reviewing it now.

On Wed, May 26, 2010 at 10:56 AM, <chaitanyag@chromium.org> wrote:

> On 2010/05/25 23:15:19, Chaitanya wrote:
>
>> On 2010/05/25 00:22:18, Chaitanya wrote:
>> >
>>
>
>  ping?
>>
>
> Nicholas has Ok'd the UI. Does this look good otherwise?
>
>
> http://codereview.chromium.org/2127014/show
>

[oshima, 2010-05-26 18:12:19]

LGTM but please fix style/comment issue below.

http://codereview.chromium.org/2127014/diff/1/3
File chrome/browser/chromeos/options/system_page_view.cc (right):

http://codereview.chromium.org/2127014/diff/1/3#newcode419
chrome/browser/chromeos/options/system_page_view.cc:419: virtual void
NotifyPrefChanged(const std::wstring* pref_name);
Is this from views::View?

http://codereview.chromium.org/2127014/diff/1/3#newcode437
chrome/browser/chromeos/options/system_page_view.cc:437:
accessibility_checkbox_(NULL) {
fix indent

http://codereview.chromium.org/2127014/diff/1/3#newcode452
chrome/browser/chromeos/options/system_page_view.cc:452: profile()->GetPrefs(),
this);
indent

http://codereview.chromium.org/2127014/diff/1/3#newcode466
chrome/browser/chromeos/options/system_page_view.cc:466: bool enabled = 
accessibility_enabled_.GetValue();
extra space after =

http://codereview.chromium.org/2127014/diff/1/6
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2127014/diff/1/6#newcode258
chrome/common/pref_names.cc:258: const wchar_t kAccessibilityEnabled[] =
L"settings.accessibility";
please add comment for this parameter.

[Chaitanya, 2010-05-26 19:05:03]

Fixed all.

On Wed, May 26, 2010 at 11:12 AM, <oshima@chromium.org> wrote:

> LGTM but please fix style/comment issue below.
>
>
> http://codereview.chromium.org/2127014/diff/1/3
> File chrome/browser/chromeos/options/system_page_view.cc (right):
>
> http://codereview.chromium.org/2127014/diff/1/3#newcode419
> chrome/browser/chromeos/options/system_page_view.cc:419: virtual void
> NotifyPrefChanged(const std::wstring* pref_name);
> Is this from views::View?
>
> http://codereview.chromium.org/2127014/diff/1/3#newcode437
> chrome/browser/chromeos/options/system_page_view.cc:437:
> accessibility_checkbox_(NULL) {
> fix indent
>
> http://codereview.chromium.org/2127014/diff/1/3#newcode452
> chrome/browser/chromeos/options/system_page_view.cc:452:
> profile()->GetPrefs(), this);
> indent
>
> http://codereview.chromium.org/2127014/diff/1/3#newcode466
> chrome/browser/chromeos/options/system_page_view.cc:466: bool enabled =
> accessibility_enabled_.GetValue();
> extra space after =
>
> http://codereview.chromium.org/2127014/diff/1/6
> File chrome/common/pref_names.cc (right):
>
> http://codereview.chromium.org/2127014/diff/1/6#newcode258
> chrome/common/pref_names.cc:258: const wchar_t kAccessibilityEnabled[] =
> L"settings.accessibility";
> please add comment for this parameter.
>
>
> http://codereview.chromium.org/2127014/show
>

[oshima, 2010-05-27 21:16:33]

chaitanya, can you fix below and upload new patch?

http://codereview.chromium.org/2127014/diff/12001/13002
File chrome/browser/chromeos/options/system_page_view.cc (right):

http://codereview.chromium.org/2127014/diff/12001/13002#newcode420
chrome/browser/chromeos/options/system_page_view.cc:420: virtual void
NotifyPrefChanged(const std::wstring* pref_name);
can you move this to the same section above?

http://codereview.chromium.org/2127014/diff/12001/13002#newcode437
chrome/browser/chromeos/options/system_page_view.cc:437:
IDS_OPTIONS_SETTINGS_SECTION_TITLE_ACCESSIBILITY),
this is not right indent

http://codereview.chromium.org/2127014/diff/12001/13005
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2127014/diff/12001/13005#newcode258
chrome/common/pref_names.cc:258: // A boolean pref which determines whether
accessibility is enabled
"." at the end.

[Chaitanya, 2010-05-27 22:09:18]

http://codereview.chromium.org/2127014/diff/12001/13002
File chrome/browser/chromeos/options/system_page_view.cc (right):

http://codereview.chromium.org/2127014/diff/12001/13002#newcode420
chrome/browser/chromeos/options/system_page_view.cc:420: virtual void
NotifyPrefChanged(const std::wstring* pref_name);
On 2010/05/27 21:16:33, oshima wrote:
> can you move this to the same section above?

Done.

http://codereview.chromium.org/2127014/diff/12001/13002#newcode437
chrome/browser/chromeos/options/system_page_view.cc:437:
IDS_OPTIONS_SETTINGS_SECTION_TITLE_ACCESSIBILITY),
On 2010/05/27 21:16:33, oshima wrote:
> this is not right indent

Done.

http://codereview.chromium.org/2127014/diff/12001/13005
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/2127014/diff/12001/13005#newcode258
chrome/common/pref_names.cc:258: // A boolean pref which determines whether
accessibility is enabled
On 2010/05/27 21:16:33, oshima wrote:
> "." at the end.

Done.