Implementation of the Touch Exploration Mode - Part II (ash)

Implementation of the Touch Exploration Mode - Part II (ash)

The ash/ part of the implementation of the Touch Exploration Mode. This is dependent on Part I:
https://codereview.chromium.org/262483003/

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

[mfomitchev, 2014-05-01 20:48:02]

Sadrul, can you please review? Thanks!

[mfomitchev, 2014-05-02 15:19:32]

+@oshima

[sadrul, 2014-05-02 15:47:23]

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.cc:349: switches::kAshEnableTouchExplorationMode)) {
You can probably just check if touch_exploration_controller_ exists, instead of
looking at the flag here?

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:22: #include
"ash/test/root_window_controller_test_api_chromeos.h"
You should not include from ash/test/

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:91: public AccessibilityObserver,
Instead of making the RootWindowController an AccessibilityObserver, could we
have a chromeos-only implementation of AccessiblityObserver (e.g.
CrosAccessiblityObserver, which toggles the touch-exploration mode on/off) that
the RootWindowController owns? We would have less #ifdef with that.

https://codereview.chromium.org/261863002/diff/30001/ash/test/root_window_con...
File ash/test/root_window_controller_test_api_chromeos.h (right):

https://codereview.chromium.org/261863002/diff/30001/ash/test/root_window_con...
ash/test/root_window_controller_test_api_chromeos.h:17:
RootWindowControllerTestApi(RootWindowController* root_window_controller);
explicit

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:57:
GURL("chrome://version"),
This still looks bad. We have a lot of browser-tests that load a data-url and it
works. I would encourage you to look at them and see what they are doing
differently.

[mfomitchev, 2014-05-02 17:33:57]

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:57:
GURL("chrome://version"),
Navigating to a URL is just a hack to have the resize complete. Navhgation
itself completes fine, but sometimes the resize comes after the navigation
unless I also do a WaitForCommit().

I think a better way to do this would be to add an empty PRE_ test. If there's a
PRE_ test, the resize happens during the PRE_ test. It's still a hack, but it's
a cleaner hack. Does this sound okay?

[dmazzoni, 2014-05-02 18:28:29]

lgtm

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:91: public AccessibilityObserver,
On 2014/05/02 15:47:24, sadrul wrote:
> Instead of making the RootWindowController an AccessibilityObserver, could we
> have a chromeos-only implementation of AccessiblityObserver (e.g.
> CrosAccessiblityObserver, which toggles the touch-exploration mode on/off)
that
> the RootWindowController owns? We would have less #ifdef with that.

Yeah, that makes sense. Actually it's not clear why any of this has to live in
RootWindowController - the only thing it uses is GetRootWindow. A little helper
class could do everything.

I'd call the class TouchExplorationController or something like that, and have
it implement AccessiblityObserver.

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:57:
GURL("chrome://version"),
On 2014/05/02 17:33:57, mfomitchev wrote:
> Navigating to a URL is just a hack to have the resize complete. Navhgation
> itself completes fine, but sometimes the resize comes after the navigation
> unless I also do a WaitForCommit().

Does the window size actually change? Could you just set the initial window size
to zero and then spin until the window size is updated or something like that?

[mfomitchev, 2014-05-02 19:22:52]

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.cc:349: switches::kAshEnableTouchExplorationMode)) {
On 2014/05/02 15:47:24, sadrul wrote:
> You can probably just check if touch_exploration_controller_ exists, instead
of
> looking at the flag here?

Done.

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:22: #include
"ash/test/root_window_controller_test_api_chromeos.h"
Oops! Actually we don't even need this test API anymore, since we are no longer
using the RootWindowController in the unit test, so I will just get rid of
ash::test::RootWindowControllerTestApi.

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:91: public AccessibilityObserver,
@dmazzoni:
Somebody has to own TouchExplorationController. I chose RootWindowController
because it owns some similar objects, e.g. touch_hud_projection_,
touch_hud_debug_. If you can think of a better owner - let me know.

https://codereview.chromium.org/261863002/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:91: public AccessibilityObserver,
@sadrul

Done.

https://codereview.chromium.org/261863002/diff/30001/ash/test/root_window_con...
File ash/test/root_window_controller_test_api_chromeos.h (right):

https://codereview.chromium.org/261863002/diff/30001/ash/test/root_window_con...
ash/test/root_window_controller_test_api_chromeos.h:17:
RootWindowControllerTestApi(RootWindowController* root_window_controller);
Got rid of this class

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/261863002/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:57:
GURL("chrome://version"),
Yes, it does. I am not sure why - sadrul@ seems to think it shouldn't so it
needs more investigation. I am going to come back to this after I come back from
my vacation. It's even possible I am just synced to a bad revision and syncing
to tip of tree will solve this :)
Once we understand what exactly is going on, I think we'll be in a better
position to come up with the best solution. Adding a PRE_ test I hope is okay
until then.

