Implement touch control

Implement touch control

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1982643004

Committed: https://skia.googlesource.com/skia/+/d3cdbcad65673596ae37e65fec842d8d4d81c5a7

[liyuqian, 2016-05-16 16:11:57]

Description was changed from

==========
Implement touch control

BUG=skia:
==========

to

==========
Implement touch control

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[liyuqian, 2016-05-16 16:12:32]

Description was changed from

==========
Implement touch control

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement touch control

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[liyuqian, 2016-05-16 16:12:32]

liyuqian@google.com changed reviewers:
+ djsollen@google.com, jvanverth@google.com

[liyuqian, 2016-05-16 16:22:05]

This is the first draft of the touch control. Please check if I handle the touch
events correctly and efficiently.

Note that I did NOT:

1. Make a copy of the SkTouchGesture. I haven't changed anything about the touch
gesture so I keep this CL clean without copying it. If we need to modify it in
the future, we can make a copy, or maybe change the original SkTouchGesture so
other projects can also benefit from our modifications.

2. Check the boundary so we won't move our matrix too far that the content
completely disappears in the screen. I'll implement this later. So far, I want
to make sure that I'm doing the right thing to handle the touch events.

[djsollen, 2016-05-16 18:16:00]

https://codereview.chromium.org/1982643004/diff/1/platform_tools/android/apps...
File
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerActivity.java
(right):

https://codereview.chromium.org/1982643004/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerActivity.java:106:
switch (action) {
why can't you do this in the native code where you do the other mappings?

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/Viewer.cpp#new...
tools/viewer/Viewer.cpp:262: updateMatrix();
comment why we need to update the matrix after we paint.

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/Window.h
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/Window....
tools/viewer/sk_app/Window.h:138: const SkMatrix& getGestureLocalM();
why can't this function be const as well?

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/android...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.cpp:232:
skiaAndroidApp->postMessage(message);
do you need to post a message for this or can you just update the SkTouchGesture
in your thread and trigger an inval if needed?

[liyuqian, 2016-05-16 18:32:38]

See the revision and comments below.

https://codereview.chromium.org/1982643004/diff/1/platform_tools/android/apps...
File
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerActivity.java
(right):

https://codereview.chromium.org/1982643004/diff/1/platform_tools/android/apps...
platform_tools/android/apps/viewer/src/main/java/org/skia/viewer/ViewerActivity.java:106:
switch (action) {
On 2016/05/16 18:16:00, djsollen wrote:
> why can't you do this in the native code where you do the other mappings?

Done. You're right. I just copied this piece of code and I should have changed
it :)

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/Viewer.cpp#new...
tools/viewer/Viewer.cpp:262: updateMatrix();
On 2016/05/16 18:16:00, djsollen wrote:
> comment why we need to update the matrix after we paint.

Done.

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/Window.h
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/Window....
tools/viewer/sk_app/Window.h:138: const SkMatrix& getGestureLocalM();
On 2016/05/16 18:16:00, djsollen wrote:
> why can't this function be const as well?

Because SkTouchGesture's globalM has const while its localM does not. (See
skia/include/views/SkTouchGesture.h)

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/android...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1982643004/diff/1/tools/viewer/sk_app/android...
tools/viewer/sk_app/android/surface_glue_android.cpp:232:
skiaAndroidApp->postMessage(message);
On 2016/05/16 18:16:00, djsollen wrote:
> do you need to post a message for this or can you just update the
SkTouchGesture
> in your thread and trigger an inval if needed?

I tend to defer as much work as possible to my own pthread. First, I don't want
to make UI thread do too much work which might make the app less responsive.
Second (more importantly), I don't want two threads (one UI thread, one pthread)
to concurrently modify some Skia properties/canvas, which may trigger race
conditions; putting everything into our message queue is the simplest and most
efficient way to guarantee that.

[jvanverth1, 2016-05-16 18:48:58]

The setup here looks consistent with its use in SampleApp, though I'm not an
expert on this gesture support.

[djsollen, 2016-05-16 19:09:03]

lgtm with nit

