Re-introduce the partial magnifier

Re-introduce the partial magnifier

Major Changes:
- Adding a selectbox at the right of screen magnifier setting on the setting page.
- Adding 'screen_magnifier_type2' pref.
- Separating the enable/disable state of magnifier from MagnifierType. MagnifierType does no longer indicate the current enable/disable status.
- Add IsMagnifierEnabled() to shell delegate.

BUG=166832
TEST=confirm that magnifier can be enabled/disabled via the tray and the settings page. browser_test passes.

R=zork@chromium.org, derat@chromium.org, nkostylev@chromium.org
TBR=jhawkins@chromium.org
# TBRing for small changed in C/B/ui/webui/options/ and C/B/resources/options/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176087

[yoshiki, 2012-12-20 02:49:36]

@zork: could you take a look? thanks.

[Zachary Kuznia, 2012-12-20 05:54:21]

https://codereview.chromium.org/11642014/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11642014/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.h:37: virtual void
SetMagnifier(bool enabled, ash::MagnifierType type) = 0;
Please separate this into SetMagnifierEnabled and SetMagnifierType

[yoshiki, 2012-12-21 07:32:08]

@zork: PTAL

https://codereview.chromium.org/11642014/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11642014/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.h:37: virtual void
SetMagnifier(bool enabled, ash::MagnifierType type) = 0;
On 2012/12/20 05:54:21, Zachary Kuznia wrote:
> Please separate this into SetMagnifierEnabled and SetMagnifierType

Done.

[Zachary Kuznia, 2012-12-26 01:09:55]

https://codereview.chromium.org/11642014/diff/13004/ash/magnifier/magnifier_c...
File ash/magnifier/magnifier_constants.h (right):

https://codereview.chromium.org/11642014/diff/13004/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:13: MAGNIFIER_DEFAULT        =
MAGNIFIER_FULL,
Separate this out of the enum, and make it a separate const.

https://codereview.chromium.org/11642014/diff/13004/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11642014/diff/13004/chrome/common/pref_names....
chrome/common/pref_names.cc:737: // A string pref which determines what type of
scre en magnifier is enabled.
"An integer pref"
s/scre en/screen/

[yoshiki, 2013-01-07 05:38:13]

zork: PTAL, Thanks,

https://codereview.chromium.org/11642014/diff/13004/ash/magnifier/magnifier_c...
File ash/magnifier/magnifier_constants.h (right):

https://codereview.chromium.org/11642014/diff/13004/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:13: MAGNIFIER_DEFAULT        =
MAGNIFIER_FULL,
On 2012/12/26 01:09:56, Zachary Kuznia wrote:
> Separate this out of the enum, and make it a separate const.

Done.

https://codereview.chromium.org/11642014/diff/13004/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/11642014/diff/13004/chrome/common/pref_names....
chrome/common/pref_names.cc:737: // A string pref which determines what type of
scre en magnifier is enabled.
On 2012/12/26 01:09:56, Zachary Kuznia wrote:
> "An integer pref"
> s/scre en/screen/

Done.

[Zachary Kuznia, 2013-01-07 06:06:38]

lgtm

[yoshiki, 2013-01-08 00:02:50]

+derat, nkostylev, jhawkins
Could you review following files? Thanks,

@derat
- ash/
- chrome/browser/chromeos/
- chrome/browser/ui/ash/

@nkostylev
- chrome/browser/ui/webui/chromeos/login/

@jhawkins:
- chrome/browser/ui/webui/options/
- chrome/browser/resources/options/

[Daniel Erat, 2013-01-08 00:28:49]

Mostly just nits from me.

https://codereview.chromium.org/11642014/diff/26001/ash/magnifier/magnifier_c...
File ash/magnifier/magnifier_constants.h (right):

https://codereview.chromium.org/11642014/diff/26001/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:11: MAGNIFIER_FULL           = 1,  // This
should not be changed for UMA and pref.
nit: get rid of extra spaces before '=' characters

