Adding persistance for gesture recognition parms.

Adding persistance for gesture recognition parms.
Observer updates GestureRecognition parameters when settings change.

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

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

[rjkroege, 2012-02-24 15:21:34]

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
File chrome/browser/gesture_prefs.cc (right):

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
chrome/browser/gesture_prefs.cc:16: #if defined(USE_AURA)
From my experience, this pattern is atypical. I would expect that it is
preferable to have a gesture_prefs.h, gesture_prefs_aura.h and
gesture_prefs_aura.cc with the second two included only in the aura built by gyp
configuration.

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
chrome/browser/gesture_prefs.cc:51: // The list of prefs we want to observe.
this would need to be updated for changes to the GR? Can you make a note here
noting the requirement to keep this in alignment with actual GR parameters.

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
chrome/browser/gesture_prefs.cc:62: void
GesturePrefs::RegisterPrefs(PrefService* ) {
how will the webui adjust the constants? Am I right to think that it should get
a local_state object and then call local_state->SetDouble('key', num)

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
File chrome/browser/gesture_prefs.h (right):

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
chrome/browser/gesture_prefs.h:16: class GesturePrefsHelper {
class GesturePrefs is in the .cc? and the helper is here? This doesn't seem
quite right.

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:97: #include
"chrome/browser/gesture_prefs.h"
this should not hang on its lonesome here but sort in above.

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:149: #if defined(USE_AURA)
you have coded it in the other place to be harmless here?

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_name...
chrome/common/pref_names.cc:1764: #if defined(USE_AURA)
the ordering convention of this file does not seem self-evident. you might have
to ask them. (but the consts need to be in alphabetical order.)

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_name...
chrome/common/pref_names.h:661: #if defined(USE_AURA)
they need to be in alphabetical order. my bad if that wasn't noted before.

[girard, 2012-02-28 23:48:05]

On 2012/02/24 15:21:34, rjkroege wrote:
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
> File chrome/browser/gesture_prefs.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
> chrome/browser/gesture_prefs.cc:16: #if defined(USE_AURA)
> From my experience, this pattern is atypical. I would expect that it is
> preferable to have a gesture_prefs.h, gesture_prefs_aura.h and
> gesture_prefs_aura.cc with the second two included only in the aura built by
gyp
> configuration.

Corrected.  The new patch contains gesture_prefs_aura.cc

>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
> chrome/browser/gesture_prefs.cc:51: // The list of prefs we want to observe.
> this would need to be updated for changes to the GR? Can you make a note here
> noting the requirement to keep this in alignment with actual GR parameters.
> 
So noted. 

>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
> chrome/browser/gesture_prefs.cc:62: void
> GesturePrefs::RegisterPrefs(PrefService* ) {
> how will the webui adjust the constants? Am I right to think that it should
get
> a local_state object and then call local_state->SetDouble('key', num)
> 
This is correct.  I have noted that usage in Issue 111993

>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
> File chrome/browser/gesture_prefs.h (right):
> 
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/gesture_...
> chrome/browser/gesture_prefs.h:16: class GesturePrefsHelper {
> class GesturePrefs is in the .cc? and the helper is here? This doesn't seem
> quite right.

Agreed.  I have renamed the GesturePrefs class to be called GesturePrefsHelper,
and it exists only in gesture_prefs_aura.cc.  The old GesturePrefsHelper is now
called GesturePrefs.
 
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/prefs/br...
> File chrome/browser/prefs/browser_prefs.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/prefs/br...
> chrome/browser/prefs/browser_prefs.cc:97: #include
> "chrome/browser/gesture_prefs.h"
> this should not hang on its lonesome here but sort in above.
 
Fixed.

>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/browser/prefs/br...
> chrome/browser/prefs/browser_prefs.cc:149: #if defined(USE_AURA)
> you have coded it in the other place to be harmless here?
> 
The ifdef is now removed, thanks to the gesture_prefs_aura.cc refactoring.

>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_name...
> File chrome/common/pref_names.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_name...
> chrome/common/pref_names.cc:1764: #if defined(USE_AURA)
> the ordering convention of this file does not seem self-evident. you might
have
> to ask them. (but the consts need to be in alphabetical order.)
> 
Corrected ordering.  (Doesn't look as nice, but it's alphabetical.)

>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_names.h
> File chrome/common/pref_names.h (right):
> 
>
https://chromiumcodereview.appspot.com/9460001/diff/1/chrome/common/pref_name...
> chrome/common/pref_names.h:661: #if defined(USE_AURA)
> they need to be in alphabetical order. my bad if that wasn't noted before.

[girard, 2012-02-28 23:52:01]

+scottmg since the current checkin aims to make the
gesture-recognizer-configuration logic behave nicely in a cross-platform
environment.

[rjkroege, 2012-02-29 15:45:35]

nit: class name change.  you'll need an owner too.

LGTM

https://chromiumcodereview.appspot.com/9460001/diff/6001/chrome/browser/gestu...
File chrome/browser/gesture_prefs_aura.cc (right):

https://chromiumcodereview.appspot.com/9460001/diff/6001/chrome/browser/gestu...
chrome/browser/gesture_prefs_aura.cc:18: class GesturePrefsHelper : public
content::NotificationObserver {
I'm thinking this is a GesturePrefsObserverAura. Am not sure what this implies
about the naming of this file.

https://chromiumcodereview.appspot.com/9460001/diff/6001/chrome/browser/gestu...
chrome/browser/gesture_prefs_aura.cc:109: void
GesturePrefs::RegisterPrefs(PrefService* prefs) {
this should become a global static function.

[girard, 2012-02-29 20:21:02]

+ben (owner)
Updated code as per rjkroege's review.
Renamed helper class to better reflect its actual function
GesturePrefsObserverAura.

[Ben Goodger (Google), 2012-02-29 20:49:16]

Can we put these new files into:

chrome/browser/ui/views/aura

?

[girard, 2012-02-29 21:20:20]

On 2012/02/29 20:49:16, Ben Goodger (Google) wrote:
> Can we put these new files into:
> 
> chrome/browser/ui/views/aura
> 
> ?

Excellent suggestion.  I'm assuming you meant for 
 chorme/browser/gesture_prefs_aura.cc 
to move to 
 chrome/browser/ui/views/aura/gesture_prefs_aura.cc
The other files (gesture_prefs.*) are featherweight stubs for cross-platform
functionality, so they would need to stay out of aura.

[Ben Goodger (Google), 2012-02-29 21:44:33]

k. at the very least those can move to browser/ui ?

-Ben

On Wed, Feb 29, 2012 at 1:20 PM, <girard@chromium.org> wrote:

> On 2012/02/29 20:49:16, Ben Goodger (Google) wrote:
>
>> Can we put these new files into:
>>
>
>  chrome/browser/ui/views/aura
>>
>
>  ?
>>
>
> Excellent suggestion.  I'm assuming you meant for
>  chorme/browser/gesture_prefs_**aura.cc
> to move to
>  chrome/browser/ui/views/aura/**gesture_prefs_aura.cc
> The other files (gesture_prefs.*) are featherweight stubs for
> cross-platform
> functionality, so they would need to stay out of aura.
>
>
https://chromiumcodereview.**appspot.com/9460001/<https://chromiumcodereview....
>

[girard, 2012-02-29 22:23:47]

Agreed.  I've moved the files as per Ben's suggestion.
Thanks

[Ben Goodger (Google), 2012-02-29 22:25:37]

lgtm

[commit-bot: I haz the power, 2012-02-29 22:26:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/9460001/14001

[commit-bot: I haz the power, 2012-03-01 03:15:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/9460001/19001

[commit-bot: I haz the power, 2012-03-01 03:15:34]

Can't apply patch for file chrome/chrome_browser.gypi.
While running patch -p1 --forward --force;
patching file chrome/chrome_browser.gypi
Hunk #1 succeeded at 3125 (offset 11 lines).
Hunk #2 succeeded at 3529 (offset 8 lines).
Hunk #3 FAILED at 4555.
Hunk #4 succeeded at 4604 (offset 4 lines).
1 out of 4 hunks FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej

[commit-bot: I haz the power, 2012-03-01 16:27:43]

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

[commit-bot: I haz the power, 2012-03-01 18:23:18]

Change committed as 124433

[commit-bot: I haz the power, 2012-03-07 16:48:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/9460001/47001

[commit-bot: I haz the power, 2012-03-07 16:48:05]

Can't apply patch for file chrome/chrome_browser.gypi.
While running patch -p1 --forward --force;
patching file chrome/chrome_browser.gypi
Hunk #1 FAILED at 1886.
Hunk #2 succeeded at 3142 (offset 2 lines).
Hunk #3 succeeded at 3554 (offset 2 lines).
Hunk #4 FAILED at 4594.
Hunk #5 succeeded at 4636 (offset 2 lines).
2 out of 5 hunks FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej

[commit-bot: I haz the power, 2012-03-07 16:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/9460001/52001

[commit-bot: I haz the power, 2012-03-07 18:59:53]

Change committed as 125421

[commit-bot: I haz the power, 2012-03-07 20:24:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/9460001/56001

[commit-bot: I haz the power, 2012-03-07 20:25:01]

Can't apply patch for file chrome/browser/prefs/browser_prefs.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/prefs/browser_prefs.cc
Hunk #1 FAILED at 52.
Hunk #2 succeeded at 232 with fuzz 2 (offset 72 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/prefs/browser_prefs.cc.rej