https://codereview.chromium.org/1982643004/diff/20001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/1982643004/diff/20001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:263: updateMatrix();
shouldn't this update happen as a reponse to touch input or an inval and not on
every paint?

[liyuqian, 2016-05-16 20:34:55]

updateMatrix is now computeMatrix.

I also added fContentJustInvalidated to avoid duplicate content invalidation.

https://codereview.chromium.org/1982643004/diff/20001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/1982643004/diff/20001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:263: updateMatrix();
On 2016/05/16 19:09:03, djsollen wrote:
> shouldn't this update happen as a reponse to touch input or an inval and not
on
> every paint?

Done.

[liyuqian, 2016-05-16 20:58:16]

Now the deduplication of invalidation events is simpler and more robust.

The fIsContentInvalided keeps track of whether invalidation events are already
in the queue. If they are, we will not push new invalidation events. This will
allow timed animation to issue invalidation events. (Our previous approach may
not work with timed animations.)

[djsollen, 2016-05-17 12:40:26]

https://codereview.chromium.org/1982643004/diff/60001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (left):

https://codereview.chromium.org/1982643004/diff/60001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:105: });
It seems more natural to me to implement the touch logic and SkTouchGesture
logic here.

https://codereview.chromium.org/1982643004/diff/60001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/Window.cpp (right):

https://codereview.chromium.org/1982643004/diff/60001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.cpp:55: bool Window::onTouch(int owner, InputState
state, float x, float y) {
seems odd to me that the window interprets touch as a gesture and that it isn't
the decision of each window how it wants to handle touch input.

[liyuqian, 2016-05-17 18:25:00]

I've now moved the SkTouchGesture inside viewer.cpp, and used the mutex to
guarantee atomic read/write of fIsContentInvalided.

[djsollen, 2016-05-17 18:48:03]

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/Viewer.cp...
tools/viewer/Viewer.cpp:33: Viewer* vv = reinterpret_cast<Viewer*>(userData);
Viewer* viewer =

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.cpp:92:
pthread_mutex_lock(&fMutex);
use SkAutoTAcquire

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.cpp:114:
pthread_mutex_lock(&skiaAndroidApp->fMutex);
use SkAutoTAcquire

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
File tools/viewer/sk_app/android/surface_glue_android.h (right):

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.h:68: pthread_mutex_t fMutex;
use SkMutex as it will handle the create and destroy steps for you

[liyuqian, 2016-05-17 19:06:35]

Changed to SkMutex

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/Viewer.cp...
tools/viewer/Viewer.cpp:33: Viewer* vv = reinterpret_cast<Viewer*>(userData);
On 2016/05/17 18:48:02, djsollen wrote:
> Viewer* viewer =

Done.

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
File tools/viewer/sk_app/android/surface_glue_android.cpp (right):

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.cpp:92:
pthread_mutex_lock(&fMutex);
On 2016/05/17 18:48:02, djsollen wrote:
> use SkAutoTAcquire

Done.

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.cpp:114:
pthread_mutex_lock(&skiaAndroidApp->fMutex);
On 2016/05/17 18:48:02, djsollen wrote:
> use SkAutoTAcquire

Done.

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
File tools/viewer/sk_app/android/surface_glue_android.h (right):

https://codereview.chromium.org/1982643004/diff/100001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.h:68: pthread_mutex_t fMutex;
On 2016/05/17 18:48:03, djsollen wrote:
> use SkMutex as it will handle the create and destroy steps for you

Done.

[djsollen, 2016-05-17 19:16:58]

lgtm

[liyuqian, 2016-05-17 19:18:16]

The CQ bit was checked by liyuqian@google.com

[commit-bot: I haz the power, 2016-05-17 19:18:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1982643004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1982643004/120001

[commit-bot: I haz the power, 2016-05-17 19:44:26]

Description was changed from

==========
Implement touch control

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement touch control

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/d3cdbcad65673596ae37e65fec842d8d4d81c5a7
==========

[commit-bot: I haz the power, 2016-05-17 19:44:30]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/d3cdbcad65673596ae37e65fec842d8d4d81c5a7