also, just move the comment to the top:

  // Note: Do not change these values; UMA and prefs depend on them.
  enum MagnifierType {

https://codereview.chromium.org/11642014/diff/26001/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:15: const MagnifierType
DEFAULT_MAGNIFIER_TYPE = MAGNIFIER_FULL;
nit: use kDefaultMagnifierType instead

https://codereview.chromium.org/11642014/diff/26001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/11642014/diff/26001/ash/shell_delegate.h#newc...
ash/shell_delegate.h:155: // Invoked to enable the screen magnifier.
nit: fix this comment (it's the same as SetMagnifierEnabled()'s)

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/accessibility_util.cc (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_util.cc:126:
UMA_HISTOGRAM_ENUMERATION("Accessibility.CrosScreenMagnifier", type, 3);
is there any chance that more values will be added in the future?  if so, do you
need to reserve more space here?  can you double-check with mark pearson?

also, i'd recommend adding some kNumMagnifierTypes = 3 enum value in
magnifier_constants.h and using that here instead of hardcoding a constant. 
(i'm not sure whether kNumMagnifierTypes or NUM_MAGNIFIER_TYPES is more common
here; you might want to try to see what's done elsewhere.)

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:64: if (enabled_
== enabled && !enabled)
i'm a bit surprised by this -- you need to do the rest of the work if enabled_
stayed true?  if this is correct, please add a comment explaining why it's
needed.

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:72: if (enabled
!= prefs->GetBoolean(prefs::kScreenMagnifierEnabled)) {
shouldn't we already match the pref?  why is this check needed?

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:99: if (type !=
prefs->GetInteger(prefs::kScreenMagnifierType)) {
ditto here

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:184: void
UpdateMagnifierStatus() {
nit: maybe rename this to something like UpdateMagnifierStatusFromPref()?

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.h:33: virtual bool
IsMagnifierEnabled() const = 0;
nit: please add blank lines between these method names; it can be hard to read
otherwise since they all have comments

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/preferences.cc:100: // the pref here in case of
Chrome/Linux with ChromeOS=1.
nit (can happen as part of a separate change): is this comment still true?  does
it apply to all of these prefs, or just one of them?  just want to make sure
that a bunch of extra lines aren't being added here for no good reason...

[yoshiki, 2013-01-09 03:03:15]

@derat: PTAL, Thanks,

https://codereview.chromium.org/11642014/diff/26001/ash/magnifier/magnifier_c...
File ash/magnifier/magnifier_constants.h (right):

https://codereview.chromium.org/11642014/diff/26001/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:11: MAGNIFIER_FULL           = 1,  // This
should not be changed for UMA and pref.
On 2013/01/08 00:28:49, Daniel Erat wrote:
> nit: get rid of extra spaces before '=' characters
> 
> also, just move the comment to the top:
> 
>   // Note: Do not change these values; UMA and prefs depend on them.
>   enum MagnifierType {

Done.

https://codereview.chromium.org/11642014/diff/26001/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:15: const MagnifierType
DEFAULT_MAGNIFIER_TYPE = MAGNIFIER_FULL;
On 2013/01/08 00:28:49, Daniel Erat wrote:
> nit: use kDefaultMagnifierType instead

Done.

https://codereview.chromium.org/11642014/diff/26001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/11642014/diff/26001/ash/shell_delegate.h#newc...
ash/shell_delegate.h:155: // Invoked to enable the screen magnifier.
On 2013/01/08 00:28:49, Daniel Erat wrote:
> nit: fix this comment (it's the same as SetMagnifierEnabled()'s)

Done.

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/accessibility_util.cc (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_util.cc:126:
UMA_HISTOGRAM_ENUMERATION("Accessibility.CrosScreenMagnifier", type, 3);
This value is same as the previous code, which is wrote by dmazzoni@. I think
this is ok to keep as it is.

And changed '3' with |kNumMagnifierTypes| defined in magnifier_constants.h.

On 2013/01/08 00:28:49, Daniel Erat wrote:
> is there any chance that more values will be added in the future?  if so, do
you
> need to reserve more space here?  can you double-check with mark pearson?
> 
> also, i'd recommend adding some kNumMagnifierTypes = 3 enum value in
> magnifier_constants.h and using that here instead of hardcoding a constant. 
> (i'm not sure whether kNumMagnifierTypes or NUM_MAGNIFIER_TYPES is more common
> here; you might want to try to see what's done elsewhere.)

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:64: if (enabled_
== enabled && !enabled)
We need the rest of the work. Added a comment.

On 2013/01/08 00:28:49, Daniel Erat wrote:
> i'm a bit surprised by this -- you need to do the rest of the work if enabled_
> stayed true?  if this is correct, please add a comment explaining why it's
> needed.

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:72: if (enabled
!= prefs->GetBoolean(prefs::kScreenMagnifierEnabled)) {
We need to keep it for the reason in the above comment.

On 2013/01/08 00:28:49, Daniel Erat wrote:
> shouldn't we already match the pref?  why is this check needed?

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:99: if (type !=
prefs->GetInteger(prefs::kScreenMagnifierType)) {
On 2013/01/08 00:28:49, Daniel Erat wrote:
> ditto here

Done. This can be removed.

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:184: void
UpdateMagnifierStatus() {
On 2013/01/08 00:28:49, Daniel Erat wrote:
> nit: maybe rename this to something like UpdateMagnifierStatusFromPref()?

Done.

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.h:33: virtual bool
IsMagnifierEnabled() const = 0;
On 2013/01/08 00:28:49, Daniel Erat wrote:
> nit: please add blank lines between these method names; it can be hard to read
> otherwise since they all have comments

Done.

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/11642014/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/preferences.cc:100: // the pref here in case of
Chrome/Linux with ChromeOS=1.
Good point. I think some of if-clauses can be removed here. I'll do this work at
an another CL.

On 2013/01/08 00:28:49, Daniel Erat wrote:
> nit (can happen as part of a separate change): is this comment still true? 
does
> it apply to all of these prefs, or just one of them?  just want to make sure
> that a bunch of extra lines aren't being added here for no good reason...

[Nikita (slow), 2013-01-09 14:02:14]

chrome/browser/ui/webui/chromeos/login/ lgtm

[Daniel Erat, 2013-01-09 15:43:55]

lgtm

https://chromiumcodereview.appspot.com/11642014/diff/22006/ash/magnifier/magn...
File ash/magnifier/magnifier_constants.h (right):

https://chromiumcodereview.appspot.com/11642014/diff/22006/ash/magnifier/magn...
ash/magnifier/magnifier_constants.h:16: // This value must be the maximum number
of MagnifierType) + 1
s/)//

this is a bit misleading, though (it's 3, but there are only 2 types).  maybe
you could rename this to kMaxMagnifierType, set it to 2, and then just do
kMaxMagnifierType + 1 when recording the metric?

https://chromiumcodereview.appspot.com/11642014/diff/22006/chrome/browser/chr...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://chromiumcodereview.appspot.com/11642014/diff/22006/chrome/browser/chr...
chrome/browser/chromeos/accessibility/magnification_manager.cc:64: // This
method may be invoked even when the other magnifier setting (e.g.
nit: s/setting/settings/

https://chromiumcodereview.appspot.com/11642014/diff/22006/chrome/browser/chr...
chrome/browser/chromeos/accessibility/magnification_manager.cc:65: // type or
scale) is changed, so we need to call magnification controller
nit: s/is/are/

https://chromiumcodereview.appspot.com/11642014/diff/22006/chrome/browser/chr...
chrome/browser/chromeos/accessibility/magnification_manager.cc:66: // even
|enabled| is unchanged. Only if |enabled| is false and the magnifier
nit: s/even/even if/

i'd also delete the last sentence; it's unnecessary

[yoshiki, 2013-01-10 06:25:39]

Thanks for reviews!

https://codereview.chromium.org/11642014/diff/22006/ash/magnifier/magnifier_c...
File ash/magnifier/magnifier_constants.h (right):

https://codereview.chromium.org/11642014/diff/22006/ash/magnifier/magnifier_c...
ash/magnifier/magnifier_constants.h:16: // This value must be the maximum number
of MagnifierType) + 1
On 2013/01/09 15:43:56, Daniel Erat wrote:
> s/)//
> 
> this is a bit misleading, though (it's 3, but there are only 2 types).  maybe
> you could rename this to kMaxMagnifierType, set it to 2, and then just do
> kMaxMagnifierType + 1 when recording the metric?

Done.

https://codereview.chromium.org/11642014/diff/22006/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/11642014/diff/22006/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:64: // This
method may be invoked even when the other magnifier setting (e.g.
On 2013/01/09 15:43:56, Daniel Erat wrote:
> nit: s/setting/settings/

Done.

https://codereview.chromium.org/11642014/diff/22006/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:65: // type or
scale) is changed, so we need to call magnification controller
On 2013/01/09 15:43:56, Daniel Erat wrote:
> nit: s/is/are/

Done.

https://codereview.chromium.org/11642014/diff/22006/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.cc:66: // even
|enabled| is unchanged. Only if |enabled| is false and the magnifier
On 2013/01/09 15:43:56, Daniel Erat wrote:
> nit: s/even/even if/
> 
> i'd also delete the last sentence; it's unnecessary

Done.

[commit-bot: I haz the power, 2013-01-10 06:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11642014/25030

[commit-bot: I haz the power, 2013-01-10 07:30:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_aura&n...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-10 08:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11642014/25031

[commit-bot: I haz the power, 2013-01-10 11:25:17]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-10 12:37:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11642014/25031

[commit-bot: I haz the power, 2013-01-10 15:08:13]

Change committed as 176087

[Dan Beam, 2013-01-11 00:34:22]

there are other reviewers for these directories

https://chromiumcodereview.appspot.com/11642014/diff/26001/chrome/browser/ui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://chromiumcodereview.appspot.com/11642014/diff/26001/chrome/browser/ui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:433: base::ListValue*
magnifierList = new base::ListValue();
nit: magnifier_list (C++ is var_name not varName)

https://chromiumcodereview.appspot.com/11642014/diff/26001/chrome/browser/ui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:435:
option_full->Append(base::Value::CreateIntegerValue(ash::MAGNIFIER_FULL));
nit: I think base::Value::Create* functions are deprecated / being removed.

https://chromiumcodereview.appspot.com/11642014/diff/26001/chrome/browser/ui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:443:
magnifierList->Append(option_full);
totally optional nit: I'd put some visual separation because each list creation
and consider using scoped_ptr<>s, i.e.:

  scoped_ptr<base::ListValue> magnifier_list(new base::ListValue);
  
  scoped_ptr<base::ListValue> option_full(new base::ListValue);
  option_full->Append(
      new base::Value::IntegerValue(ash::MAGNIFIER_FULL));
  magnifier_list->Append(option_full.release());
  
  scoped_ptr<base::ListValue> option_partial(new base::ListValue);
  option_partial->Append(
      new base::Value::IntegerValue(ash::MAGNIFIER_PARTIAL));
  option_partial->Append(new base::StringValue(l10n_util::GetStringUTF16(
      IDS_OPTIONS_SETTINGS_ACCESSIBILITY_SCREEN_MAGNIFIER_PARTIAL))
  magnifier_list->Append(option_partial);

  values->Set("magnifierList", magnifier_list.release());