[Android] Offset smart clip rect output when there is a change in viewport

[Android] Offset smart clip rect output when there is a change in viewport

Smart clip rect output needs to be offsetted when there is a change in
viewport, e.g., when location bar is shown.

BUG=414597
TEST=verified by OEM that this fixes the bug

Committed: https://crrev.com/02a50d7b66b351ea282bb8374c980a2d000d17f9
Cr-Commit-Position: refs/heads/master@{#296154}

[Changwan Ryu, 2014-09-19 02:30:28]

changwan@chromium.org changed reviewers:
+ jdduke@chromium.org

[Changwan Ryu, 2014-09-19 02:33:14]

changwan@chromium.org changed reviewers:
+ tedchoc@chromium.org

[jdduke (slow), 2014-09-19 17:38:37]

jdduke@chromium.org changed reviewers:
+ yfriedmanc@chromium.org

[jdduke (slow), 2014-09-19 17:38:37]

https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2825:
mSmartClipDataListener.onSmartClipDataExtracted(text, html, clipRect);
This seems reasonable, though I haven't reviewed any of the smart clip code
before. Was that yfriedman@?

[jdduke (slow), 2014-09-19 17:39:15]

jdduke@chromium.org changed reviewers:
+ yfriedman@chromium.org
- yfriedmanc@chromium.org

[jdduke (slow), 2014-09-19 17:41:26]

https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2825:
mSmartClipDataListener.onSmartClipDataExtracted(text, html, clipRect);
On 2014/09/19 17:38:37, jdduke wrote:
> This seems reasonable, though I haven't reviewed any of the smart clip code
> before. Was that yfriedman@?

I dunno, this seems like it could be racy but I haven't reviewed any smartclip
patches (beyond just merging them =/), and it's possible the downstream code
does the right thing when setting the offsets. I'll defer to tedchoc@ here.

[Ted C, 2014-09-19 22:23:06]

https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2825:
mSmartClipDataListener.onSmartClipDataExtracted(text, html, clipRect);
On 2014/09/19 17:41:26, jdduke wrote:
> On 2014/09/19 17:38:37, jdduke wrote:
> > This seems reasonable, though I haven't reviewed any of the smart clip code
> > before. Was that yfriedman@?
> 
> I dunno, this seems like it could be racy but I haven't reviewed any smartclip
> patches (beyond just merging them =/), and it's possible the downstream code
> does the right thing when setting the offsets. I'll defer to tedchoc@ here.

I can't say that I know what to expect here either.  Assuming this is async,
which based on the method signature it appears to be, this definitely has the
potential to be several frames behind in terms of those offsets.

Ideally, you'd want to send those offsets as separate params so that you can
correctly negate them when returning the values (or wrap the whole process in a
callback that can isolate the single request).

If this ok to be right"ish", then we should at least document that these offsets
can be different and we accept when they are wrong.

[Changwan Ryu, 2014-09-22 13:29:19]

On 2014/09/19 22:23:06, Ted C wrote:
>
https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/565383005/diff/20001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2825:
> mSmartClipDataListener.onSmartClipDataExtracted(text, html, clipRect);
> On 2014/09/19 17:41:26, jdduke wrote:
> > On 2014/09/19 17:38:37, jdduke wrote:
> > > This seems reasonable, though I haven't reviewed any of the smart clip
code
> > > before. Was that yfriedman@?
> > 
> > I dunno, this seems like it could be racy but I haven't reviewed any
smartclip
> > patches (beyond just merging them =/), and it's possible the downstream code
> > does the right thing when setting the offsets. I'll defer to tedchoc@ here.
> 
> I can't say that I know what to expect here either.  Assuming this is async,
> which based on the method signature it appears to be, this definitely has the
> potential to be several frames behind in terms of those offsets.
> 
> Ideally, you'd want to send those offsets as separate params so that you can
> correctly negate them when returning the values (or wrap the whole process in
a
> callback that can isolate the single request).
> 
> If this ok to be right"ish", then we should at least document that these
offsets
> can be different and we accept when they are wrong.

Jared, thanks for adding Ted and Yaron as reviewers!
Ted, I just added the necessary comment. I totally agree that offsets should be
passed as separate params, but I'm afraid that blink side changes could only be
applied
after doing some major refactoring (redesign) around smart clip (Issue 416432)
is resolved. I've added this as a TODO as well.

In the meantime, I've asked OEM to test further to see if this can really be a
problem or not.

[Ted C, 2014-09-22 15:23:51]

lgtm

https://codereview.chromium.org/565383005/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/565383005/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2822:
// as long as OEM has a UI that consumes all the inputs and waits until the
Sadly these offsets can be driven not only by user input, but also by the
renderer animating off the top controls.

In the end, this is probably "close-ish" and slightly more correct than it was
previously, but it definitely is not a fool proof method.

[Changwan Ryu, 2014-09-23 06:46:42]

On 2014/09/22 15:23:51, Ted C wrote:
> lgtm
> 
>
https://codereview.chromium.org/565383005/diff/60001/content/public/android/j...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
> (right):
> 
>
https://codereview.chromium.org/565383005/diff/60001/content/public/android/j...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2822:
> // as long as OEM has a UI that consumes all the inputs and waits until the
> Sadly these offsets can be driven not only by user input, but also by the
> renderer animating off the top controls.
> 
> In the end, this is probably "close-ish" and slightly more correct than it was
> previously, but it definitely is not a fool proof method.

The OEM confirmed that the timing issue is hard to reproduce.
Let me merge this for M38, and follow up on crbug.com/416758 to use correct
offset values.

Thanks for the reviews.

[Changwan Ryu, 2014-09-23 06:47:19]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2014-09-23 06:48:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/565383005/60001

[commit-bot: I haz the power, 2014-09-23 07:35:53]

Committed patchset #4 (id:60001) as ef5f419f26d4850f3cd228fb8c01623847e27f7e

[commit-bot: I haz the power, 2014-09-23 07:36:28]

Patchset 4 (id:??) landed as
https://crrev.com/02a50d7b66b351ea282bb8374c980a2d000d17f9
Cr-Commit-Position: refs/heads/master@{#296154}