Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(752)

Issue 1999583002: Add Event and EventTest (Closed)

Created:
4 years, 7 months ago by Hzj_jie
Modified:
4 years, 6 months ago
Reviewers:
Lambros, joedow
CC:
chromium-reviews, chromoting-reviews_chromium.org, Jamie
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG=615277 Committed: https://crrev.com/55898ecf9688df5ce26897362e35667f01eed7c3 Cr-Commit-Position: refs/heads/master@{#396752}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Synchronized mArray.size() calls #

Patch Set 3 : Remove WeakReference based event #

Patch Set 4 : Use only a Set instead of a Set and a List #

Total comments: 36

Patch Set 5 : Resolve review comments #

Total comments: 37

Patch Set 6 : Resolve review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+399 lines, -1 line) Patch
M remoting/android/BUILD.gn View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
M remoting/android/client_java_tmpl.gni View 1 chunk +2 lines, -0 lines 0 comments Download
A remoting/android/java/src/org/chromium/chromoting/Event.java View 1 2 3 4 5 1 chunk +176 lines, -0 lines 0 comments Download
A remoting/android/java/src/org/chromium/chromoting/Preconditions.java View 1 2 3 4 1 chunk +37 lines, -0 lines 0 comments Download
A remoting/android/javatests/src/org/chromium/chromoting/EventTest.java View 1 2 3 4 5 1 chunk +146 lines, -0 lines 0 comments Download
A remoting/android/javatests/src/org/chromium/chromoting/test/util/MutableReference.java View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download

Messages

Total messages: 33 (13 generated)
Hzj_jie
This change is ready to be reviewed.
4 years, 7 months ago (2016-05-20 23:50:04 UTC) #8
Hzj_jie
https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java#newcode53 remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; i < mArray.size(); i++) ...
4 years, 7 months ago (2016-05-23 23:39:15 UTC) #10
Lambros
https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java#newcode53 remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; i < mArray.size(); i++) ...
4 years, 7 months ago (2016-05-24 17:56:42 UTC) #11
Hzj_jie
https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java#newcode53 remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; i < mArray.size(); i++) ...
4 years, 7 months ago (2016-05-24 18:24:38 UTC) #12
Lambros
On 2016/05/24 18:24:38, Hzj_jie wrote: > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java > File remoting/android/java/src/org/chromium/chromoting/Event.java (right): > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java#newcode53 > ...
4 years, 7 months ago (2016-05-24 18:37:02 UTC) #13
Hzj_jie
On 2016/05/24 18:37:02, Lambros wrote: > On 2016/05/24 18:24:38, Hzj_jie wrote: > > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/src/org/chromium/chromoting/Event.java ...
4 years, 7 months ago (2016-05-24 20:39:56 UTC) #14
Hzj_jie
On 2016/05/24 20:39:56, Hzj_jie wrote: > On 2016/05/24 18:37:02, Lambros wrote: > > On 2016/05/24 ...
4 years, 7 months ago (2016-05-25 19:25:25 UTC) #15
Lambros
> Regarding the issues we have talked offline. > 1. Is there an Android or ...
4 years, 7 months ago (2016-05-25 21:43:48 UTC) #16
Hzj_jie
On 2016/05/25 21:43:48, Lambros wrote: > > Regarding the issues we have talked offline. > ...
4 years, 7 months ago (2016-05-25 23:18:51 UTC) #17
Lambros
On 2016/05/25 23:18:51, Hzj_jie wrote: > On 2016/05/25 21:43:48, Lambros wrote: > > > Regarding ...
4 years, 7 months ago (2016-05-26 19:32:02 UTC) #18
Lambros
Please don't link to internal documents in the CL description. Any links in Chromium should ...
4 years, 7 months ago (2016-05-27 00:38:52 UTC) #19
Hzj_jie
On 2016/05/26 19:32:02, Lambros wrote: > On 2016/05/25 23:18:51, Hzj_jie wrote: > > On 2016/05/25 ...
4 years, 7 months ago (2016-05-27 00:41:24 UTC) #20
Lambros
Also, this work is substantial enough that you should probably file a bug and include ...
4 years, 7 months ago (2016-05-27 00:43:12 UTC) #21
Hzj_jie
https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/src/org/chromium/chromoting/Event.java File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/src/org/chromium/chromoting/Event.java#newcode33 remoting/android/java/src/org/chromium/chromoting/Event.java:33: * An event provider version of {@link Event} implementation, ...
4 years, 7 months ago (2016-05-27 02:55:20 UTC) #23
Lambros
lgtm when nits are addressed. Please read my comments. I've made some suggestions, but I'm ...
4 years, 6 months ago (2016-05-27 22:26:07 UTC) #24
Hzj_jie
Thank you for your review, I have resolved most of the issues you have mentioned. ...
4 years, 6 months ago (2016-05-28 00:32:07 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999583002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999583002/160001
4 years, 6 months ago (2016-05-30 18:27:00 UTC) #28
commit-bot: I haz the power
Committed patchset #6 (id:160001)
4 years, 6 months ago (2016-05-30 21:14:35 UTC) #30
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/55898ecf9688df5ce26897362e35667f01eed7c3 Cr-Commit-Position: refs/heads/master@{#396752}
4 years, 6 months ago (2016-05-30 21:16:07 UTC) #32
Lambros
4 years, 6 months ago (2016-05-31 17:50:36 UTC) #33
Message was sent while issue was closed.
https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/Event.java (right):

https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/Event.java:99: if
(!mCallback.run(p)) {
I know the CL has landed now, but I see a couple of problems with this:

1) We may be holding onto mLock for too long. The callback can contain
arbitrary code, which (in theory) could include a re-entrant call to
this same method. This would re-acquire the lock (so the lock would be
acquired twice). We would probably end up removing mEvent twice,
triggering the precondition failure.

2) Some other thread (or the same re-entrant thread) might remove the
object from the event (by calling Event.remove() or Event.clear()),
between checking mOwner.contains() and calling mOwner.remove().

Maybe remove the Preconditions check?

Powered by Google App Engine
This is Rietveld 408576698