MD Settings: Pointers: Avoid setting mouse swap pref while mouse pressed

MD Settings: Pointers: Avoid setting mouse swap pref while mouse pressed

BUG=686949
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2753353002
Cr-Commit-Position: refs/heads/master@{#459555}
Committed: https://chromium.googlesource.com/chromium/src/+/9b5e6ff303ed188622c81186bb86fe37e60ebb67

[stevenjb, 2017-03-17 22:08:44]

Description was changed from

==========
MD Settings: Pointers: Avoid setting mouse swap pref while mouse pressed

BUG=686949
==========

to

==========
MD Settings: Pointers: Avoid setting mouse swap pref while mouse pressed

BUG=686949
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2017-03-17 22:09:10]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2017-03-17 22:53:26]

https://codereview.chromium.org/2753353002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/pointers.js (right):

https://codereview.chromium.org/2753353002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/pointers.js:62:
this.receivedMouseSwapButtonsUp_ = false;
Tracking two booleans is confusing, and the "why" isn't really clear.

Would only tracking mouseDown achieve the same thing? (This simplifies the
process a bit by not tracking whether the state has "changed" -- setting the
pref value to its current value should have no effect.)

onDown_() {
  this.mouseDown_ = true;
},

onUp_() {
  this.mouseDown_ = false;
  this.$.mouseSwapButton.sendPrefChange();
},

onChange_() {
  if (!this.mouseDown_)
    this.$.mouseSwapButton.sendPrefChange();
},

[michaelpg, 2017-03-17 23:09:14]

https://codereview.chromium.org/2753353002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/pointers.js (right):

https://codereview.chromium.org/2753353002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/pointers.js:62:
this.receivedMouseSwapButtonsUp_ = false;
On 2017/03/17 22:53:26, michaelpg wrote:
> Tracking two booleans is confusing, and the "why" isn't really clear.
> 
> Would only tracking mouseDown achieve the same thing? (This simplifies the
> process a bit by not tracking whether the state has "changed" -- setting the
> pref value to its current value should have no effect.)
> 
> onDown_() {
>   this.mouseDown_ = true;
> },
> 
> onUp_() {
>   this.mouseDown_ = false;
>   this.$.mouseSwapButton.sendPrefChange();
> },
> 
> onChange_() {
>   if (!this.mouseDown_)
>     this.$.mouseSwapButton.sendPrefChange();
> },

So as you said offline, keeping both booleans avoids the redundant
sendPrefChange call. If we keep them, could we rename them, e.g.
isMouseDownOnSwapToggle_ and hasSentSwapTogglePrefChange_ -- tracking the state
of things, rather than tracking which raw events we've received?

[stevenjb, 2017-03-17 23:13:12]

Yeah, that's simpler. It calls 'set' twice, but I think the simpler code is
better.

[michaelpg, 2017-03-17 23:34:41]

lgtm w/ console logging removed

https://codereview.chromium.org/2753353002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/pointers.js (right):

https://codereview.chromium.org/2753353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/pointers.js:67:
console.info('change');
remove

https://codereview.chromium.org/2753353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/pointers.js:75:
console.info('change');
remove

[stevenjb, 2017-03-24 19:32:25]

https://codereview.chromium.org/2753353002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/pointers.js (right):

https://codereview.chromium.org/2753353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/pointers.js:67:
console.info('change');
On 2017/03/17 23:34:41, michaelpg wrote:
> remove

Doh. Thanks.

https://codereview.chromium.org/2753353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/pointers.js:75:
console.info('change');
On 2017/03/17 23:34:41, michaelpg wrote:
> remove

Done.

[stevenjb, 2017-03-24 19:32:27]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2017-03-24 19:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2753353002/#ps40001
(title: "Elim console.log")

[commit-bot: I haz the power, 2017-03-24 19:33:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 21:19:46]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490383947797560,
"parent_rev": "e5d0b0e21128dafea994f88487e49f1c43fe4550", "commit_rev":
"9b5e6ff303ed188622c81186bb86fe37e60ebb67"}

[commit-bot: I haz the power, 2017-03-24 21:20:38]

Description was changed from

==========
MD Settings: Pointers: Avoid setting mouse swap pref while mouse pressed

BUG=686949
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Pointers: Avoid setting mouse swap pref while mouse pressed

BUG=686949
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2753353002
Cr-Commit-Position: refs/heads/master@{#459555}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b5e6ff303ed188622c81186bb86...
==========

[commit-bot: I haz the power, 2017-03-24 21:20:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9b5e6ff303ed188622c81186bb86...