Add the partial screen magnifier to Chrome OS.

Add the partial screen magnifier to Chrome OS.

BUG=127075
TEST=Set settings->Advanced Settings -> Accessibility -> Screen magnifier to partial.  Check that the area around the mouse cursor is magnified


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

[Zachary Kuznia, 2012-09-07 08:32:03]

Hello,
Could you review this at:
https://chromiumcodereview.appspot.com/10915140

sky: Could you have a look at the magnifier code?
jhawkins: Could you review the changes to the settings?

Thanks,
-Zach

[sky, 2012-09-07 16:06:58]

I added Antoine in case he wants to look at Layer changes, and Oshima as he was
asking about magnifiers the other day.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
File ash/magnifier/partial_magnification_controller.cc (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:110: aura::RootWindow*
root_window_;
What happens if root_window_ is destroyed?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:142:
Shell::GetInstance()->RemoveEnvEventFilter(this);
What happens if zoom_window_/zoom_widget_ are non-null?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:163: *scale =
kNonPartialMagnifiedScale;
How come you have Threshold and Scale? And the comments aren't really helpful.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:201: zoom_window_ = new
aura::Window(NULL);
Who deletes this?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:209: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
You never delete zoom_widget_, which means because you're setting this it leaks.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:214:
zoom_widget_->SetBounds(gfx::Rect(mouse.x() - width / 2,
You should make sure this does what you want when a maximized window is up.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
File ash/magnifier/partial_magnification_controller.h (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.h:25: class
PartialMagnificationController {
Description.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:304: zoom_x_offset_ = x_offset;
These values aren't used locally, only passed to webkit. Why do we need to cache
them in Layer too?

[piman, 2012-09-07 21:38:55]

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:312: (bounds().width() / zoom_),
is bounds() what you want, if the layer is animated? It feels like
GetTargetBounds() would be more appropriate.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:313: (bounds().height() / zoom_)), 16));
16 looks very magic. Should it be passed as a parameter to SetBackgroundZoom?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:315: web_layer_->setBackgroundFilters(filters);
This overrides the SetBackgroundBlur setting and vice versa. Can we make sure to
keep this orthogonal? E.g. by recreating the background filter list when either
of them changes.

[oshima, 2012-09-07 22:30:55]

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
File ash/magnifier/partial_magnification_controller.cc (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:92: // Returns if the
magnification scale is 1.0 or not (larger then 1.0).
s/then/than
but this description is not clear if it's = 1.0 or >1.0.
I guess it's >1.0, so perhaps, simply saying scale > 1.0 is better?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:96: void ValidateScale(float*
scale);
CorrectScale, EnsureScale ?

validate sounds like predicate or assertion

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:132: :
root_window_(ash::Shell::GetPrimaryRootWindow()),
Do you need to set this to primary first? Or can we always call
SwitchTargetRootWindow before used?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:152: }
nuke {} and use const for size.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:167: if (*scale >
kMaxPartialMagnifiedScaleThreshold)
else if

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:209: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
On 2012/09/07 16:06:59, sky wrote:
> You never delete zoom_widget_, which means because you're setting this it
leaks.

To add a bit more explanation, widget with WIDGET_OWNS_NATIVE_WIDGET won't be
deleted by Close and you're responsible for deleting it.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:278: if
(root_bounds.Contains(event->root_location())) {
I believe this root location is native X's, and not aura's root, so it probably
works on primary display, but not on 2nd display. you should convert location()
from target to root.
 We probably should rename it to native_root_location or something.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
File ash/magnifier/partial_magnification_controller.h (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.h:13: #include "ui/gfx/rect.h"
clean up unused headers

[Zachary Kuznia, 2012-09-13 11:49:01]

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
File ash/magnifier/partial_magnification_controller.cc (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:92: // Returns if the
magnification scale is 1.0 or not (larger then 1.0).
On 2012/09/07 22:30:55, oshima wrote:
> s/then/than
> but this description is not clear if it's = 1.0 or >1.0.
> I guess it's >1.0, so perhaps, simply saying scale > 1.0 is better?

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:96: void ValidateScale(float*
scale);
On 2012/09/07 22:30:55, oshima wrote:
> CorrectScale, EnsureScale ?
> 
> validate sounds like predicate or assertion

Removed.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:110: aura::RootWindow*
root_window_;
On 2012/09/07 16:06:59, sky wrote:
> What happens if root_window_ is destroyed?

Added self as an observer.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:132: :
root_window_(ash::Shell::GetPrimaryRootWindow()),
On 2012/09/07 22:30:55, oshima wrote:
> Do you need to set this to primary first? Or can we always call
> SwitchTargetRootWindow before used?

Deferred to using GetActiveRootWindow() when enabled.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:142:
Shell::GetInstance()->RemoveEnvEventFilter(this);
On 2012/09/07 16:06:59, sky wrote:
> What happens if zoom_window_/zoom_widget_ are non-null?

Fixed.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:152: }
On 2012/09/07 22:30:55, oshima wrote:
> nuke {} and use const for size.
> 

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:163: *scale =
kNonPartialMagnifiedScale;
On 2012/09/07 16:06:59, sky wrote:
> How come you have Threshold and Scale? And the comments aren't really helpful.

Removed this function for now, since magnification isn't dynamic.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:167: if (*scale >
kMaxPartialMagnifiedScaleThreshold)
On 2012/09/07 22:30:55, oshima wrote:
> else if

Removed this function for now, since magnification isn't dynamic.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:201: zoom_window_ = new
aura::Window(NULL);
On 2012/09/07 16:06:59, sky wrote:
> Who deletes this?

Added the delete after close.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:209: params.ownership =
views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
On 2012/09/07 22:30:55, oshima wrote:
> On 2012/09/07 16:06:59, sky wrote:
> > You never delete zoom_widget_, which means because you're setting this it
> leaks.
> 
> To add a bit more explanation, widget with WIDGET_OWNS_NATIVE_WIDGET won't be
> deleted by Close and you're responsible for deleting it. 
> 

Fixed.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:214:
zoom_widget_->SetBounds(gfx::Rect(mouse.x() - width / 2,
On 2012/09/07 16:06:59, sky wrote:
> You should make sure this does what you want when a maximized window is up.

It seems to.  What strange behavior might happen?

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.cc:278: if
(root_bounds.Contains(event->root_location())) {
On 2012/09/07 22:30:55, oshima wrote:
> I believe this root location is native X's, and not aura's root, so it
probably
> works on primary display, but not on 2nd display. you should convert
location()
> from target to root.
>  We probably should rename it to native_root_location or something.

This seems to be aura's root location.  When I convert it, it no longer matches
the position of the mouse cursor.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
File ash/magnifier/partial_magnification_controller.h (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.h:13: #include "ui/gfx/rect.h"
On 2012/09/07 22:30:55, oshima wrote:
> clean up unused headers

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ash/magnifier/parti...
ash/magnifier/partial_magnification_controller.h:25: class
PartialMagnificationController {
On 2012/09/07 16:06:59, sky wrote:
> Description.

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:304: zoom_x_offset_ = x_offset;
On 2012/09/07 16:06:59, sky wrote:
> These values aren't used locally, only passed to webkit. Why do we need to
cache
> them in Layer too?

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:312: (bounds().width() / zoom_),
On 2012/09/07 21:38:56, piman wrote:
> is bounds() what you want, if the layer is animated? It feels like
> GetTargetBounds() would be more appropriate.

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:313: (bounds().height() / zoom_)), 16));
On 2012/09/07 21:38:56, piman wrote:
> 16 looks very magic. Should it be passed as a parameter to SetBackgroundZoom?

Done.

https://chromiumcodereview.appspot.com/10915140/diff/4002/ui/compositor/layer...
ui/compositor/layer.cc:315: web_layer_->setBackgroundFilters(filters);
On 2012/09/07 21:38:56, piman wrote:
> This overrides the SetBackgroundBlur setting and vice versa. Can we make sure
to
> keep this orthogonal? E.g. by recreating the background filter list when
either
> of them changes.

Done.

[oshima, 2012-09-13 16:35:31]

is it possible to add test case for this?

http://codereview.chromium.org/10915140/diff/9001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/10915140/diff/9001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:253: int new_scale_index =
std::max(0, std::min(8, scale_index + delta_index));
I probably would create a small function to get scale and write unit tests.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/magnification...
ash/magnifier/magnification_controller.h:33: virtual bool IsEnabled() = 0;
can this be const?

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:25: const int kMagnifierHeight
= 200;
doument

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:34: explicit
ZoomDelegateView();
remove explicit

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:160: int y = location.y();
or you can use gfx::Point::Offset. up to you.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:217:
SetRootWindow(ash::Shell::GetActiveRootWindow());
Don't you want to use root window where the mouse pointer is currently on?

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:223:
root_window_->AddChild(zoom_window_);
to which container this will be added? Will this survive when the display where
zoom UI is on is disconnected?

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:293: aura::RootWindow*
current_root = target->GetRootWindow();
IIRC, target can be on different root when input is captured (like while menu is
open). Does this work while menu is open?

http://codereview.chromium.org/10915140/diff/9001/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/accessibility_util.cc (right):

http://codereview.chromium.org/10915140/diff/9001/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/accessibility_util.cc:245: }
nuke {}

http://codereview.chromium.org/10915140/diff/9001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

http://codereview.chromium.org/10915140/diff/9001/ui/compositor/layer.h#newco...
ui/compositor/layer.h:383: int zoom_inset_;
document

[piman, 2012-09-13 17:22:05]

ui/compositor looks good after other comments addressed.

[Zachary Kuznia, 2012-10-09 09:02:36]

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/magnification...
File ash/magnifier/magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/magnification...
ash/magnifier/magnification_controller.h:33: virtual bool IsEnabled() = 0;
On 2012/09/13 16:35:31, oshima wrote:
> can this be const?

Done.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:25: const int kMagnifierHeight
= 200;
On 2012/09/13 16:35:31, oshima wrote:
> doument

Done.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:34: explicit
ZoomDelegateView();
On 2012/09/13 16:35:31, oshima wrote:
> remove explicit

Done.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:160: int y = location.y();
On 2012/09/13 16:35:31, oshima wrote:
> or you can use gfx::Point::Offset. up to you.

Done.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:217:
SetRootWindow(ash::Shell::GetActiveRootWindow());
On 2012/09/13 16:35:31, oshima wrote:
> Don't you want to use root window where the mouse pointer is currently on?

What is the proper way to get that window?  Check all of the root windows for
the mouse cursor?

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:223:
root_window_->AddChild(zoom_window_);
On 2012/09/13 16:35:31, oshima wrote:
> to which container this will be added? Will this survive when the display
where
> zoom UI is on is disconnected?

It's added after the containers, so that it should always be in front of them.

http://codereview.chromium.org/10915140/diff/9001/ash/magnifier/partial_magni...
ash/magnifier/partial_magnification_controller.cc:293: aura::RootWindow*
current_root = target->GetRootWindow();
On 2012/09/13 16:35:31, oshima wrote:
> IIRC, target can be on different root when input is captured (like while menu
is
> open). Does this work while menu is open? 

It seems to work properly with menus.

http://codereview.chromium.org/10915140/diff/9001/chrome/browser/chromeos/acc...
File chrome/browser/chromeos/accessibility/accessibility_util.cc (right):

http://codereview.chromium.org/10915140/diff/9001/chrome/browser/chromeos/acc...
chrome/browser/chromeos/accessibility/accessibility_util.cc:245: }
On 2012/09/13 16:35:31, oshima wrote:
> nuke {}

Done.

http://codereview.chromium.org/10915140/diff/9001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

http://codereview.chromium.org/10915140/diff/9001/ui/compositor/layer.h#newco...
ui/compositor/layer.h:383: int zoom_inset_;
On 2012/09/13 16:35:31, oshima wrote:
> document

Done.

[piman, 2012-10-09 17:38:04]

http://codereview.chromium.org/10915140/diff/13001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10915140/diff/13001/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:67: zoom_(1),
zoom_inset_ should be initialized.

http://codereview.chromium.org/10915140/diff/13001/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:93: zoom_(1),
zoom_inset_ should be initialized.

[oshima, 2012-10-10 00:57:47]

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:93: void OnMouseMove(const
gfx::Point& location);
specify coordinate (location_in_root)

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:127: aura::RootWindow*
root_window_;
Any chance that using zoom_window_'s root window may simplify the code? You care
it only when there is a zoom_window, right?

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:224:
SetRootWindow(ash::Shell::GetActiveRootWindow());
I'm not sure about exact use case, but ActiveRootWindow may not be the right
choice. Isn't it better to use the one mouse pointer is located (when mouse
pointer is visible).

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:230:
root_window_->AddChild(zoom_window_);
What happen if I lock the screen while zoom window is shown?

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:289: if (IsPartialMagnified()
&& event->type() == ui::ET_MOUSE_MOVED) {
you may want to try ET_MOUSE_ENTERED instead of ET_MOUSE_MOVED>

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:321: aura::RootWindow*
new_root_window = ash::Shell::GetActiveRootWindow();
I'd like to minimize the use of active root window, and in this case, you can
just try PrimaryRootWindow().

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:35: virtual void SetScale(float
scale) = 0;
new line between methods

http://codereview.chromium.org/10915140/diff/13001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/10915140/diff/13001/chrome/common/pref_names.h...
chrome/common/pref_names.h:269: extern const char
kPartialScreenMagnifierEnabled[];
isn't this ash/aura specific?

[Zachary Kuznia, 2012-10-10 09:45:43]

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:93: void OnMouseMove(const
gfx::Point& location);
On 2012/10/10 00:57:47, oshima wrote:
> specify coordinate (location_in_root)

Done.

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:127: aura::RootWindow*
root_window_;
On 2012/10/10 00:57:47, oshima wrote:
> Any chance that using zoom_window_'s root window may simplify the code? You
care
> it only when there is a zoom_window, right?

Done.

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:224:
SetRootWindow(ash::Shell::GetActiveRootWindow());
On 2012/10/10 00:57:47, oshima wrote:
> I'm not sure about exact use case, but ActiveRootWindow may not be the right
> choice. Isn't it better to use the one mouse pointer is located (when mouse
> pointer is visible).

Done.

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:230:
root_window_->AddChild(zoom_window_);
On 2012/10/10 00:57:47, oshima wrote:
> What happen if I lock the screen while zoom window is shown?

The screen magnifier should stay active.  Since the window is added on top of
the containers, it will still work on the lock screen.

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:289: if (IsPartialMagnified()
&& event->type() == ui::ET_MOUSE_MOVED) {
On 2012/10/10 00:57:47, oshima wrote:
> you may want to try ET_MOUSE_ENTERED instead of ET_MOUSE_MOVED>

Won't that only give events when the mouse changes windows?  The window should
be following the mouse cursor.

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:321: aura::RootWindow*
new_root_window = ash::Shell::GetActiveRootWindow();
Switched to internal function.

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/13001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:35: virtual void SetScale(float
scale) = 0;
On 2012/10/10 00:57:47, oshima wrote:
> new line between methods

Done.

http://codereview.chromium.org/10915140/diff/13001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/10915140/diff/13001/chrome/common/pref_names.h...
chrome/common/pref_names.h:269: extern const char
kPartialScreenMagnifierEnabled[];
On 2012/10/10 00:57:47, oshima wrote:
> isn't this ash/aura specific?

It's Chrome OS specific.

http://codereview.chromium.org/10915140/diff/13001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10915140/diff/13001/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:67: zoom_(1),
On 2012/10/09 17:38:05, piman wrote:
> zoom_inset_ should be initialized.

Done.

http://codereview.chromium.org/10915140/diff/13001/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:93: zoom_(1),
On 2012/10/09 17:38:05, piman wrote:
> zoom_inset_ should be initialized.

Done.

[piman, 2012-10-10 16:33:10]

LGTM

[piman, 2012-10-10 16:33:11]

LGTM

[oshima, 2012-10-10 19:55:52]

lgtm with nits

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:71: child_at(0)->SetBounds(0,
0, width(), height());
if (child_count())
  child_at(0)->SetBounds(0, 0, width(), height());

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:168: kMagnifierWidth,
kMagnifierHeight));
you need {} in this case.

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:206:
root_window->GetLastMouseLocationInRoot()))
indent

