[Workaround] Notify Overscroll Actions that a gesture ended.

[Workaround] Notify Overscroll Actions that a gesture ended.

The OverscrollActionController adds a UIPanGestureRecognizer.
When text is selected and SelectionSpeak is enabled, the
UIPanGestureRecognizer's end state is not always communicated to the
OverscrollActionController.

This CL calls the OverscrollActionController whenever the
UIPanGestureRecognizer is reset.

patchset from jif@chromium.org

TEST=Enable SelectionSpeak. Visit a page. Select text. In a single gesture, scroll a
little bit up and down. Once you lift your finger from the screen, the browser
should not be frozen, i.e. you should be able to scroll the page.
BUG=699655
patch from issue 2787723003 at patchset 40001 (http://crrev.com/2787723003#ps40001)

Review-Url: https://codereview.chromium.org/2817483002
Cr-Commit-Position: refs/heads/master@{#464267}
Committed: https://chromium.googlesource.com/chromium/src/+/998bf426e310eda213fa80898a840a89f815d676

[justincohen, 2017-04-11 18:24:12]

justincohen@chromium.org changed reviewers:
+ jif@chromium.org, noyau@chromium.org, rohitrao@chromium.org

[justincohen, 2017-04-11 18:24:13]

Patch stolen from jif@ while he's out here:
https://codereview.chromium.org/2787723003/

Only change is sorting headers and removing the call to removeTarget.  I'm not
sure why it's added.  See:
https://codereview.chromium.org/2787723003/diff/40001/ios/chrome/browser/ui/o...

Perhaps for completeness?  It's not wrong.  But it's never called either..

WDYT?

[noyau (Ping after 24h), 2017-04-12 09:01:53]

On 2017/04/11 18:24:13, justincohen wrote:
> Patch stolen from jif@ while he's out here:
> https://codereview.chromium.org/2787723003/
> 
> Only change is sorting headers and removing the call to removeTarget.  I'm not
> sure why it's added.  See:
>
https://codereview.chromium.org/2787723003/diff/40001/ios/chrome/browser/ui/o...
> 
> Perhaps for completeness?  It's not wrong.  But it's never called either..
> 
> WDYT?

One could add and remove targets with -addTarget:action and
-removeTarget:action:, it's part of the API of the supersuperclass. I can see
the desire for completeness, in witch case both should be implemented properly.
In our case we don't need them, but adding just a DCHECK to make sure those two
methods are never called on this subclass instead is fine by me.

Also the superclass already retains the target, the instance variable could
probably be changed to a weak reference.

[jif-google, 2017-04-12 12:06:52]

On 2017/04/12 09:01:53, noyau wrote:
> On 2017/04/11 18:24:13, justincohen wrote:
> > Patch stolen from jif@ while he's out here:
> > https://codereview.chromium.org/2787723003/
> > 
> > Only change is sorting headers and removing the call to removeTarget.  I'm
not
> > sure why it's added.  See:
> >
>
https://codereview.chromium.org/2787723003/diff/40001/ios/chrome/browser/ui/o...
> > 
> > Perhaps for completeness?  It's not wrong.  But it's never called either..
> > 
> > WDYT?
> 
> One could add and remove targets with -addTarget:action and
> -removeTarget:action:, it's part of the API of the supersuperclass. I can see
> the desire for completeness, in witch case both should be implemented
properly.
> In our case we don't need them, but adding just a DCHECK to make sure those
two
> methods are never called on this subclass instead is fine by me.
> 
> Also the superclass already retains the target, the instance variable could
> probably be changed to a weak reference.

Regarding the |removeTarget:|:
I wanted to make sure that if we started to use |removeTarget:| with the target
passed in the init, the OverscrollActionsGestureRecognizer would still work as
expected.
I agree with noyau that we could just add a DCHECK:
 -(void)removeTarget:(id)target action:(SEL)action {
    DCHECK(target != target_) {
   [super removeTarget:target action:action];
 }

[rohitrao (ping after 24h), 2017-04-12 12:25:57]

lgtm

Note that Justin does not believe that this should go into the initial M58
release.

https://codereview.chromium.org/2817483002/diff/1/ios/chrome/browser/ui/overs...
File
ios/chrome/browser/ui/overscroll_actions/overscroll_actions_gesture_recognizer.mm
(right):

https://codereview.chromium.org/2817483002/diff/1/ios/chrome/browser/ui/overs...
ios/chrome/browser/ui/overscroll_actions/overscroll_actions_gesture_recognizer.mm:12:
base::scoped_nsprotocol<id> target_;
This might be better as a weak pointer.

https://codereview.chromium.org/2817483002/diff/1/ios/chrome/browser/ui/overs...
ios/chrome/browser/ui/overscroll_actions/overscroll_actions_gesture_recognizer.mm:30:
[target_ performSelector:action_ withObject:self];
Does the order of these calls matter?  Is it ok to call the action after
resetting the superclass?

[justincohen, 2017-04-12 16:04:28]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 16:05:21]

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

[justincohen, 2017-04-12 16:05:34]

PTAL

https://codereview.chromium.org/2817483002/diff/1/ios/chrome/browser/ui/overs...
File
ios/chrome/browser/ui/overscroll_actions/overscroll_actions_gesture_recognizer.mm
(right):

https://codereview.chromium.org/2817483002/diff/1/ios/chrome/browser/ui/overs...
ios/chrome/browser/ui/overscroll_actions/overscroll_actions_gesture_recognizer.mm:12:
base::scoped_nsprotocol<id> target_;
On 2017/04/12 12:25:57, rohitrao wrote:
> This might be better as a weak pointer.

Done.

https://codereview.chromium.org/2817483002/diff/1/ios/chrome/browser/ui/overs...
ios/chrome/browser/ui/overscroll_actions/overscroll_actions_gesture_recognizer.mm:30:
[target_ performSelector:action_ withObject:self];
On 2017/04/12 12:25:57, rohitrao wrote:
> Does the order of these calls matter?  Is it ok to call the action after
> resetting the superclass?

It shouldn't matter, as long as the panGesture: call's with the right state
(which is correct at this point)

[commit-bot: I haz the power, 2017-04-12 17:23:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 17:23:58]

Dry run: This issue passed the CQ dry run.

[justincohen, 2017-04-12 21:29:02]

The CQ bit was checked by justincohen@chromium.org

[justincohen, 2017-04-12 21:29:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2817483002/#ps20001
(title: "Add back removeTarget, use weak protocol")

[commit-bot: I haz the power, 2017-04-12 21:30:20]

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-04-13 03:04:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492032542361680,
"parent_rev": "ce787054f9f35cb312ccc5853cea4a6b04aa0dff", "commit_rev":
"998bf426e310eda213fa80898a840a89f815d676"}

[commit-bot: I haz the power, 2017-04-13 03:09:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492032542361680,
"parent_rev": "ce787054f9f35cb312ccc5853cea4a6b04aa0dff", "commit_rev":
"998bf426e310eda213fa80898a840a89f815d676"}

[commit-bot: I haz the power, 2017-04-13 03:09:21]

Description was changed from

==========
[Workaround] Notify Overscroll Actions that a gesture ended.

The OverscrollActionController adds a UIPanGestureRecognizer.
When text is selected and SelectionSpeak is enabled, the
UIPanGestureRecognizer's end state is not always communicated to the
OverscrollActionController.

This CL calls the OverscrollActionController whenever the
UIPanGestureRecognizer is reset.

patchset from jif@chromium.org

TEST=Enable SelectionSpeak. Visit a page. Select text. In a single gesture,
scroll a
little bit up and down. Once you lift your finger from the screen, the browser
should not be frozen, i.e. you should be able to scroll the page.
BUG=699655

patch from issue 2787723003 at patchset 40001
(http://crrev.com/2787723003#ps40001)
==========

to

==========
[Workaround] Notify Overscroll Actions that a gesture ended.

The OverscrollActionController adds a UIPanGestureRecognizer.
When text is selected and SelectionSpeak is enabled, the
UIPanGestureRecognizer's end state is not always communicated to the
OverscrollActionController.

This CL calls the OverscrollActionController whenever the
UIPanGestureRecognizer is reset.

patchset from jif@chromium.org

TEST=Enable SelectionSpeak. Visit a page. Select text. In a single gesture,
scroll a
little bit up and down. Once you lift your finger from the screen, the browser
should not be frozen, i.e. you should be able to scroll the page.
BUG=699655

patch from issue 2787723003 at patchset 40001
(http://crrev.com/2787723003#ps40001)

Review-Url: https://codereview.chromium.org/2817483002
Cr-Commit-Position: refs/heads/master@{#464267}
Committed:
https://chromium.googlesource.com/chromium/src/+/998bf426e310eda213fa80898a84...
==========

[commit-bot: I haz the power, 2017-04-13 03:09:22]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/998bf426e310eda213fa80898a84...