[dmazzoni, 2014-05-02 19:29:22]

still lgtm

https://codereview.chromium.org/261863002/diff/40001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/40001/ash/root_window_controll...
ash/root_window_controller.cc:267: // feedback is on.
nit: no indent here

https://codereview.chromium.org/261863002/diff/40001/ash/root_window_controll...
ash/root_window_controller.cc:268: class CrosAccessibilityObserver : public
AccessibilityObserver {
This is a lot cleaner. Looks good.

[mfomitchev, 2014-05-02 19:40:58]

https://codereview.chromium.org/261863002/diff/40001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/40001/ash/root_window_controll...
ash/root_window_controller.cc:267: // feedback is on.
On 2014/05/02 19:29:23, dmazzoni wrote:
> nit: no indent here

Done.

https://codereview.chromium.org/261863002/diff/40001/ash/root_window_controll...
ash/root_window_controller.cc:268: class CrosAccessibilityObserver : public
AccessibilityObserver {
Agreed!

[oshima, 2014-05-02 20:04:23]

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:268: class CrosAccessibilityObserver : public
AccessibilityObserver {
please move this to anonymous namespace above

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:283: touch_exploration_controller_.reset();
you don't need this.

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:296: } else if (!enabled &&
touch_exploration_controller_.get()) {
you can skip checking _.get() and always reset().

https://codereview.chromium.org/261863002/diff/60001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/261863002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:26:
ash::switches::kAshEnableTouchExplorationMode);
Can you do this in SetUpCommandLine?

[mfomitchev, 2014-05-02 20:46:32]

Q on one comment. I will address the rest of the feedback.

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:268: class CrosAccessibilityObserver : public
AccessibilityObserver {
It's used in .h file and I get a 'fatal' warning if I try to move this to
anonymous namespace:

../../ash/root_window_controller.h:83:18: error: ‘ash::RootWindowController’ has
a field
 ‘ash::RootWindowController::cros_accessibility_observer_’ whose type uses the
anonymous
 namespace [-Werror]

[mfomitchev, 2014-05-02 21:00:59]

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:283: touch_exploration_controller_.reset();
On 2014/05/02 20:04:24, oshima wrote:
> you don't need this.

Done.

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:296: } else if (!enabled &&
touch_exploration_controller_.get()) {
On 2014/05/02 20:04:24, oshima wrote:
> you can skip checking _.get() and always reset().

Done.

https://codereview.chromium.org/261863002/diff/60001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/261863002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:26:
ash::switches::kAshEnableTouchExplorationMode);
On 2014/05/02 20:04:24, oshima wrote:
> Can you do this in SetUpCommandLine?

Done.

[oshima, 2014-05-02 21:25:45]

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
ash/root_window_controller.cc:268: class CrosAccessibilityObserver : public
AccessibilityObserver {
On 2014/05/02 20:46:33, mfomitchev wrote:
> It's used in .h file and I get a 'fatal' warning if I try to move this to
> anonymous namespace:
> 
> ../../ash/root_window_controller.h:83:18: error: ‘ash::RootWindowController’
has
> a field
>  ‘ash::RootWindowController::cros_accessibility_observer_’ whose type uses the
> anonymous
>  namespace [-Werror] 

Can you use AccessibilityObserver in the header instead? This way, you can avoid
leaking the internal class name in the haeder.

[mfomitchev, 2014-05-02 21:34:49]

On 2014/05/02 21:25:45, oshima wrote:
>
https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
> File ash/root_window_controller.cc (right):
> 
>
https://codereview.chromium.org/261863002/diff/60001/ash/root_window_controll...
> ash/root_window_controller.cc:268: class CrosAccessibilityObserver : public
> AccessibilityObserver {
> On 2014/05/02 20:46:33, mfomitchev wrote:
> > It's used in .h file and I get a 'fatal' warning if I try to move this to
> > anonymous namespace:
> > 
> > ../../ash/root_window_controller.h:83:18: error: ‘ash::RootWindowController’
> has
> > a field
> >  ‘ash::RootWindowController::cros_accessibility_observer_’ whose type uses
the
> > anonymous
> >  namespace [-Werror] 
> 
> Can you use AccessibilityObserver in the header instead? This way, you can
avoid
> leaking the internal class name in the haeder.

Ah, good point,! Done

[oshima, 2014-05-02 22:16:36]

lgtm

[Rick Byers, 2014-05-05 18:38:46]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-05-05 18:40:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/261863002/100001

[commit-bot: I haz the power, 2014-05-05 22:05:05]

Change committed as 268291