http://codereview.chromium.org/10915140/diff/17003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10915140/diff/17003/ash/shell.cc#newcode93
ash/shell.cc:93: #include "ui/views/widget/widget_delegate.h"
do you need this?

http://codereview.chromium.org/10915140/diff/17003/ash/shell.cc#newcode516
ash/shell.cc:516: //display_controller_->InitSecondaryDisplays();
remove this.

[Zachary Kuznia, 2012-10-11 08:57:23]

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:71: child_at(0)->SetBounds(0,
0, width(), height());
On 2012/10/10 19:55:52, oshima wrote:
> if (child_count())
>   child_at(0)->SetBounds(0, 0, width(), height());

Done.

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:168: kMagnifierWidth,
kMagnifierHeight));
On 2012/10/10 19:55:52, oshima wrote:
> you need {} in this case.

Done.

http://codereview.chromium.org/10915140/diff/17003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:206:
root_window->GetLastMouseLocationInRoot()))
On 2012/10/10 19:55:52, oshima wrote:
> indent

Done.

http://codereview.chromium.org/10915140/diff/17003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10915140/diff/17003/ash/shell.cc#newcode93
ash/shell.cc:93: #include "ui/views/widget/widget_delegate.h"
On 2012/10/10 19:55:52, oshima wrote:
> do you need this?

