Move inval dedup to Window for wider usages.

Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2001153002

Committed: https://skia.googlesource.com/skia/+/566c8e4a36c51cb24d8bb97cc71b3eb26737c012

[liyuqian, 2016-05-23 14:13:50]

Description was changed from

==========
Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

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

to

==========
Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2001153002
==========

[liyuqian, 2016-05-23 14:15:49]

Description was changed from

==========
Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2001153002
==========

to

==========
Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2001153002
==========

[liyuqian, 2016-05-23 14:15:50]

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

[jvanverth1, 2016-05-23 14:22:32]

Just one comment:

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

https://codereview.chromium.org/2001153002/diff/1/tools/viewer/sk_app/Window....
tools/viewer/sk_app/Window.cpp:66: SkAutoMutexAcquire ama(fMutex);
How long does the mutex take? On single-threaded systems we'd want to run as
fast as possible. Maybe place in SKIA_BUILD_ANDROID block?

[jvanverth1, 2016-05-23 14:36:13]

lgtm with removal of mutex

[liyuqian, 2016-05-23 14:40:36]

Mutex removed. Please double check it and give me the second LGTM before I
commit to the repo :)

[djsollen, 2016-05-23 14:44:26]

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

https://codereview.chromium.org/2001153002/diff/1/tools/viewer/sk_app/Window....
tools/viewer/sk_app/Window.cpp:67: fIsContentInvalidated = false;
putting this here won't work on Android as the inval message calls
paintIfNeeded() which checks the status of the backing window before getting
here.  So you could end up in a situation like...

