Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
790413002:
Focus following for the non-editable controls on web page in magnifier mode. (Closed)
|
Created:
6 years ago by jennyz Modified:
6 years ago CC:
aboxhall+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, davemoore+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, kalyank, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, plundblad+watch_chromium.org, sadrul, stevenjb+watch_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFocus following for the non-editable controls on web page in magnifier mode.
BUG=426233
Committed: https://crrev.com/399d6e66077c98e0a8cdf81c7bf7a027fd325d6e
Cr-Commit-Position: refs/heads/master@{#308494}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix a nit. #
Total comments: 35
Patch Set 3 : Address code review comments. #Patch Set 4 : A little code optimization. #
Total comments: 2
Patch Set 5 : Move FocusedNodeDetails to a better place. #
Total comments: 4
Patch Set 6 : Fix nits. #Messages
Total messages: 34 (7 generated)
jennyz@chromium.org changed reviewers: + oshima@chromium.org
jennyz@chromium.org changed reviewers: + dmazzoni@chromium.org
jennyz@chromium.org changed reviewers: + sadrul@chromium.org
sadrul: please help review render_view_xx related files. Need owner's approval, thanks!
Awesome. I think it should work, but it might be a good idea to make sure it works correctly inside of an iframe.
Thanks for your suggestion. I tried with some iframe examples, looks like it works fine inside the iframe for following the focus movement. Jenny On Thu, Dec 11, 2014 at 10:51 AM, <dmazzoni@chromium.org> wrote: > Awesome. I think it should work, but it might be a good idea to make sure > it > works correctly inside of an iframe. > > > https://codereview.chromium.org/790413002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
looks like it doesn't follow the caret movement. Are we going to support that scenario? (like focused element is a text area larger than magnifier's viewport) https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_view_host_impl.cc:1227: const gfx::Rect& node_bounds) { ditto https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_view_host_impl.h:372: const gfx::Rect& node_bounds); nit: node_bounds_in_viewport
On 2014/12/11 19:08:59, oshima wrote: > looks like it doesn't follow the caret movement. Are we going to support that > scenario? (like focused element is a text area larger than magnifier's viewport) > > https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_view_host_impl.cc (right): > > https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_view_host_impl.cc:1227: const gfx::Rect& > node_bounds) { > ditto > > https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/render_view_host_impl.h (right): > > https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/render_view_host_impl.h:372: const gfx::Rect& > node_bounds); > nit: node_bounds_in_viewport The caret following feature has been implemented earlier in https://codereview.chromium.org/665903003. For the editable control, it always follow the caret position.
I have uploaded a new patch, but I could not publish the message on codereview, it comes back with some unhandled exception error, which other people also run into. PTAL at my new patch and comments in email. Thanks, Jenny On Thu, Dec 11, 2014 at 12:35 PM, <jennyz@chromium.org> wrote: > On 2014/12/11 19:08:59, oshima wrote: > >> looks like it doesn't follow the caret movement. Are we going to support >> that >> scenario? (like focused element is a text area larger than magnifier's >> > viewport) > > > https://codereview.chromium.org/790413002/diff/1/content/ > browser/renderer_host/render_view_host_impl.cc > >> File content/browser/renderer_host/render_view_host_impl.cc (right): >> > > > https://codereview.chromium.org/790413002/diff/1/content/ > browser/renderer_host/render_view_host_impl.cc#newcode1227 > >> content/browser/renderer_host/render_view_host_impl.cc:1227: const >> gfx::Rect& >> node_bounds) { >> ditto >> > > > https://codereview.chromium.org/790413002/diff/1/content/ > browser/renderer_host/render_view_host_impl.h > >> File content/browser/renderer_host/render_view_host_impl.h (right): >> > > > https://codereview.chromium.org/790413002/diff/1/content/ > browser/renderer_host/render_view_host_impl.h#newcode372 > >> content/browser/renderer_host/render_view_host_impl.h:372: const >> gfx::Rect& >> node_bounds); >> nit: node_bounds_in_viewport >> > > The caret following feature has been implemented earlier in > https://codereview.chromium.org/665903003. For the editable control, it > always > follow the caret position. > > https://codereview.chromium.org/790413002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:172: // Moves the view port to follow |rect|, if |rect| is partially or completely Move the viewport so that rect is fully visible. (assuming that's what we want) Can you also document what happens when the rect is larger than viewport? https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:683: bool start_zoom = false; shouldn't this be called "should_pan/should_scroll" (or something like that)? https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:685: const gfx::Rect window_rect = GetViewportRect(); viewport_rect https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:694: x_diff = rect_center.x() - window_center.x(); I didn't fully understand why this is using center. shouldn't x origin be rect.x() if rect.x() < viewport.x() or rect.right() - viewport.width() if viewport.right() < rect.right() ? https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:710: if (start_zoom && !is_on_animation_) { shouldn't we stop the current animation, and pan? https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.h (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.h:58: virtual gfx::Rect GetViewportRect() = 0; can this be const method? https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc:46: virtual ~MagnificationControllerTest() {} no virutal. just override https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc:76: } these methods do not have to be instance methods. Can you move them to anonymous namespace? https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:280: bool monitor_focus_change_in_page_; optional: observing_focus_change_in_page_ maybe better, but you can keep it if you want.
https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_view_host_impl.cc:1227: const gfx::Rect& node_bounds) { On 2014/12/11 19:08:58, oshima wrote: > ditto Done. https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/790413002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_view_host_impl.h:372: const gfx::Rect& node_bounds); On 2014/12/11 19:08:59, oshima wrote: > nit: node_bounds_in_viewport Done.
https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.h (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.h:61: // non-editable controls. I think you should remove the bit about 'focusNodeChanged event'. Keeping the rest of the comment would be sufficient. https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:228: } Can you use a RenderViewObserver instead of using notification? https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... content/public/browser/render_view_host.h:53: This doesn't look like the right place for this. https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... content/renderer/render_view_impl.cc:1935: } Instead of this, why not include the bounds with the call to focusedNodeChanged()?
https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:172: // Moves the view port to follow |rect|, if |rect| is partially or completely On 2014/12/12 00:48:44, oshima wrote: > Move the viewport so that rect is fully visible. (assuming that's what we want) > > Can you also document what happens when the rect is larger than viewport? Done. https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:683: bool start_zoom = false; On 2014/12/12 00:48:45, oshima wrote: > shouldn't this be called "should_pan/should_scroll" (or something like that)? Done. https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:685: const gfx::Rect window_rect = GetViewportRect(); On 2014/12/12 00:48:45, oshima wrote: > viewport_rect Done. https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:694: x_diff = rect_center.x() - window_center.x(); On 2014/12/12 00:48:45, oshima wrote: > I didn't fully understand why this is using center. > shouldn't x origin be > > rect.x() if rect.x() < viewport.x() > > or > > rect.right() - viewport.width() if viewport.right() < rect.right() > > ? For the non-editable control, we move the viewport to center it horizontally if it is not fully contained in the viewport horizontally, ditto for vertical direction. https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:710: if (start_zoom && !is_on_animation_) { On 2014/12/12 00:48:45, oshima wrote: > shouldn't we stop the current animation, and pan? Done. https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.h (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.h:58: virtual gfx::Rect GetViewportRect() = 0; On 2014/12/12 00:48:45, oshima wrote: > can this be const method? Done. https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.h:61: // non-editable controls. On 2014/12/12 17:13:49, sadrul wrote: > I think you should remove the bit about 'focusNodeChanged event'. Keeping the > rest of the comment would be sufficient. Done. https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc:46: virtual ~MagnificationControllerTest() {} On 2014/12/12 00:48:45, oshima wrote: > no virutal. just override Done. https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc:76: } On 2014/12/12 00:48:45, oshima wrote: > these methods do not have to be instance methods. Can you move them to anonymous > namespace? Done. https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:228: } On 2014/12/12 17:13:49, sadrul wrote: > Can you use a RenderViewObserver instead of using notification? Is there a strong reason you prefer using RenderViewObserver instead of notification? Since MagnificationManager has already been a NotificationObserver, I thought it is just simple to register one more notification here. https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:280: bool monitor_focus_change_in_page_; On 2014/12/12 00:48:45, oshima wrote: > optional: observing_focus_change_in_page_ maybe better, but you can keep it if > you want. Done. https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... content/public/browser/render_view_host.h:53: On 2014/12/12 17:13:49, sadrul wrote: > This doesn't look like the right place for this. Yes, I feel a little awkward too, do you have any suggestion for a better place? https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... content/renderer/render_view_impl.cc:1935: } On 2014/12/12 17:13:49, sadrul wrote: > Instead of this, why not include the bounds with the call to > focusedNodeChanged()? Do you mean include |node_bounds| in call to FocusedNodeChanged for the RenderViewObserver?
https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:694: x_diff = rect_center.x() - window_center.x(); ok, so x can just be rect_center.x() - window_rect.width() / 2; right? https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:705: y_diff = rect_center.y() - window_center.y(); ditto
https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/790413002/diff/20001/ash/magnifier/magnificat... ash/magnifier/magnification_controller.cc:694: x_diff = rect_center.x() - window_center.x(); On 2014/12/12 23:23:43, oshima wrote: > ok, so x can just be > > rect_center.x() - window_rect.width() / 2; > > right? Done.
my bits lg now but I'll review again once you get approval from sadrul.
Should there be test coverage in ash_unittests for the changes in ash/? (I see the magnification_controller_browsertest.cc, but I think it should be possible to also have some unit-tests in ash/magnifier/magnification_controller_unittest.cc) https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:228: } On 2014/12/12 23:14:45, jennyz wrote: > On 2014/12/12 17:13:49, sadrul wrote: > > Can you use a RenderViewObserver instead of using notification? > > Is there a strong reason you prefer using RenderViewObserver instead of > notification? Since MagnificationManager has already been a > NotificationObserver, I thought it is just simple to register one more > notification here. We should avoid using NotificationService when there's an alternate way of doing things (e.g. an observer interface). (see various relevant threads on chromium-dev@). From a quick look, it looks like only test code currently uses NOTIFICATION_FOCUS_CHANGED_IN_PAGE, so it would be nice to not add its use in chrome. However, looks like RenderViewObserver is actually for renderer process code, and so it can't be used here for UI code. https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... content/public/browser/render_view_host.h:53: On 2014/12/12 23:14:45, jennyz wrote: > On 2014/12/12 17:13:49, sadrul wrote: > > This doesn't look like the right place for this. > > Yes, I feel a little awkward too, do you have any suggestion for a better place? > I can't think of a better place either. This might be indicative that we should find a fix that doesn't need this. A content OWNER may have suggestions. https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... content/renderer/render_view_impl.cc:1935: } On 2014/12/12 23:14:45, jennyz wrote: > On 2014/12/12 17:13:49, sadrul wrote: > > Instead of this, why not include the bounds with the call to > > focusedNodeChanged()? > > Do you mean include |node_bounds| in call to FocusedNodeChanged for the > RenderViewObserver? I mean including the bounds in WebViewClient::focusedNodeChanged(). https://codereview.chromium.org/790413002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:160: MonitorFocusInPageChange(); Shouldn't you always call this (i.e. outside of if after changing enabled_)?
jennyz@chromium.org changed reviewers: + jam@chromium.org
Please review files under content. Sadrul has done some review, and he suggested not using NotificationService to pass focusedNodeChanged event from render process to UI. I am not very familiar with how the IPC communication between render and browser, would you please give some suggestions for the possible alternative approach?
I just looked at this. While we want to get rid of notificationservice, given that this doesn't add a new notification, but adds an extra parameter to an existing one, i'm fine with this. i do see the point that the previous usages are non-shipping code, but i don't think this makes much of a difference here since removing that notification will still roughly be the same amount of work. lgtm with moving the struct to a new file in content/public/browser
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:228: } On 2014/12/15 18:37:16, sadrul wrote: > On 2014/12/12 23:14:45, jennyz wrote: > > On 2014/12/12 17:13:49, sadrul wrote: > > > Can you use a RenderViewObserver instead of using notification? > > > > Is there a strong reason you prefer using RenderViewObserver instead of > > notification? Since MagnificationManager has already been a > > NotificationObserver, I thought it is just simple to register one more > > notification here. > > We should avoid using NotificationService when there's an alternate way of doing > things (e.g. an observer interface). (see various relevant threads on > chromium-dev@). From a quick look, it looks like only test code currently uses > NOTIFICATION_FOCUS_CHANGED_IN_PAGE, so it would be nice to not add its use in > chrome. > > However, looks like RenderViewObserver is actually for renderer process code, > and so it can't be used here for UI code. Talked to jam@, he is ok to use the existing notification in this case. https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... File content/public/browser/render_view_host.h (right): https://codereview.chromium.org/790413002/diff/20001/content/public/browser/r... content/public/browser/render_view_host.h:53: On 2014/12/15 18:37:16, sadrul wrote: > On 2014/12/12 23:14:45, jennyz wrote: > > On 2014/12/12 17:13:49, sadrul wrote: > > > This doesn't look like the right place for this. > > > > Yes, I feel a little awkward too, do you have any suggestion for a better > place? > > > > I can't think of a better place either. This might be indicative that we should > find a fix that doesn't need this. A content OWNER may have suggestions. Moved to content/public/browser/focused_node_details.h as suggested by jam@. https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/790413002/diff/20001/content/renderer/render_... content/renderer/render_view_impl.cc:1935: } On 2014/12/15 18:37:16, sadrul wrote: > On 2014/12/12 23:14:45, jennyz wrote: > > On 2014/12/12 17:13:49, sadrul wrote: > > > Instead of this, why not include the bounds with the call to > > > focusedNodeChanged()? > > > > Do you mean include |node_bounds| in call to FocusedNodeChanged for the > > RenderViewObserver? > > I mean including the bounds in WebViewClient::focusedNodeChanged(). As we chatted, this will need to make change in blink code, I would like to get this feature in for R41. I will change blink code in a follow up cl. https://codereview.chromium.org/790413002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/magnification_manager.cc (right): https://codereview.chromium.org/790413002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/magnification_manager.cc:160: MonitorFocusInPageChange(); On 2014/12/15 18:37:17, sadrul wrote: > Shouldn't you always call this (i.e. outside of if after changing enabled_)? Since MagnificationController is not used for ui::MAGNIFIER_PARTIAL mode, the focus following feature is only enabled for ui::MAGNIFIER_FULL mode, therefore, I only want to call this in ui::MAGNIFIER_FULL.
jennyz@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@, would you please take a look at: content/common/view_messages.h Need owners approval, thanks!
ash lgtm
lgtm with some nits https://codereview.chromium.org/790413002/diff/100001/ash/magnifier/magnifica... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/790413002/diff/100001/ash/magnifier/magnifica... ash/magnifier/magnification_controller.cc:175: void MoveMaginifierWindowFollowRect(const gfx::Rect& rect); MoveMagnifierWindowFollowRect. https://codereview.chromium.org/790413002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc (right): https://codereview.chromium.org/790413002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc:119: DISALLOW_COPY_AND_ASSIGN(MagnificationControllerTest); Nit: usually this is in a private: section
https://codereview.chromium.org/790413002/diff/100001/ash/magnifier/magnifica... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/790413002/diff/100001/ash/magnifier/magnifica... ash/magnifier/magnification_controller.cc:175: void MoveMaginifierWindowFollowRect(const gfx::Rect& rect); On 2014/12/15 23:43:29, dcheng wrote: > MoveMagnifierWindowFollowRect. Done. https://codereview.chromium.org/790413002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc (right): https://codereview.chromium.org/790413002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/magnification_controller_browsertest.cc:119: DISALLOW_COPY_AND_ASSIGN(MagnificationControllerTest); On 2014/12/15 23:43:29, dcheng wrote: > Nit: usually this is in a private: section Done.
The CQ bit was checked by jennyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/790413002/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/399d6e66077c98e0a8cdf81c7bf7a027fd325d6e Cr-Commit-Position: refs/heads/master@{#308494} |
