Add persisted preference for projection touch HUD

Add persisted preference for projection touch HUD

A new preference is created showing if the projection touch HUD is
enabled or not. This preference is per-user and unsyncable. It is not
exposed anywhere in the UI, but can be toggled using the Ctrl+Alt+9
accelerator combination.

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

[mohsen, 2013-07-02 16:06:06]

sadrul@: can you take a look, please...

[mohsen, 2013-07-03 00:34:11]

sky@: Please take a look...

[sadrul, 2013-07-03 00:57:53]

lgtm

[sky, 2013-07-03 13:47:08]

https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
chrome/browser/chromeos/preferences.cc:711:
ash::Shell::GetInstance()->TouchHudProjectionEnabledChanged(enabled);
Wouldn't all this be simpler if Shell had a
SetTouchHudProjectionEnabled(boolean); along with a delegate method
IsTouchUndProjectionEnabledChanged() ?

[mohsen, 2013-07-03 15:30:40]

https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
chrome/browser/chromeos/preferences.cc:711:
ash::Shell::GetInstance()->TouchHudProjectionEnabledChanged(enabled);
On 2013/07/03 13:47:08, sky wrote:
> Wouldn't all this be simpler if Shell had a
> SetTouchHudProjectionEnabled(boolean); along with a delegate method
> IsTouchUndProjectionEnabledChanged() ?

Do you mean a SetTouchHudProjectionEnabled(boolean) in the preference side to be
called from the Ash side (through ShellDelegate) whenever user toggles
projection HUD on and off; and an IsTouchHudProjectionEnabledChanged() in the
Ash side to be called from the preferences side to notify Ash of changes in
preference? Then, we also need an IsTouchHudProjectionEnabled() in the
preference side to be called from the Ash side to get the current value of the
preference. Isn't it similar to current implementation? Or I'm misunderstanding
you!

[sky, 2013-07-03 17:24:20]

I mean Shell::SetTouchHudProjectionEnabled(boolean) that calls
ShellObserver::IsTouchHudProjectionEnabledChange (if the value
changed). At startup chrome side invokes Shell::SetTouch... with the
right value. This way shell maintains the enabled state and chrome
persists the preference. Seems simpler than what you have now.

On Wed, Jul 3, 2013 at 8:30 AM,  <mohsen@chromium.org> wrote:
>
>
https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
> File chrome/browser/chromeos/preferences.cc (right):
>
>
https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
> chrome/browser/chromeos/preferences.cc:711:
> ash::Shell::GetInstance()->TouchHudProjectionEnabledChanged(enabled);
> On 2013/07/03 13:47:08, sky wrote:
>>
>> Wouldn't all this be simpler if Shell had a
>> SetTouchHudProjectionEnabled(boolean); along with a delegate method
>> IsTouchUndProjectionEnabledChanged() ?
>
>
> Do you mean a SetTouchHudProjectionEnabled(boolean) in the preference
> side to be called from the Ash side (through ShellDelegate) whenever
> user toggles projection HUD on and off; and an
> IsTouchHudProjectionEnabledChanged() in the Ash side to be called from
> the preferences side to notify Ash of changes in preference? Then, we
> also need an IsTouchHudProjectionEnabled() in the preference side to be
> called from the Ash side to get the current value of the preference.
> Isn't it similar to current implementation? Or I'm misunderstanding you!
>
> https://codereview.chromium.org/18163006/

[mohsen, 2013-07-03 18:49:06]