1) post inval message
2) receive inval but don't call onPaint b/c the surface is not ready
3) all subsequent attempts to inval will be ignored
4) nothing ever draws. :(

[liyuqian, 2016-05-23 14:58:24]

Good catch! Revised.

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

https://codereview.chromium.org/2001153002/diff/1/tools/viewer/sk_app/Window....
tools/viewer/sk_app/Window.cpp:67: fIsContentInvalidated = false;
On 2016/05/23 14:44:26, djsollen wrote:
> putting this here won't work on Android as the inval message calls
> paintIfNeeded() which checks the status of the backing window before getting
> here.  So you could end up in a situation like...
> 
> 1) post inval message
> 2) receive inval but don't call onPaint b/c the surface is not ready
> 3) all subsequent attempts to inval will be ignored
> 4) nothing ever draws. :(

Good catch! Revised.

[djsollen, 2016-05-23 15:22:42]

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

https://codereview.chromium.org/2001153002/diff/60001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.h:35: virtual void inval() = 0;
make this protected

https://codereview.chromium.org/2001153002/diff/60001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.h:40: void uncheckInval();
make this protected. I also find this naming confusing.  How about...

public:
  void inval();
protected:
  virtual void onInval() = 0;
  void markInvalReceived();

[liyuqian, 2016-05-23 15:49:25]

Name changed. Please see comments below.

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

https://codereview.chromium.org/2001153002/diff/60001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.h:35: virtual void inval() = 0;
On 2016/05/23 15:22:42, djsollen wrote:
> make this protected

Done.

https://codereview.chromium.org/2001153002/diff/60001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.h:40: void uncheckInval();
On 2016/05/23 15:22:42, djsollen wrote:
> make this protected. I also find this naming confusing.  How about...
> 
> public:
>   void inval();
> protected:
>   virtual void onInval() = 0;
>   void markInvalReceived();

I don't want to change inval's name because it's implemented in many places and
let it be the name for another purpose is very confusing... Maybe renaming
checkAndInval to invalIfNeeded and putting inval in protected is clear enough?

Also, I'd prefer markInvalProcessed and make it public because other classes
have to call it.

[djsollen, 2016-05-23 15:57:32]

I'm fine with not updating inval to onInval, but I do think that
markInvalProcessed should be protected.  On Android we just need to put a public
method on our subclass where it can decide what to do.

[liyuqian, 2016-05-23 16:10:21]

Ok, I made markInvalProcessed protected. How about this?

[djsollen, 2016-05-23 16:48:51]

I know I waffled on my decision to call the function onInval() vs inval(), but
after looking at the change I think this makes for the best API.

https://codereview.chromium.org/2001153002/diff/140001/tools/viewer/sk_app/Wi...
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/2001153002/diff/140001/tools/viewer/sk_app/Wi...
tools/viewer/sk_app/Window.h:32: void invalIfNeeded();
if a client should never call anything other than this function I would prefer
we just called it inval and then leave the existing call sites alone.  That
would mean that we need to change the protected method to onInval() but that
should require less modification than this current change.

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

https://codereview.chromium.org/2001153002/diff/140001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.cpp:105:
((Window_android*)skiaAndroidApp->fWindow)->paintIfNeeded();
simplify to window_android->paintIfNeeded();

[liyuqian, 2016-05-23 17:00:38]

Ok, inval is now onInval

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

https://codereview.chromium.org/2001153002/diff/140001/tools/viewer/sk_app/an...
tools/viewer/sk_app/android/surface_glue_android.cpp:105:
((Window_android*)skiaAndroidApp->fWindow)->paintIfNeeded();
On 2016/05/23 16:48:51, djsollen wrote:
> simplify to window_android->paintIfNeeded();

Done.

[djsollen, 2016-05-23 17:06:48]

lgtm with nits

https://codereview.chromium.org/2001153002/diff/160001/tools/viewer/sk_app/Wi...
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/2001153002/diff/160001/tools/viewer/sk_app/Wi...
tools/viewer/sk_app/Window.h:31: // Check if the current window has any pending
inval; call inval if not.
update the comment...

// shedules an invalidation event for window if one is not currently pending.

https://codereview.chromium.org/2001153002/diff/160001/tools/viewer/sk_app/Wi...
tools/viewer/sk_app/Window.h:170: // Make sure that either onPaint or
markInvalReceived is called in the subclasses'
I think this comment is outdated.  the client window doesn't need to worry about
this unless they don't call onPaint as part of handling the inval event.

[liyuqian, 2016-05-23 17:29:51]

Comments updated.

https://codereview.chromium.org/2001153002/diff/160001/tools/viewer/sk_app/Wi...
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/2001153002/diff/160001/tools/viewer/sk_app/Wi...
tools/viewer/sk_app/Window.h:170: // Make sure that either onPaint or
markInvalReceived is called in the subclasses'
On 2016/05/23 17:06:48, djsollen wrote:
> I think this comment is outdated.  the client window doesn't need to worry
about
> this unless they don't call onPaint as part of handling the inval event.

In case that the client window doesn't call onPaint (e.g., Android), this
comment reminds it to call markInvalProcessed. I'll update the comment to
reflect the name change of inval to onInval.

[liyuqian, 2016-05-23 17:29:58]

The CQ bit was checked by liyuqian@google.com

[liyuqian, 2016-05-23 17:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
jvanverth@google.com, djsollen@google.com
Link to the patchset: https://codereview.chromium.org/2001153002/#ps200001
(title: "Improve Comments")

[commit-bot: I haz the power, 2016-05-23 17:30:03]

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

[commit-bot: I haz the power, 2016-05-23 17:52:36]

Description was changed from

==========
Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2001153002
==========

to

==========
Move inval dedup to Window for wider usages.

It turns out that the inval dedup is not just useful for Android.  Hence
we move it up to the Window level so more OSes such as Linux, Windows
can also use it.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2001153002

Committed:
https://skia.googlesource.com/skia/+/566c8e4a36c51cb24d8bb97cc71b3eb26737c012
==========

[commit-bot: I haz the power, 2016-05-23 17:52:38]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/566c8e4a36c51cb24d8bb97cc71b3eb26737c012