Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10388141:
Full-screen Magnifier: Support warping the cursor on the edge (Closed)
|
Created:
8 years, 7 months ago by yoshiki Modified:
8 years, 6 months ago CC:
chromium-reviews, mazda+watch_chromium.org, sadrul, derat+watch_chromium.org, ben+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFull-screen Magnifier: Support warping the cursor on the edge
With this CL, a magnified region will moved when the cursor on the edge.
Only on debug build, we can test the this magnifier with "Ctrl + {" and "Ctrl + }" shortcut. These shortcut keys are temporary. These will be removed just after either the shortcuts or setting UI are fixed.
BUG=126880
TEST=manual test (even with high-DPI and external display)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=143514
Patch Set 1 #
Total comments: 14
Patch Set 2 : sync & fix reviews #Patch Set 3 : Make MagnificationController virtual class. #
Total comments: 7
Patch Set 4 : Change behaivior of the magnified region. #
Total comments: 18
Patch Set 5 : Review fix #
Total comments: 26
Patch Set 6 : a #Patch Set 7 : sync #Patch Set 8 : Review fix #
Total comments: 16
Patch Set 9 : review fix #
Total comments: 6
Patch Set 10 : review fix #
Total comments: 7
Patch Set 11 : Review fix #Patch Set 12 : sync #
Messages
Total messages: 32 (0 generated)
zork: Could you take a look? sky: Could you take a look at ui/aura/* and ash/shell.cc? I added the |magnification_layer_| to the root window and the magnifier uses it instead of |layer_|, because we want to avoid from transforming the location of a mouse event. Although this CL works correctlly, I wonder if this way is good or not from the point of view of performance and aura design. Please let me know what you think about the approach.
Could you describe the effect you're going after? Are you looking to only magnify a region under the cursor, or are you looking to scale the whole screen? -Scott On Tue, May 15, 2012 at 3:26 AM, <yoshiki@chromium.org> wrote: > Reviewers: sky, Zachary Kuznia, > > Message: > zork: Could you take a look? > > sky: Could you take a look at ui/aura/* and ash/shell.cc? I added the > |magnification_layer_| to the root window and the magnifier uses it instead > of > |layer_|, because we want to avoid from transforming the location of a mouse > event. Although this CL works correctlly, I wonder if this way is good or > not > from the point of view of performance and aura design. Please let me know > what > you think about the approach. > > Description: > Full-screen Magnifier: Add magnified mouse cursor class. > > In this CL, a magnified cursor is just black 10x10px square (a blank layer). > It > will be replaced magnified cursor icon or accessible-friendly cursor. And > the > magnified window tracks the magnified mouse cursor. > > Only on debug build, we can test the this magnifier with "Ctrl + {" and > "Ctrl + > }" shortcut. These shortcut keys are temporary. These will be removed just > after > either the shortcuts or setting UI are fixed. > > BUG=126880 > TEST=manual test (even with high-DPI and external display) > > Please review this at https://chromiumcodereview.appspot.com/10388141/ > > SVN Base: http://git.chromium.org/chromium/src.git@master > > Affected files: > M ash/accelerators/accelerator_controller.cc > M ash/accelerators/accelerator_table.h > M ash/accelerators/accelerator_table.cc > M ash/ash.gyp > M ash/magnifier/magnification_controller.h > M ash/magnifier/magnification_controller.cc > A ash/magnifier/magnified_cursor.h > A ash/magnifier/magnified_cursor.cc > M ash/shell.cc > M ui/aura/event.h > M ui/aura/root_window.h > M ui/aura/root_window.cc > >
We'd like to magnify a region under the cursor to full-screen and move the region when cursor is the edge of the region. And we are also planning to magnify the region or point specified by accessibility extension (planning to scale the forced element or so). On Wed, May 16, 2012 at 12:16 AM, Scott Violet <sky@chromium.org> wrote: > Could you describe the effect you're going after? Are you looking to > only magnify a region under the cursor, or are you looking to scale > the whole screen? > > -Scott > > On Tue, May 15, 2012 at 3:26 AM, <yoshiki@chromium.org> wrote: >> Reviewers: sky, Zachary Kuznia, >> >> Message: >> zork: Could you take a look? >> >> sky: Could you take a look at ui/aura/* and ash/shell.cc? I added the >> |magnification_layer_| to the root window and the magnifier uses it instead >> of >> |layer_|, because we want to avoid from transforming the location of a mouse >> event. Although this CL works correctlly, I wonder if this way is good or >> not >> from the point of view of performance and aura design. Please let me know >> what >> you think about the approach. >> >> Description: >> Full-screen Magnifier: Add magnified mouse cursor class. >> >> In this CL, a magnified cursor is just black 10x10px square (a blank layer). >> It >> will be replaced magnified cursor icon or accessible-friendly cursor. And >> the >> magnified window tracks the magnified mouse cursor. >> >> Only on debug build, we can test the this magnifier with "Ctrl + {" and >> "Ctrl + >> }" shortcut. These shortcut keys are temporary. These will be removed just >> after >> either the shortcuts or setting UI are fixed. >> >> BUG=126880 >> TEST=manual test (even with high-DPI and external display) >> >> Please review this at https://chromiumcodereview.appspot.com/10388141/ >> >> SVN Base: http://git.chromium.org/chromium/src.git@master >> >> Affected files: >> M ash/accelerators/accelerator_controller.cc >> M ash/accelerators/accelerator_table.h >> M ash/accelerators/accelerator_table.cc >> M ash/ash.gyp >> M ash/magnifier/magnification_controller.h >> M ash/magnifier/magnification_controller.cc >> A ash/magnifier/magnified_cursor.h >> A ash/magnifier/magnified_cursor.cc >> M ash/shell.cc >> M ui/aura/event.h >> M ui/aura/root_window.h >> M ui/aura/root_window.cc >> >>
I only made it part way through the patch. Based on the patch it looks like you're magnifying the whole screen, and not just a region. Is that right? https://chromiumcodereview.appspot.com/10388141/diff/1/ash/magnifier/magnifie... File ash/magnifier/magnified_cursor.h (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ash/magnifier/magnifie... ash/magnifier/magnified_cursor.h:21: class MagnifiedCursor: public ui::LayerDelegate { space after cursor. https://chromiumcodereview.appspot.com/10388141/diff/1/ash/magnifier/magnifie... ash/magnifier/magnified_cursor.h:23: MagnifiedCursor(aura::RootWindow* root_window); explicit https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/event.h File ui/aura/event.h (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/event.h#newcod... ui/aura/event.h:175: bool HasLocation() { return type() != ui::ET_MOUSE_CAPTURE_CHANGED; } I don't think its worth adding this. https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.cc File ui/aura/root_window.cc (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.cc... ui/aura/root_window.cc:328: event->UpdateForRootTransform(magnification_layer()->transform()); Why wouldn't it effect mouse or gesture events? https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h File ui/aura/root_window.h (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:123: void SetForceHideCursor(bool hide) { Add description https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:124: force_hide_cursor_ = hide; SHouldn't this update the cursor immediately if the value is changing? https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:357: scoped_ptr<ui::Layer> magnification_layer_; Add description.
Scott, thanks you for looking. You are right. This CL magnifies the whole screen, and not just a targeted region. But the remaining area outside the targeted region would be put out of the host screen by magnification, so that the user can see only the magnified targeted region. And I've made MagnificationController class virtual at Patch Set #3, as Zach suggested offine. https://chromiumcodereview.appspot.com/10388141/diff/1/ash/magnifier/magnifie... File ash/magnifier/magnified_cursor.h (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ash/magnifier/magnifie... ash/magnifier/magnified_cursor.h:21: class MagnifiedCursor: public ui::LayerDelegate { On 2012/05/16 15:40:48, sky wrote: > space after cursor. Done. https://chromiumcodereview.appspot.com/10388141/diff/1/ash/magnifier/magnifie... ash/magnifier/magnified_cursor.h:23: MagnifiedCursor(aura::RootWindow* root_window); On 2012/05/16 15:40:48, sky wrote: > explicit Done. https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/event.h File ui/aura/event.h (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/event.h#newcod... ui/aura/event.h:175: bool HasLocation() { return type() != ui::ET_MOUSE_CAPTURE_CHANGED; } On 2012/05/16 15:40:48, sky wrote: > I don't think its worth adding this. Done. https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.cc File ui/aura/root_window.cc (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.cc... ui/aura/root_window.cc:328: event->UpdateForRootTransform(magnification_layer()->transform()); This CL magnifies the screen with |magnification_layer_|. This is because we don't want to change the position/speed of the mouse cursor, to keep the UX of mouse movement, even after the screen is magnified. Once the screen is magnified by |magnification_layer_|, the hardware cursor by the host indicated the wrong position because mouse events is not transformed. But this is not the problem, because we hide the hardware cursor and draw a software cursor instead. And scroll events are same. However, touch events should be transformed by |magnification_layer_| because it should not be misaligned. But we don't need to transform gesture events because the events are fired from touch events and they are already transformed. And the host size should not be transformed in OnHostResized(), because the desktop size/shape should not be affected by magnification scale. We don't want to make the logical desktop size smaller even after zoomed. On 2012/05/16 15:40:48, sky wrote: > Why wouldn't it effect mouse or gesture events? https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h File ui/aura/root_window.h (right): https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:123: void SetForceHideCursor(bool hide) { On 2012/05/16 15:40:48, sky wrote: > Add description Done. https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:124: force_hide_cursor_ = hide; On 2012/05/16 15:40:48, sky wrote: > SHouldn't this update the cursor immediately if the value is changing? Done. https://chromiumcodereview.appspot.com/10388141/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:357: scoped_ptr<ui::Layer> magnification_layer_; On 2012/05/16 15:40:48, sky wrote: > Add description. Done.
How come we need the magnification layer? Can't we set the transform and what not directly on the root layer?
I'm adding Antoine as a reviewer in case he has any comments.
https://chromiumcodereview.appspot.com/10388141/diff/14/ash/magnifier/magnifi... File ash/magnifier/magnified_cursor.cc (right): https://chromiumcodereview.appspot.com/10388141/diff/14/ash/magnifier/magnifi... ash/magnifier/magnified_cursor.cc:16: : root_window_(root_window) { root_window_ doesn't seem used at all. Remove? https://chromiumcodereview.appspot.com/10388141/diff/14/ash/magnifier/magnifi... ash/magnifier/magnified_cursor.cc:27: gfx::Rect rect(position.x() - 5, position.y() -5 , 10, 10); nit: space between - and 5. https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.cc File ui/aura/root_window.cc (right): https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.c... ui/aura/root_window.cc:173: magnification_layer_.reset(new ui::Layer(ui::LAYER_TEXTURED)); Does this need to be LAYER_TEXTURED? Since it has no delegate, nothing is going to paint the texture. Also, since the rest of the layer tree is going to be drawn on top, this is just going to draw black, just to be replaced by something else. Culling can help, but we should just avoid doing this in the first place. https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.c... ui/aura/root_window.cc:177: magnification_layer_->SetVisible(true); Is this layer needed at all? If it's just there to add a transform, can't we just bake it into the root layer? https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.c... ui/aura/root_window.cc:228: ShowCursor(!hide); This makes SetForceHideCursor and ShowCursor non-orthogonal which is a little surprising. If I do ShowCursor(false); SetForceHideCursor(false); then the cursor will be shown. How about we keep 2 states, (like we already have), force_hide_cursor_ (set by SetForceHideCursor) and cursor_shown_ (set by ShowCursor), and call host_->ShowCursor(cursor_shown_ && !force_hide_cursor_) when any of them changes? https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.h File ui/aura/root_window.h (right): https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.h... ui/aura/root_window.h:126: // used when chrome draws an alnative cursor instead. (cf. magnified cursor nit: s/alnative/alternative/ https://chromiumcodereview.appspot.com/10388141/diff/14/ui/aura/root_window.h... ui/aura/root_window.h:127: // for accessibility) nit: end with a period.
Scott, Sorry for late response. I created a brief document to explain why we can't set the transform to root layer directly. https://docs.google.com/a/google.com/document/d/1EHofmQRi_cpo7Ltvb0RlNHRgwNUh... On Fri, May 18, 2012 at 2:58 AM, <sky@chromium.org> wrote: > How come we need the magnification layer? Can't we set the transform and > what > not directly on the root layer? > > https://chromiumcodereview.appspot.com/10388141/
What behavior are you hoping to achieve as the user moves the mouse around? Will the user need to push against an edge to change the visible region, or within some insets from the edge? Windows addresses the problems you site by warping the cursor. That is, they appear to inset the screen slightly. When the cursor enters the insets the magnified region changes and cursor is pushed back to fall within the insets. Having to fallback to software rendering of the cursor seems like it'll make any cursor movement feel laggy. -Scott On Fri, May 25, 2012 at 5:31 PM, Yoshiki IGUCHI <yoshiki@google.com> wrote: > Scott, > > Sorry for late response. I created a brief document to explain why we > can't set the transform to root layer directly. > https://docs.google.com/a/google.com/document/d/1EHofmQRi_cpo7Ltvb0RlNHRgwNUh... > > On Fri, May 18, 2012 at 2:58 AM, <sky@chromium.org> wrote: >> How come we need the magnification layer? Can't we set the transform and >> what >> not directly on the root layer? >> >> https://chromiumcodereview.appspot.com/10388141/
Scott: thanks for good advice. I've changed the behavior of the magnified region as you suggested. This approach can remove the additional layer from root_window. And I've removed the code of magnified_cursor from this CL. I'll add magnified cursor feature using X11 custom cursor in the another CL. It will have no laggy cursor behavior. Could you take a look at this CL? On 2012/05/29 15:34:02, sky wrote: > What behavior are you hoping to achieve as the user moves the mouse > around? Will the user need to push against an edge to change the > visible region, or within some insets from the edge? > > Windows addresses the problems you site by warping the cursor. That > is, they appear to inset the screen slightly. When the cursor enters > the insets the magnified region changes and cursor is pushed back to > fall within the insets. > > Having to fallback to software rendering of the cursor seems like > it'll make any cursor movement feel laggy. > > -Scott > > On Fri, May 25, 2012 at 5:31 PM, Yoshiki IGUCHI <mailto:yoshiki@google.com> wrote: > > Scott, > > > > Sorry for late response. I created a brief document to explain why we > > can't set the transform to root layer directly. > > > https://docs.google.com/a/google.com/document/d/1EHofmQRi_cpo7Ltvb0RlNHRgwNUh... > > > > On Fri, May 18, 2012 at 2:58 AM, mailto: <sky@chromium.org> wrote: > >> How come we need the magnification layer? Can't we set the transform and > >> what > >> not directly on the root layer? > >> > >> https://chromiumcodereview.appspot.com/10388141/
sky: ping? On 2012/06/05 15:43:26, yoshiki wrote: > Scott: thanks for good advice. I've changed the behavior of the magnified region > as you suggested. This approach can remove the additional layer from > root_window. > > And I've removed the code of magnified_cursor from this CL. I'll add magnified > cursor feature using X11 custom cursor in the another CL. It will have no laggy > cursor behavior. > > Could you take a look at this CL? > > On 2012/05/29 15:34:02, sky wrote: > > What behavior are you hoping to achieve as the user moves the mouse > > around? Will the user need to push against an edge to change the > > visible region, or within some insets from the edge? > > > > Windows addresses the problems you site by warping the cursor. That > > is, they appear to inset the screen slightly. When the cursor enters > > the insets the magnified region changes and cursor is pushed back to > > fall within the insets. > > > > Having to fallback to software rendering of the cursor seems like > > it'll make any cursor movement feel laggy. > > > > -Scott > > > > On Fri, May 25, 2012 at 5:31 PM, Yoshiki IGUCHI <mailto:yoshiki@google.com> > wrote: > > > Scott, > > > > > > Sorry for late response. I created a brief document to explain why we > > > can't set the transform to root layer directly. > > > > > > https://docs.google.com/a/google.com/document/d/1EHofmQRi_cpo7Ltvb0RlNHRgwNUh... > > > > > > On Fri, May 18, 2012 at 2:58 AM, mailto: <sky@chromium.org> wrote: > > >> How come we need the magnification layer? Can't we set the transform and > > >> what > > >> not directly on the root layer? > > >> > > >> https://chromiumcodereview.appspot.com/10388141/
http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:177: float scale = (delta_index > 0) ? 1.2f : 1.0f/1.2f; Use constants (and spaces). http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:179: float curren_scale = ash::Shell::GetInstance()-> current_scale http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:182: magnification_controller()->SetScale(curren_scale * scale, true); Won't this lead to rounding errors? Wouldn't it be better to maintain scale as an int and calculate the float value from that? http://codereview.chromium.org/10388141/diff/27002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/27002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:41: void SetScale(float scale, bool animation); virtual and OVERRIDE http://codereview.chromium.org/10388141/diff/27002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:54: void OnImplicitAnimationsCompleted() OVERRIDE; methods before fields. http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:35: #include "ui/gfx/point3.h" sort http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:222: gfx::Point3f p3(location_in_dip); Why do we need point3 here?
Sorry for the delay. I got a bunch of stuff dropped in my lap yesterday. -Scott On Thu, Jun 7, 2012 at 10:04 AM, <yoshiki@chromium.org> wrote: > sky: ping? > > > On 2012/06/05 15:43:26, yoshiki wrote: >> >> Scott: thanks for good advice. I've changed the behavior of the magnified > > region >> >> as you suggested. This approach can remove the additional layer from >> root_window. > > >> And I've removed the code of magnified_cursor from this CL. I'll add >> magnified >> cursor feature using X11 custom cursor in the another CL. It will have no > > laggy >> >> cursor behavior. > > >> Could you take a look at this CL? > > >> On 2012/05/29 15:34:02, sky wrote: >> > What behavior are you hoping to achieve as the user moves the mouse >> > around? Will the user need to push against an edge to change the >> > visible region, or within some insets from the edge? >> > >> > Windows addresses the problems you site by warping the cursor. That >> > is, they appear to inset the screen slightly. When the cursor enters >> > the insets the magnified region changes and cursor is pushed back to >> > fall within the insets. >> > >> > Having to fallback to software rendering of the cursor seems like >> > it'll make any cursor movement feel laggy. >> > >> > -Scott >> > >> > On Fri, May 25, 2012 at 5:31 PM, Yoshiki IGUCHI >> > <mailto:yoshiki@google.com> >> wrote: >> > > Scott, >> > > >> > > Sorry for late response. I created a brief document to explain why we >> > > can't set the transform to root layer directly. >> > > >> > > > > https://docs.google.com/a/google.com/document/d/1EHofmQRi_cpo7Ltvb0RlNHRgwNUh... >> >> > > >> > > On Fri, May 18, 2012 at 2:58 AM, mailto: <sky@chromium.org> >> > > wrote: >> > >> How come we need the magnification layer? Can't we set the transform >> > >> and >> > >> what >> > >> not directly on the root layer? >> > >> >> > >> https://chromiumcodereview.appspot.com/10388141/ > > > > > http://codereview.chromium.org/10388141/
http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:222: gfx::Point3f p3(location_in_dip); On 2012/06/07 18:05:50, sky wrote: > Why do we need point3 here? Transform convert gfx::point to point3 internally, so you don't need this here. http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:223: layer()->transform().TransformPoint(p3); I believe location_in_dip is in DIP coordinate system that is already transformed using this transform() + scale factor. Please see render_widget_host_viwe_aura.cc
oshima: Thank you for comment and I have some questions inline. http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:177: float scale = (delta_index > 0) ? 1.2f : 1.0f/1.2f; On 2012/06/07 18:05:50, sky wrote: > Use constants (and spaces). Done. http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:179: float curren_scale = ash::Shell::GetInstance()-> On 2012/06/07 18:05:50, sky wrote: > current_scale Done. http://codereview.chromium.org/10388141/diff/27002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:182: magnification_controller()->SetScale(curren_scale * scale, true); On 2012/06/07 18:05:50, sky wrote: > Won't this lead to rounding errors? Wouldn't it be better to maintain scale as > an int and calculate the float value from that? Done. http://codereview.chromium.org/10388141/diff/27002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/27002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:41: void SetScale(float scale, bool animation); On 2012/06/07 18:05:50, sky wrote: > virtual and OVERRIDE Done. http://codereview.chromium.org/10388141/diff/27002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:54: void OnImplicitAnimationsCompleted() OVERRIDE; On 2012/06/07 18:05:50, sky wrote: > methods before fields. Done. http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:35: #include "ui/gfx/point3.h" On 2012/06/07 18:05:50, sky wrote: > sort Done. http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:222: gfx::Point3f p3(location_in_dip); On 2012/06/07 18:25:07, oshima wrote: > On 2012/06/07 18:05:50, sky wrote: > > Why do we need point3 here? > Transform convert gfx::point to point3 internally, so you don't need this here. Done. http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:223: layer()->transform().TransformPoint(p3); On 2012/06/07 18:25:07, oshima wrote: > I believe location_in_dip is in DIP coordinate system that is already > transformed using this transform() + scale factor. > Please see render_widget_host_viwe_aura.cc oshima: Sorry, I can't find it in render_widget_host_view_aura.cc. There seems to be no transforming operation. Could you teach me where is it? Anyway, I think this transform should be here, not on render_widget_host_viwe_aura.cc. Because this transforming of location, which will be sent to the host with host->MoveCursorTo(), is the reverse calculation of the transforms used for event locations from the host (event->UpdateForRootTransform()) in root_window,cc. I think the location both from the host and to the host should be transformed on same layer. What would you advise about it?
http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:181: // Mafnify the screen Magnify the screen. http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:184: scale_index = std::max(0, std::min(8, scale_index + delta_index)); Why is this static here? Shouldn't the value live with the mangificationcontroller? http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:187: SetScale(std::pow(kMagnificationFactor, scale_index), true); indent 4 http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:42: float GetScale() const { return scale_; } virtual OVERRIDE on all these. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:52: bool is_on_zooming_; Add description. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:56: int x_; Any reason you're not using gfx::Point? http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:59: // ui::ImplicitAnimationObserver overrides: methods before fields. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:73: const gfx::Rect& target_rect_in_dip, float scale, bool animation); If you can't fill all arguments on the first line, then wrap each onto its own. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:107: : scale_(1.0f), x_(0), y_(0) { each arg on its own line. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:107: : scale_(1.0f), x_(0), y_(0) { Set root_window_ and is_on_zooming_ in the member list too. http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:34: #include "ui/gfx/point3.h" remove include.
http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/27002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:223: layer()->transform().TransformPoint(p3); On 2012/06/08 22:27:45, yoshiki wrote: > On 2012/06/07 18:25:07, oshima wrote: > > I believe location_in_dip is in DIP coordinate system that is already > > transformed using this transform() + scale factor. > > Please see render_widget_host_viwe_aura.cc > > oshima: Sorry, I can't find it in render_widget_host_view_aura.cc. There seems > to be no transforming operation. Could you teach me where is it? > > Anyway, I think this transform should be here, not on > render_widget_host_viwe_aura.cc. Because this transforming of location, which > will be sent to the host with host->MoveCursorTo(), is the reverse calculation > of the transforms used for event locations from the host > (event->UpdateForRootTransform()) in root_window,cc. I think the location both > from the host and to the host should be transformed on same layer. What would > you advise about it? no, what I meant is the all mouse events fed to aura and up are already transformed in RootWindow::DispatchMouseEventImpl, including the event in render_widget_host_view_aura, so applying here means you're applying the same transform to the event location twice.
http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:207: gfx::Point mouse = root_window_->last_mouse_location(); please use the mouse event instead of using last mouse location. http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:223: layer()->transform().TransformPoint(location); I look at the code a bit closely and I think what I said was wrong. Sorry about that. And I found that RWHVA chagne (r117967) was wrong too. r117967 is wrong because MoveCursorTo takes a ponit coordinate relative to Root, but OnMouseEvent/LockMouse in RWHVA are using coordinate relative to parent window (which may not be root). UnlockMouse is using global X/Y from webkit, which may work for single monitor, but would break for multi monitor.
oshima: Thank you for investigation and I have a question. What kind of coordinate should RootWindow::MoveCursorTo() take? Global X/Y coordinate as same as RWHVA::UnlockMouse()? On 2012/06/11 21:48:30, oshima wrote: > http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... > File ash/magnifier/magnification_controller.cc (right): > > http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:207: gfx::Point mouse = > root_window_->last_mouse_location(); > please use the mouse event instead of using last mouse location. > > http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc > File ui/aura/root_window.cc (right): > > http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... > ui/aura/root_window.cc:223: layer()->transform().TransformPoint(location); > I look at the code a bit closely and I think what I said was wrong. Sorry about > that. And I found that RWHVA chagne (r117967) was wrong too. > r117967 is wrong because MoveCursorTo takes a ponit coordinate relative to Root, > but OnMouseEvent/LockMouse in RWHVA are using coordinate relative to parent > window (which may not be root). UnlockMouse is using global X/Y from webkit, > which may work for single monitor, but would break for multi monitor.
On 2012/06/13 16:31:12, yoshiki wrote: > oshima: Thank you for investigation and I have a question. What kind of > coordinate should RootWindow::MoveCursorTo() take? Global X/Y coordinate as same > as RWHVA::UnlockMouse()? It's sort of broken now. We should have Window::MoveCursorTo(const gfx::Point& point_in_local_cord), instead and hide coordinate translation. Can you fix this? > > On 2012/06/11 21:48:30, oshima wrote: > > > http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... > > File ash/magnifier/magnification_controller.cc (right): > > > > > http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... > > ash/magnifier/magnification_controller.cc:207: gfx::Point mouse = > > root_window_->last_mouse_location(); > > please use the mouse event instead of using last mouse location. > > > > http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc > > File ui/aura/root_window.cc (right): > > > > > http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... > > ui/aura/root_window.cc:223: layer()->transform().TransformPoint(location); > > I look at the code a bit closely and I think what I said was wrong. Sorry > about > > that. And I found that RWHVA chagne (r117967) was wrong too. > > r117967 is wrong because MoveCursorTo takes a ponit coordinate relative to > Root, > > but OnMouseEvent/LockMouse in RWHVA are using coordinate relative to parent > > window (which may not be root). UnlockMouse is using global X/Y from webkit, > > which may work for single monitor, but would break for multi monitor.
> It's sort of broken now. We should have Window::MoveCursorTo(const gfx::Point& > point_in_local_cord), > instead and hide coordinate translation. Can you fix this? OK, I'll fix it. On 2012/06/13 17:16:39, oshima wrote: > On 2012/06/13 16:31:12, yoshiki wrote: > > oshima: Thank you for investigation and I have a question. What kind of > > coordinate should RootWindow::MoveCursorTo() take? Global X/Y coordinate as > same > > as RWHVA::UnlockMouse()? > > It's sort of broken now. We should have Window::MoveCursorTo(const gfx::Point& > point_in_local_cord), > instead and hide coordinate translation. Can you fix this? > > > > > On 2012/06/11 21:48:30, oshima wrote: > > > > > > http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... > > > File ash/magnifier/magnification_controller.cc (right): > > > > > > > > > http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... > > > ash/magnifier/magnification_controller.cc:207: gfx::Point mouse = > > > root_window_->last_mouse_location(); > > > please use the mouse event instead of using last mouse location. > > > > > > http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc > > > File ui/aura/root_window.cc (right): > > > > > > > > > http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... > > > ui/aura/root_window.cc:223: layer()->transform().TransformPoint(location); > > > I look at the code a bit closely and I think what I said was wrong. Sorry > > about > > > that. And I found that RWHVA chagne (r117967) was wrong too. > > > r117967 is wrong because MoveCursorTo takes a ponit coordinate relative to > > Root, > > > but OnMouseEvent/LockMouse in RWHVA are using coordinate relative to parent > > > window (which may not be root). UnlockMouse is using global X/Y from webkit, > > > which may work for single monitor, but would break for multi monitor.
sky: PTAL, since the problem Oshima mentioned has been solved and committed at crrev.com/142888. http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... File ash/accelerators/accelerator_controller.cc (right): http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:181: // Mafnify the screen On 2012/06/11 16:19:14, sky wrote: > Magnify the screen. Done. http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:184: scale_index = std::max(0, std::min(8, scale_index + delta_index)); On 2012/06/11 16:19:14, sky wrote: > Why is this static here? Shouldn't the value live with the > mangificationcontroller? Done. http://codereview.chromium.org/10388141/diff/26002/ash/accelerators/accelerat... ash/accelerators/accelerator_controller.cc:187: SetScale(std::pow(kMagnificationFactor, scale_index), true); On 2012/06/11 16:19:14, sky wrote: > indent 4 Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:42: float GetScale() const { return scale_; } On 2012/06/11 16:19:14, sky wrote: > virtual OVERRIDE on all these. Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:52: bool is_on_zooming_; On 2012/06/11 16:19:14, sky wrote: > Add description. Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:56: int x_; On 2012/06/11 16:19:14, sky wrote: > Any reason you're not using gfx::Point? Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:59: // ui::ImplicitAnimationObserver overrides: On 2012/06/11 16:19:14, sky wrote: > methods before fields. Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:73: const gfx::Rect& target_rect_in_dip, float scale, bool animation); On 2012/06/11 16:19:14, sky wrote: > If you can't fill all arguments on the first line, then wrap each onto its own. Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:107: : scale_(1.0f), x_(0), y_(0) { On 2012/06/11 16:19:14, sky wrote: > each arg on its own line. Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:107: : scale_(1.0f), x_(0), y_(0) { On 2012/06/11 16:19:14, sky wrote: > Set root_window_ and is_on_zooming_ in the member list too. Done. http://codereview.chromium.org/10388141/diff/26002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:207: gfx::Point mouse = root_window_->last_mouse_location(); On 2012/06/11 21:48:30, oshima wrote: > please use the mouse event instead of using last mouse location. Done. http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:34: #include "ui/gfx/point3.h" On 2012/06/11 16:19:14, sky wrote: > remove include. Done. http://codereview.chromium.org/10388141/diff/26002/ui/aura/root_window.cc#new... ui/aura/root_window.cc:223: layer()->transform().TransformPoint(location); On 2012/06/11 21:48:30, oshima wrote: > I look at the code a bit closely and I think what I said was wrong. Sorry about > that. And I found that RWHVA chagne (r117967) was wrong too. > r117967 is wrong because MoveCursorTo takes a ponit coordinate relative to Root, > but OnMouseEvent/LockMouse in RWHVA are using coordinate relative to parent > window (which may not be root). UnlockMouse is using global X/Y from webkit, > which may work for single monitor, but would break for multi monitor. > > > Done.
http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:56: // changed. should -> Should Also, this doesn't actually describe what these do nor does it describe what the return value means. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:122: ui::ConvertPointToDIP(root_window_->layer(), position); indent 4 http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:184: ui::ConvertRectToDIP(root_window_->layer(), target_rect); indent 4 http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:24: class MagnificationController : public aura::EventFilter { Why does the virtual interface implement EventFilter? Isn't that an implementation detail? http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:26: MagnificationController() {} Shouldn't this be protected? http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:31: virtual void SetScale(float scale, bool animation) = 0; animation -> animate in all places. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:43: virtual void EnsureShowRect(const gfx::Rect& rect, bool animation) = 0; Why do we need both of these? Can't we have EnsurePointIsVisible and leave it to the caller to figure out what point is important? http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:47: DISALLOW_COPY_AND_ASSIGN(MagnificationController); This is no longer needed.
sky: thanks, PTAL. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:56: // changed. On 2012/06/19 19:47:52, sky wrote: > should -> Should > Also, this doesn't actually describe what these do nor does it describe what the > return value means. Done. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:122: ui::ConvertPointToDIP(root_window_->layer(), position); On 2012/06/19 19:47:52, sky wrote: > indent 4 Done. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:184: ui::ConvertRectToDIP(root_window_->layer(), target_rect); On 2012/06/19 19:47:52, sky wrote: > indent 4 Done. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:24: class MagnificationController : public aura::EventFilter { On 2012/06/19 19:47:52, sky wrote: > Why does the virtual interface implement EventFilter? Isn't that an > implementation detail? Done. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:26: MagnificationController() {} On 2012/06/19 19:47:52, sky wrote: > Shouldn't this be protected? Done. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:31: virtual void SetScale(float scale, bool animation) = 0; On 2012/06/19 19:47:52, sky wrote: > animation -> animate in all places. Done. http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:43: virtual void EnsureShowRect(const gfx::Rect& rect, bool animation) = 0; I'll add the scale adjustment (un-zooming) in case that the magnification window can't contain the given rect, but it is not implemented in this patch. I added the TODO comment in Impl::EnsureRectIsVisibleDIP(). On 2012/06/19 19:47:52, sky wrote: > Why do we need both of these? Can't we have EnsurePointIsVisible and leave it to > the caller to figure out what point is important? http://codereview.chromium.org/10388141/diff/41007/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:47: DISALLOW_COPY_AND_ASSIGN(MagnificationController); On 2012/06/19 19:47:52, sky wrote: > This is no longer needed. Done.
http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:120: Shell::GetInstance()->AddEnvEventFilter(this); Don't you need to remove this in the destructor? http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:41: virtual void EnsurePointIsVisible(const gfx::Point& point, bool animate) = 0; Do we really need both EnsureRectIsVisible and EnsurePointIsVisible? Can we force folks to use EnsurePointIsVisible? http://codereview.chromium.org/10388141/diff/38009/ash/shell.cc File ash/shell.cc (right): http://codereview.chromium.org/10388141/diff/38009/ash/shell.cc#newcode418 ash/shell.cc:418: magnification_controller_.reset( Does this really work now that you've made the destructor protected?
sky: PTAL http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:120: Shell::GetInstance()->AddEnvEventFilter(this); Thanks, Done. On 2012/06/20 03:54:16, sky wrote: > Don't you need to remove this in the destructor? http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/38009/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:41: virtual void EnsurePointIsVisible(const gfx::Point& point, bool animate) = 0; We are plan to use EnsureRectIsVisible() to zoom into some elements (eg. focused element) or region (eg. dialog window) for accessibility. So we need to expose this method. On 2012/06/20 03:54:16, sky wrote: > Do we really need both EnsureRectIsVisible and EnsurePointIsVisible? Can we > force folks to use EnsurePointIsVisible? http://codereview.chromium.org/10388141/diff/38009/ash/shell.cc File ash/shell.cc (right): http://codereview.chromium.org/10388141/diff/38009/ash/shell.cc#newcode418 ash/shell.cc:418: magnification_controller_.reset( Oops, I found I've forgot to add destructor to the base class. Added. On 2012/06/20 03:54:16, sky wrote: > Does this really work now that you've made the destructor protected?
http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:25: static MagnificationController* CreateInstance(); Document caller owns this. http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:27: virtual ~MagnificationController() {} constructor/destructor should be before other methods. http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:46: MagnificationController() {} I'm still confused. How does this compile with the destructor protected since Shell wants to delete this?
http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:25: static MagnificationController* CreateInstance(); On 2012/06/20 13:41:27, sky wrote: > Document caller owns this. Done. http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:27: virtual ~MagnificationController() {} On 2012/06/20 13:41:27, sky wrote: > constructor/destructor should be before other methods. Done. http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:46: MagnificationController() {} You seem to be wrong. The destructor is not protected but public, though the constructor is protected to limit the way of creating object. This patch can be compiled. Sorry and let me know if I'm wrong. On 2012/06/20 13:41:27, sky wrote: > I'm still confused. How does this compile with the destructor protected since > Shell wants to delete this?
LGTM http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.h (right): http://codereview.chromium.org/10388141/diff/32006/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.h:46: MagnificationController() {} On 2012/06/21 02:40:36, yoshiki wrote: > You seem to be wrong. The destructor is not protected but public, though the > constructor is protected to limit the way of creating object. This patch can be > compiled. > > Sorry and let me know if I'm wrong. > > > > On 2012/06/20 13:41:27, sky wrote: > > I'm still confused. How does this compile with the destructor protected since > > Shell wants to delete this? Gah. I'm clearly wrong here. Sorry about that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10388141/43001
Failed to apply patch for ash/accelerators/accelerator_table.cc: While running patch -p1 --forward --force; patching file ash/accelerators/accelerator_table.cc Hunk #1 FAILED at 114. 1 out of 1 hunk FAILED -- saving rejects to file ash/accelerators/accelerator_table.cc.rej |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