On 2013/07/03 17:24:20, sky wrote:
> I mean Shell::SetTouchHudProjectionEnabled(boolean) that calls
> ShellObserver::IsTouchHudProjectionEnabledChange (if the value
> changed). At startup chrome side invokes Shell::SetTouch... with the
> right value. This way shell maintains the enabled state and chrome
> persists the preference. Seems simpler than what you have now.
> 
> On Wed, Jul 3, 2013 at 8:30 AM,  <mailto:mohsen@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
> > File chrome/browser/chromeos/preferences.cc (right):
> >
> >
>
https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
> > chrome/browser/chromeos/preferences.cc:711:
> > ash::Shell::GetInstance()->TouchHudProjectionEnabledChanged(enabled);
> > On 2013/07/03 13:47:08, sky wrote:
> >>
> >> Wouldn't all this be simpler if Shell had a
> >> SetTouchHudProjectionEnabled(boolean); along with a delegate method
> >> IsTouchUndProjectionEnabledChanged() ?
> >
> >
> > Do you mean a SetTouchHudProjectionEnabled(boolean) in the preference
> > side to be called from the Ash side (through ShellDelegate) whenever
> > user toggles projection HUD on and off; and an
> > IsTouchHudProjectionEnabledChanged() in the Ash side to be called from
> > the preferences side to notify Ash of changes in preference? Then, we
> > also need an IsTouchHudProjectionEnabled() in the preference side to be
> > called from the Ash side to get the current value of the preference.
> > Isn't it similar to current implementation? Or I'm misunderstanding you!
> >
> > https://codereview.chromium.org/18163006/

So, chromeos::Preferences class can implement ShellObserver and observe the
shell to be notified when user toggles the projection HUD to persist the new
value? Also, RootWindowControllers can observe the shell and enable/disable
their projection HUD based on the new value of the preference maintained in
shell? Is that what you meant?

[sky, 2013-07-03 21:02:35]

On Wed, Jul 3, 2013 at 11:49 AM,  <mohsen@chromium.org> wrote:
> On 2013/07/03 17:24:20, sky wrote:
>>
>> I mean Shell::SetTouchHudProjectionEnabled(boolean) that calls
>> ShellObserver::IsTouchHudProjectionEnabledChange (if the value
>> changed). At startup chrome side invokes Shell::SetTouch... with the
>> right value. This way shell maintains the enabled state and chrome
>> persists the preference. Seems simpler than what you have now.
>
>
>> On Wed, Jul 3, 2013 at 8:30 AM,  <mailto:mohsen@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
>>
>> > File chrome/browser/chromeos/preferences.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/18163006/diff/1/chrome/browser/chromeos/prefe...
>>
>> > chrome/browser/chromeos/preferences.cc:711:
>> > ash::Shell::GetInstance()->TouchHudProjectionEnabledChanged(enabled);
>> > On 2013/07/03 13:47:08, sky wrote:
>> >>
>> >> Wouldn't all this be simpler if Shell had a
>> >> SetTouchHudProjectionEnabled(boolean); along with a delegate method
>> >> IsTouchUndProjectionEnabledChanged() ?
>> >
>> >
>> > Do you mean a SetTouchHudProjectionEnabled(boolean) in the preference
>> > side to be called from the Ash side (through ShellDelegate) whenever
>> > user toggles projection HUD on and off; and an
>> > IsTouchHudProjectionEnabledChanged() in the Ash side to be called from
>> > the preferences side to notify Ash of changes in preference? Then, we
>> > also need an IsTouchHudProjectionEnabled() in the preference side to be
>> > called from the Ash side to get the current value of the preference.
>> > Isn't it similar to current implementation? Or I'm misunderstanding you!
>> >
>> > https://codereview.chromium.org/18163006/
>
>
> So, chromeos::Preferences class can implement ShellObserver and observe the
> shell to be notified when user toggles the projection HUD to persist the new
> value?

Sure. Or if we already have a class implementing ShellObserver in
chrome some wheres.

>Also, RootWindowControllers can observe the shell and enable/disable
> their projection HUD based on the new value of the preference maintained in
> shell? Is that what you meant?

No preference in the shell, just a boolean member.

  -Scott

>
> https://codereview.chromium.org/18163006/

[mohsen, 2013-07-04 21:09:11]

sky@: I've implemented your proposed method. Please take a look...