Done.

http://codereview.chromium.org/10915140/diff/17003/ash/shell.cc#newcode516
ash/shell.cc:516: //display_controller_->InitSecondaryDisplays();
On 2012/10/10 19:55:52, oshima wrote:
> remove this.

Done.

[sky, 2012-10-11 17:00:48]

http://codereview.chromium.org/10915140/diff/26003/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10915140/diff/26003/ash/ash.gyp#newcode137
ash/ash.gyp:137: 'keyboard_overlay/keyboard_overlay_delegate.cc',
Can you fix the sort order here. The keyboard directory should be moved up.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:54: 
remove newline

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:65: return child_count() > 0 ?
child_at(0)->GetPreferredSize() : gfx::Size();
Set the layout manager to FillLayout and you can remove this and Layout.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:135: aura::Window*
zoom_window_;
scoped_ptr

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:135: aura::Window*
zoom_window_;
Why do you need zoom_window_? AFAICT you create it only to parent zoom_widget_
to it. Why can't you create zoom_widget_ parented to the root?

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:159: void
PartialMagnificationControllerImpl::OnMouseMove(
What if someone is using touch input?

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:224: aura::RootWindow*
root_window = GetCurrentRootWindow();
Move all this into a function, maybe CreateZoomWindow or something.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:240: params.parent =
zoom_window_;
Do you want to set can_activate and accept_events to false?

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:248:
zoom_widget_->SetContentsView(delegate_view);
You should override GetContentsView() on ZoomDelegateView to return 'this'
instead of explicitly setting the contents view. Doing so avoid unnecessary
creation.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:269: if (enabled) {
Do you need both is_enabled_ and the scale? If you have a non-zero scale doesn't
that mean its enabled?

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:20: class
PartialMagnificationController {
Is there a reason to make this pure virtual?

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:304: void Layer::SetBackgroundZoom(float x_offset, float
y_offset, float zoom,
When you wrap one arg, wrap them all.

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.h
File ui/compositor/layer.h (right):

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.h#newc...
ui/compositor/layer.h:171: // Zoom the background by this amount.
Its not obvious what 'inset' is, document it.

[Zachary Kuznia, 2012-10-12 08:41:34]

http://codereview.chromium.org/10915140/diff/26003/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/10915140/diff/26003/ash/ash.gyp#newcode137
ash/ash.gyp:137: 'keyboard_overlay/keyboard_overlay_delegate.cc',
On 2012/10/11 17:00:49, sky wrote:
> Can you fix the sort order here. The keyboard directory should be moved up.

Done.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:54: 
On 2012/10/11 17:00:49, sky wrote:
> remove newline

Done.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:65: return child_count() > 0 ?
child_at(0)->GetPreferredSize() : gfx::Size();
On 2012/10/11 17:00:49, sky wrote:
> Set the layout manager to FillLayout and you can remove this and Layout.

Done.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:135: aura::Window*
zoom_window_;
On 2012/10/11 17:00:49, sky wrote:
> Why do you need zoom_window_? AFAICT you create it only to parent zoom_widget_
> to it. Why can't you create zoom_widget_ parented to the root?

Done.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:159: void
PartialMagnificationControllerImpl::OnMouseMove(
On 2012/10/11 17:00:49, sky wrote:
> What if someone is using touch input?

Support for touch input will come in a future patch.  We haven't decided the
proper gestures yet.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:224: aura::RootWindow*
root_window = GetCurrentRootWindow();
On 2012/10/11 17:00:49, sky wrote:
> Move all this into a function, maybe CreateZoomWindow or something.

Done.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:240: params.parent =
zoom_window_;
On 2012/10/11 17:00:49, sky wrote:
> Do you want to set can_activate and accept_events to false?

Done.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:248:
zoom_widget_->SetContentsView(delegate_view);
On 2012/10/11 17:00:49, sky wrote:
> You should override GetContentsView() on ZoomDelegateView to return 'this'
> instead of explicitly setting the contents view. Doing so avoid unnecessary
> creation.

Doing this causes it to crash in FillLayout().  Does it require a matching
change to the layout code?

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:269: if (enabled) {
On 2012/10/11 17:00:49, sky wrote:
> Do you need both is_enabled_ and the scale? If you have a non-zero scale
doesn't
> that mean its enabled?

They are different.  When it's enabled, you can toggle the scale between
kDefaultPartialMagnifiedScale and kNonPartialMagnifiedScale via a hotkey.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:20: class
PartialMagnificationController {
On 2012/10/11 17:00:49, sky wrote:
> Is there a reason to make this pure virtual?

It's done this way in order to match the style of magnification_controller.h

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.cc#new...
ui/compositor/layer.cc:304: void Layer::SetBackgroundZoom(float x_offset, float
y_offset, float zoom,
On 2012/10/11 17:00:49, sky wrote:
> When you wrap one arg, wrap them all.

Done.

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.h
File ui/compositor/layer.h (right):

http://codereview.chromium.org/10915140/diff/26003/ui/compositor/layer.h#newc...
ui/compositor/layer.h:171: // Zoom the background by this amount.
On 2012/10/11 17:00:49, sky wrote:
> Its not obvious what 'inset' is, document it.

Done.

[sky, 2012-10-12 17:09:18]

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:248:
zoom_widget_->SetContentsView(delegate_view);
On 2012/10/12 08:41:35, Zachary Kuznia wrote:
> On 2012/10/11 17:00:49, sky wrote:
> > You should override GetContentsView() on ZoomDelegateView to return 'this'
> > instead of explicitly setting the contents view. Doing so avoid unnecessary
> > creation.
> 
> Doing this causes it to crash in FillLayout().  Does it require a matching
> change to the layout code?

what is the crash. Maybe FillLayoutGetPreferredSize needs to do notify if
!has_children like Layout?

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:20: class
PartialMagnificationController {
On 2012/10/12 08:41:35, Zachary Kuznia wrote:
> On 2012/10/11 17:00:49, sky wrote:
> > Is there a reason to make this pure virtual?
> 
> It's done this way in order to match the style of magnification_controller.h

Is there a reason for that?

[Zachary Kuznia, 2012-10-15 08:28:40]

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:248:
zoom_widget_->SetContentsView(delegate_view);
On 2012/10/12 17:09:19, sky wrote:
> On 2012/10/12 08:41:35, Zachary Kuznia wrote:
> > On 2012/10/11 17:00:49, sky wrote:
> > > You should override GetContentsView() on ZoomDelegateView to return 'this'
> > > instead of explicitly setting the contents view. Doing so avoid
unnecessary
> > > creation.
> > 
> > Doing this causes it to crash in FillLayout().  Does it require a matching
> > change to the layout code?
> 
> what is the crash. Maybe FillLayoutGetPreferredSize needs to do notify if
> !has_children like Layout?

Actually, it turns out that since I removed zoom_window_, I don't need the
delegate anymore, so I removed it.

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:20: class
PartialMagnificationController {
On 2012/10/12 17:09:19, sky wrote:
> On 2012/10/12 08:41:35, Zachary Kuznia wrote:
> > On 2012/10/11 17:00:49, sky wrote:
> > > Is there a reason to make this pure virtual?
> > 
> > It's done this way in order to match the style of magnification_controller.h
> 
> Is there a reason for that?

Apparently the reason was to keep the header file simpler.  Would you prefer if
I eliminated the pure virtual interface?

[sky, 2012-10-15 22:48:11]

On Mon, Oct 15, 2012 at 1:28 AM,  <zork@chromium.org> wrote:
>
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> File ash/magnifier/partial_magnification_controller.cc (right):
>
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> ash/magnifier/partial_magnification_controller.cc:248:
> zoom_widget_->SetContentsView(delegate_view);
> On 2012/10/12 17:09:19, sky wrote:
>>
>> On 2012/10/12 08:41:35, Zachary Kuznia wrote:
>> > On 2012/10/11 17:00:49, sky wrote:
>> > > You should override GetContentsView() on ZoomDelegateView to
>
> return 'this'
>>
>> > > instead of explicitly setting the contents view. Doing so avoid
>
> unnecessary
>>
>> > > creation.
>> >
>> > Doing this causes it to crash in FillLayout().  Does it require a
>
> matching
>>
>> > change to the layout code?
>
>
>> what is the crash. Maybe FillLayoutGetPreferredSize needs to do notify
>
> if
>>
>> !has_children like Layout?
>
>
> Actually, it turns out that since I removed zoom_window_, I don't need
> the delegate anymore, so I removed it.
>
>
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> File ash/magnifier/partial_magnification_controller.h (right):
>
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> ash/magnifier/partial_magnification_controller.h:20: class
> PartialMagnificationController {
> On 2012/10/12 17:09:19, sky wrote:
>>
>> On 2012/10/12 08:41:35, Zachary Kuznia wrote:
>> > On 2012/10/11 17:00:49, sky wrote:
>> > > Is there a reason to make this pure virtual?
>> >
>> > It's done this way in order to match the style of
>
> magnification_controller.h
>
>> Is there a reason for that?
>
>
> Apparently the reason was to keep the header file simpler.  Would you
> prefer if I eliminated the pure virtual interface?
>
> http://codereview.chromium.org/10915140/

Yes.

  -Scott

[Zachary Kuznia, 2012-10-16 08:07:31]

On 2012/10/15 22:48:11, sky wrote:
> On Mon, Oct 15, 2012 at 1:28 AM,  <mailto:zork@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> > File ash/magnifier/partial_magnification_controller.cc (right):
> >
> >
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> > ash/magnifier/partial_magnification_controller.cc:248:
> > zoom_widget_->SetContentsView(delegate_view);
> > On 2012/10/12 17:09:19, sky wrote:
> >>
> >> On 2012/10/12 08:41:35, Zachary Kuznia wrote:
> >> > On 2012/10/11 17:00:49, sky wrote:
> >> > > You should override GetContentsView() on ZoomDelegateView to
> >
> > return 'this'
> >>
> >> > > instead of explicitly setting the contents view. Doing so avoid
> >
> > unnecessary
> >>
> >> > > creation.
> >> >
> >> > Doing this causes it to crash in FillLayout().  Does it require a
> >
> > matching
> >>
> >> > change to the layout code?
> >
> >
> >> what is the crash. Maybe FillLayoutGetPreferredSize needs to do notify
> >
> > if
> >>
> >> !has_children like Layout?
> >
> >
> > Actually, it turns out that since I removed zoom_window_, I don't need
> > the delegate anymore, so I removed it.
> >
> >
> >
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> > File ash/magnifier/partial_magnification_controller.h (right):
> >
> >
>
http://codereview.chromium.org/10915140/diff/26003/ash/magnifier/partial_magn...
> > ash/magnifier/partial_magnification_controller.h:20: class
> > PartialMagnificationController {
> > On 2012/10/12 17:09:19, sky wrote:
> >>
> >> On 2012/10/12 08:41:35, Zachary Kuznia wrote:
> >> > On 2012/10/11 17:00:49, sky wrote:
> >> > > Is there a reason to make this pure virtual?
> >> >
> >> > It's done this way in order to match the style of
> >
> > magnification_controller.h
> >
> >> Is there a reason for that?
> >
> >
> > Apparently the reason was to keep the header file simpler.  Would you
> > prefer if I eliminated the pure virtual interface?
> >
> > http://codereview.chromium.org/10915140/
> 
> Yes.
> 
>   -Scott

Done.

[sky, 2012-10-16 16:29:22]

Oshima, could you comment on
http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:128: aura::RootWindow*
new_root_window = GetCurrentRootWindow();
This makes me nervous. In particular this assumes at the time you get the
notification the root is being destroyed it is no longer in the list of
RootWindows. That is currently the case, but this seems a bit fragile. I think
it would be better to make DisplayController send notification after removing a
RootWindow and listen just to that.
Oshima, WDYT?

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:139: if (root_window)
This should be a DCHECK. And refactor CloseMagnifierWindow to share code.

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:222: SetScale(scale);
Why the SetScale here? And if you really need it, why not SetScale(GetScale())?

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:26: : public aura::EventFilter,
nit: indent 4.

http://codereview.chromium.org/10915140/diff/39001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10915140/diff/39001/ash/shell.h#newcode459
ash/shell.h:459: scoped_ptr<internal::PartialMagnificationController>
Don't expose classes in the internal namespace that are used outside of ash. If
you need it outside of ash it shouldn't be in internal.

[oshima, 2012-10-16 23:43:23]

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:65: return scale_;
this should be accessor in header.

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:97: gfx::Rect root_bounds =
current_root->bounds();
This probably doesn't work if event is captured by the window on other display.
(Try magnifier while menu is opened)
If you want to address this in separate CL, add TODO.

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:128: aura::RootWindow*
new_root_window = GetCurrentRootWindow();
On 2012/10/16 16:29:22, sky wrote:
> This makes me nervous. In particular this assumes at the time you get the
> notification the root is being destroyed it is no longer in the list of
> RootWindows. That is currently the case, but this seems a bit fragile. I think
> it would be better to make DisplayController send notification after removing
a
> RootWindow and listen just to that.
> Oshima, WDYT?

I actually wonder if we ever need this. When display is disconnected,
zoom widget gets closed above, and zoom widget will be created on
next PreHandleMouseEvent, won't it?

[Zachary Kuznia, 2012-11-12 08:43:45]

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.cc (right):

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:97: gfx::Rect root_bounds =
current_root->bounds();
Added TODO

On 2012/10/16 23:43:24, oshima wrote:
> This probably doesn't work if event is captured by the window on other
display.
> (Try magnifier while menu is opened)
> If you want to address this in separate CL, add TODO.

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:128: aura::RootWindow*
new_root_window = GetCurrentRootWindow();
That is correct, it will get recreated when the mouse is moved.  Shall I remove
this code?

On 2012/10/16 23:43:24, oshima wrote:
> On 2012/10/16 16:29:22, sky wrote:
> > This makes me nervous. In particular this assumes at the time you get the
> > notification the root is being destroyed it is no longer in the list of
> > RootWindows. That is currently the case, but this seems a bit fragile. I
think
> > it would be better to make DisplayController send notification after
removing
> a
> > RootWindow and listen just to that.
> > Oshima, WDYT?
> 
> I actually wonder if we ever need this. When display is disconnected,
> zoom widget gets closed above, and zoom widget will be created on
> next PreHandleMouseEvent, won't it?

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:139: if (root_window)
On 2012/10/16 16:29:22, sky wrote:
> This should be a DCHECK. And refactor CloseMagnifierWindow to share code.

Done.

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.cc:222: SetScale(scale);
Added a comment to clarify what's going on.

On 2012/10/16 16:29:22, sky wrote:
> Why the SetScale here? And if you really need it, why not
SetScale(GetScale())?

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
File ash/magnifier/partial_magnification_controller.h (right):

http://codereview.chromium.org/10915140/diff/39001/ash/magnifier/partial_magn...
ash/magnifier/partial_magnification_controller.h:26: : public aura::EventFilter,
On 2012/10/16 16:29:22, sky wrote:
> nit: indent 4.

Done.

http://codereview.chromium.org/10915140/diff/39001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/10915140/diff/39001/ash/shell.h#newcode459
ash/shell.h:459: scoped_ptr<internal::PartialMagnificationController>
On 2012/10/16 16:29:22, sky wrote:
> Don't expose classes in the internal namespace that are used outside of ash.
If
> you need it outside of ash it shouldn't be in internal.

Done.

[sky, 2012-11-12 15:23:45]

https://codereview.chromium.org/10915140/diff/47001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/10915140/diff/47001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:246:
->partial_magnification_controller()->IsEnabled()) {
nit: -> on previous line (you do on 248).

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
File ash/magnifier/partial_magnification_controller.cc (right):

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
ash/magnifier/partial_magnification_controller.cc:132: DCHECK(widget ==
zoom_widget_);
DCHECK_EQ

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
ash/magnifier/partial_magnification_controller.cc:177: zoom_widget_->Show();
Set a name on the window associated with the widget so we can identify it later.

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
File ash/magnifier/partial_magnification_controller.h (right):

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
ash/magnifier/partial_magnification_controller.h:36: bool IsEnabled() const;
is_enabled() and inline.

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

https://codereview.chromium.org/10915140/diff/47001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_util.cc:191:
pref_service->SetBoolean(prefs::kScreenMagnifierEnabled,
Can we have a single string preference for these?

https://codereview.chromium.org/10915140/diff/47001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/accessibility_util.h (right):

https://codereview.chromium.org/10915140/diff/47001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_util.h:27: MagnifierNone,
Enum use ALL_CAPS style

[Zachary Kuznia, 2012-11-13 08:16:14]

https://codereview.chromium.org/10915140/diff/47001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/10915140/diff/47001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:246:
->partial_magnification_controller()->IsEnabled()) {
On 2012/11/12 15:23:46, sky wrote:
> nit: -> on previous line (you do on 248).

Done.

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
File ash/magnifier/partial_magnification_controller.cc (right):

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
ash/magnifier/partial_magnification_controller.cc:132: DCHECK(widget ==
zoom_widget_);
On 2012/11/12 15:23:46, sky wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
ash/magnifier/partial_magnification_controller.cc:177: zoom_widget_->Show();
On 2012/11/12 15:23:46, sky wrote:
> Set a name on the window associated with the widget so we can identify it
later.

Done.

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
File ash/magnifier/partial_magnification_controller.h (right):

https://codereview.chromium.org/10915140/diff/47001/ash/magnifier/partial_mag...
ash/magnifier/partial_magnification_controller.h:36: bool IsEnabled() const;
On 2012/11/12 15:23:46, sky wrote:
> is_enabled() and inline.

Done.

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

https://codereview.chromium.org/10915140/diff/47001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_util.cc:191:
pref_service->SetBoolean(prefs::kScreenMagnifierEnabled,
On 2012/11/12 15:23:46, sky wrote:
> Can we have a single string preference for these?

Done.

https://codereview.chromium.org/10915140/diff/47001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/accessibility_util.h (right):

https://codereview.chromium.org/10915140/diff/47001/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/accessibility_util.h:27: MagnifierNone,
On 2012/11/12 15:23:46, sky wrote:
> Enum use ALL_CAPS style

Done.

[sky, 2012-11-13 17:30:08]

LGTM

[commit-bot: I haz the power, 2012-11-14 09:16:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10915140/56003

[commit-bot: I haz the power, 2012-11-14 09:52:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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, 2012-11-15 04:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10915140/54011

[commit-bot: I haz the power, 2012-11-15 06:52:57]

Change committed as 167878