[sky, 2013-07-08 14:06:35]

LGTM

[commit-bot: I haz the power, 2013-07-08 14:32:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/18163006/20001

[commit-bot: I haz the power, 2013-07-08 14:32:20]

Failed to apply patch for ash/root_window_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/root_window_controller.cc
  Hunk #1 FAILED at 201.
  Hunk #2 FAILED at 410.
  Hunk #3 succeeded at 514 with fuzz 1 (offset 44 lines).
  Hunk #4 succeeded at 702 (offset 33 lines).
  2 out of 4 hunks FAILED -- saving rejects to file
ash/root_window_controller.cc.rej

Patch:       ash/root_window_controller.cc
Index: ash/root_window_controller.cc
diff --git a/ash/root_window_controller.cc b/ash/root_window_controller.cc
index
b765fff9c3e96e08fef3e7280f5e7fb343d140b6..9570ad7cf89f8bdf975fa43c10902730746308bf
100644
--- a/ash/root_window_controller.cc
+++ b/ash/root_window_controller.cc
@@ -201,6 +201,8 @@ RootWindowController*
RootWindowController::ForActiveRootWindow() {
 }
 
 void RootWindowController::Shutdown() {
+  Shell::GetInstance()->RemoveShellObserver(this);
+
   CloseChildWindows();
   if (Shell::GetActiveRootWindow() == root_window_) {
     Shell::GetInstance()->set_active_root_window(
@@ -410,18 +412,6 @@ void RootWindowController::MoveWindowsTo(aura::RootWindow*
dst) {
   ReparentAllWindows(root_window_.get(), dst);
 }
 
-void RootWindowController::EnableTouchHudProjection() {
-  if (touch_hud_projection_)
-    return;
-  set_touch_hud_projection(new TouchHudProjection(root_window_.get()));
-}
-
-void RootWindowController::DisableTouchHudProjection() {
-  if (!touch_hud_projection_)
-    return;
-  touch_hud_projection_->Remove();
-}
-
 ShelfLayoutManager* RootWindowController::GetShelfLayoutManager() {
   return shelf_.get() ? shelf_->shelf_layout_manager() : NULL;
 }
@@ -470,6 +460,8 @@ void RootWindowController::InitTouchHuds() {
     set_touch_hud_debug(new TouchHudDebug(root_window_.get()));
   if (Shell::GetInstance()->is_touch_hud_projection_enabled())
     EnableTouchHudProjection();
+
+  Shell::GetInstance()->AddShellObserver(this);
 }
 
 aura::Window* RootWindowController::GetFullscreenWindow() const {
@@ -667,5 +659,24 @@ void RootWindowController::CreateContainersInRootWindow(
                   "PowerButtonAnimationContainer", root_window) ;
 }
 
+void RootWindowController::EnableTouchHudProjection() {
+  if (touch_hud_projection_)
+    return;
+  set_touch_hud_projection(new TouchHudProjection(root_window_.get()));
+}
+
+void RootWindowController::DisableTouchHudProjection() {
+  if (!touch_hud_projection_)
+    return;
+  touch_hud_projection_->Remove();
+}
+
+void RootWindowController::OnTouchHudProjectionToggled(bool enabled) {
+  if (enabled)
+    EnableTouchHudProjection();
+  else
+    DisableTouchHudProjection();
+}
+
 }  // namespace internal
 }  // namespace ash

[commit-bot: I haz the power, 2013-07-08 15:41:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/18163006/33001

[commit-bot: I haz the power, 2013-07-08 16:07:30]

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

[mohsen, 2013-07-08 23:13:36]

sky@: I've changed the code a bit (mainly to satisfy clang compiler). Please
take another look.

[sky, 2013-07-09 00:21:37]

LGTM

[commit-bot: I haz the power, 2013-07-09 02:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/18163006/33003

[commit-bot: I haz the power, 2013-07-09 08:13:20]

Change committed as 210516