Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2385593002:
[Chromoting] PromisedRaisable in Android client (Closed)
Description[Chromoting] PromisedRaisable in Android client
Some events, such as RenderStub.onClientSizeChanged(), triggers only once during
its lifetime. Though we can ensure to add a ParameterRunnable before the event
is triggered, it seems a little bit inconvenient.
So this change adds a PromisedRaisable Event derived class to ensure newly
added ParameterRunnable can at least be executed once.
BUG=615277
Committed: https://crrev.com/dcc5d04905bb29d9a1786adcae1332d447ef6332
Cr-Commit-Position: refs/heads/master@{#423800}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Resolve review comments #
Total comments: 2
Messages
Total messages: 34 (21 generated)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [Chromoting] ReproducibleEvent in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a ReproducibleRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ========== to ========== [Chromoting] ReproducibleEvent in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a ReproducibleRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ==========
zijiehe@chromium.org changed reviewers: + lambroslambrou@chromium.org, yuweih@chromium.org
An event that triggers once, and still triggers even if you add the listener after the fact. Now where have I seen that before... :) lgtm, just some nit-picking, and a question about sync/async behavior. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:67: * An {@link Raisable} which always executes the newly added {@link ParameterRunnable} with the s/An/A/ https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:69: * this class has a consistent behavior as {@link Raisable}. ..has consistent behavior with.. or ..behaves the same as.. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:70: * <p> This class is useful for some one-time events, such as RenderStub.onClientSizeChanged(). I might be mistaken, but is it common style to use <p> for paragraphs instead of just using a blank line? <p> looks a bit ugly to me, but if it's common elsewhere, I guess we can stick with it? https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:72: * <p> The {@link ParameterRunnable} will be executed in the thread which {@link #add} function ..thread which calls {@link #add}.. or ..thread in which {@link #add} is called.. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if the event has been raised before. You're saying that the implementation will call the Runnable in the "same thread". But actually this implementation calls it *immediately*. This will come as a surprise to those who are expecting Promise-like async behavior. The Promise specification requires that the callback does not immediately trigger: https://promisesaplus.com/#point-34 (2.2.4) We should either follow the same design choice here. Or we should make it clear in the JavaDoc that the method may be called synchronously inside the add() call. I think I would prefer the former, even though it means writing extra code to post the task onto the current message-loop. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:74: * <p> The {@link ParameterRunnable} may be executed twice on different threads with exactly Personally, I wouldn't make this a separate paragraph. I would continue the previous paragraph with: "Note that it may be executed twice..." If you say "Note that..." or "However, ...", you are clarifying that this is an exception to the claim that the Runnable only triggers once. This means you can remove the final sentence: "Usually.." https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:75: * same parameter. Usually this is not an issue, but consumers need to aware. ..to be aware. But you can remove this anyway.
joedow@chromium.org changed reviewers: + joedow@chromium.org
https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if the event has been raised before. On 2016/10/05 19:30:04, Lambros wrote: > You're saying that the implementation will call the Runnable in the "same > thread". But actually this implementation calls it *immediately*. > This will come as a surprise to those who are expecting Promise-like async > behavior. The Promise specification requires that the callback does not > immediately trigger: > https://promisesaplus.com/#point-34 (2.2.4) > > We should either follow the same design choice here. Or we should make it clear > in the JavaDoc that the method may be called synchronously inside the add() > call. I think I would prefer the former, even though it means writing extra code > to post the task onto the current message-loop. Agreed, I've used events which can either fire sync or async and they can be a pain due to race conditions. I'd prefer they are always async as well. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:77: public static final class ReproducibleRaisable<ParamT> extends Raisable<ParamT> { The name ReproducibleRaisable doesn't tell me that this is going to fire if it has already been called in the past. I'd think about naming this differently so someone scanning the code can get a better idea of what this class does.
https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:72: * <p> The {@link ParameterRunnable} will be executed in the thread which {@link #add} function I think to be consistent it could be better to run all callbacks on the same thread. Maybe the thread where raise() is first called or the constructor is called. Also since now all Events are being used on the UI thread, we may simplify these problems and make Event single-threaded. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if the event has been raised before. On 2016/10/05 19:30:04, Lambros wrote: > You're saying that the implementation will call the Runnable in the "same > thread". But actually this implementation calls it *immediately*. > This will come as a surprise to those who are expecting Promise-like async > behavior. The Promise specification requires that the callback does not > immediately trigger: > https://promisesaplus.com/#point-34 (2.2.4) > > We should either follow the same design choice here. Or we should make it clear > in the JavaDoc that the method may be called synchronously inside the add() > call. I think I would prefer the former, even though it means writing extra code > to post the task onto the current message-loop. Yep. Running the callbacks in current task will have the risk of infinite recursion if the callback raises the same event. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:86: if (mRaised) { Could you do the check without the boolean, say checking whether mLastParameter is not null?
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:67: * An {@link Raisable} which always executes the newly added {@link ParameterRunnable} with the On 2016/10/05 19:30:04, Lambros wrote: > s/An/A/ Done. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:69: * this class has a consistent behavior as {@link Raisable}. On 2016/10/05 19:30:03, Lambros wrote: > ..has consistent behavior with.. > or > ..behaves the same as.. Done. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:70: * <p> This class is useful for some one-time events, such as RenderStub.onClientSizeChanged(). On 2016/10/05 19:30:04, Lambros wrote: > I might be mistaken, but is it common style to use <p> for paragraphs instead of > just using a blank line? <p> looks a bit ugly to me, but if it's common > elsewhere, I guess we can stick with it? AFAICT, without <p> or <br>, the newline character will simply be ignored, and replaced by a space in the final html output. I intentionally separate the comments into several paragraphs. I may also use <ul> and <li> here, which displays the sentences as a list. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:72: * <p> The {@link ParameterRunnable} will be executed in the thread which {@link #add} function On 2016/10/05 19:30:04, Lambros wrote: > ..thread which calls {@link #add}.. > or > ..thread in which {@link #add} is called.. > Done. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:72: * <p> The {@link ParameterRunnable} will be executed in the thread which {@link #add} function On 2016/10/05 20:50:34, Yuwei wrote: > I think to be consistent it could be better to run all callbacks on the same > thread. Maybe the thread where raise() is first called or the constructor is > called. > > Also since now all Events are being used on the UI thread, we may simplify these > problems and make Event single-threaded. Yes, but I would prefer to remove all the synchronizations in a single change, and update the comments. Before that, the Event class is designed to be thread-safe. Writing a class with partial thread-safety looks weird to me. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if the event has been raised before. On 2016/10/05 20:50:34, Yuwei wrote: > On 2016/10/05 19:30:04, Lambros wrote: > > You're saying that the implementation will call the Runnable in the "same > > thread". But actually this implementation calls it *immediately*. > > This will come as a surprise to those who are expecting Promise-like async > > behavior. The Promise specification requires that the callback does not > > immediately trigger: > > https://promisesaplus.com/#point-34 (2.2.4) > > > > We should either follow the same design choice here. Or we should make it > clear > > in the JavaDoc that the method may be called synchronously inside the add() > > call. I think I would prefer the former, even though it means writing extra > code > > to post the task onto the current message-loop. > > Yep. Running the callbacks in current task will have the risk of infinite > recursion if the callback raises the same event. This should not happen, raise is only called from providers instead of listeners. The asynchronous callback is a good suggestion, I will update. But I do find a potential issue. A posted task in current looper may change mLastParameter by raising the event. So a newly attached runnable may not be able to get the exact parameter before it attached. The test cases show the difference. And we also cannot simply record mLastParameter, it will impact the ordering of parameters. I do not see this could make any real trouble for now. But I would prefer to add some comments to explain the issue here. To avoid the edge case, the add is called on a thread without looper, I will still execute the runnable immediately. This won't happen in our logic, since we are always running in the main looper. But some thirdparty threads may not have a looper, crashing or ignoring in these threads seems not correct to me. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:74: * <p> The {@link ParameterRunnable} may be executed twice on different threads with exactly On 2016/10/05 19:30:04, Lambros wrote: > Personally, I wouldn't make this a separate paragraph. I would continue the > previous paragraph with: > "Note that it may be executed twice..." > > If you say "Note that..." or "However, ...", you are clarifying that this is an > exception to the claim that the Runnable only triggers once. This means you can > remove the final sentence: "Usually.." Done. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:75: * same parameter. Usually this is not an issue, but consumers need to aware. On 2016/10/05 19:30:04, Lambros wrote: > ..to be aware. > But you can remove this anyway. Done. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:77: public static final class ReproducibleRaisable<ParamT> extends Raisable<ParamT> { On 2016/10/05 19:46:43, joedow wrote: > The name ReproducibleRaisable doesn't tell me that this is going to fire if it > has already been called in the past. I'd think about naming this differently so > someone scanning the code can get a better idea of what this class does. Changed to PromisedRaisable. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:86: if (mRaised) { On 2016/10/05 20:50:34, Yuwei wrote: > Could you do the check without the boolean, say checking whether mLastParameter > is not null? No, not a good idea, we cannot ensure a null-able parameter is always used in an Event. (Indeed we have several events with Void parameter type.)
https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:72: * <p> The {@link ParameterRunnable} will be executed in the thread which {@link #add} function On 2016/10/06 00:08:09, Hzj_jie wrote: > On 2016/10/05 20:50:34, Yuwei wrote: > > I think to be consistent it could be better to run all callbacks on the same > > thread. Maybe the thread where raise() is first called or the constructor is > > called. > > > > Also since now all Events are being used on the UI thread, we may simplify > these > > problems and make Event single-threaded. > > Yes, but I would prefer to remove all the synchronizations in a single change, > and update the comments. Before that, the Event class is designed to be > thread-safe. Writing a class with partial thread-safety looks weird to me. Acknowledged. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if the event has been raised before. On 2016/10/06 00:08:09, Hzj_jie wrote: > On 2016/10/05 20:50:34, Yuwei wrote: > > On 2016/10/05 19:30:04, Lambros wrote: > > > You're saying that the implementation will call the Runnable in the "same > > > thread". But actually this implementation calls it *immediately*. > > > This will come as a surprise to those who are expecting Promise-like async > > > behavior. The Promise specification requires that the callback does not > > > immediately trigger: > > > https://promisesaplus.com/#point-34 (2.2.4) > > > > > > We should either follow the same design choice here. Or we should make it > > clear > > > in the JavaDoc that the method may be called synchronously inside the add() > > > call. I think I would prefer the former, even though it means writing extra > > code > > > to post the task onto the current message-loop. > > > > Yep. Running the callbacks in current task will have the risk of infinite > > recursion if the callback raises the same event. > This should not happen, raise is only called from providers instead of > listeners. Well, you code doesn't disallow circular event dependency :P > The asynchronous callback is a good suggestion, I will update. > > But I do find a potential issue. A posted task in current looper may change > mLastParameter by raising the event. So a newly attached runnable may not be > able to get the exact parameter before it attached. The test cases show the > difference. > And we also cannot simply record mLastParameter, it will impact the ordering of > parameters. > I do not see this could make any real trouble for now. But I would prefer to add > some comments to explain the issue here. IIUC your concern is, when A,B is raised: 1. You get B,B if you capture `this` 2. you get B,A if you capture mLastParameter Right? I'm okay with getting B,B since we only care about the latest value. Or you can use a boolean to avoid double feeding the listener. https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:86: if (mRaised) { On 2016/10/06 00:08:09, Hzj_jie wrote: > On 2016/10/05 20:50:34, Yuwei wrote: > > Could you do the check without the boolean, say checking whether > mLastParameter > > is not null? > > No, not a good idea, we cannot ensure a null-able parameter is always used in an > Event. (Indeed we have several events with Void parameter type.) Ah. I think you are right. https://codereview.chromium.org/2385593002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:55: execute(obj, parameter); Will you also make Raisable.raise() async?
Description was changed from ========== [Chromoting] ReproducibleEvent in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a ReproducibleRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ========== to ========== [Chromoting] PromisedRaisable in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a PromisedRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ==========
https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if the event has been raised before. On 2016/10/06 01:32:24, Yuwei wrote: > On 2016/10/06 00:08:09, Hzj_jie wrote: > > On 2016/10/05 20:50:34, Yuwei wrote: > > > On 2016/10/05 19:30:04, Lambros wrote: > > > > You're saying that the implementation will call the Runnable in the "same > > > > thread". But actually this implementation calls it *immediately*. > > > > This will come as a surprise to those who are expecting Promise-like async > > > > behavior. The Promise specification requires that the callback does not > > > > immediately trigger: > > > > https://promisesaplus.com/#point-34 (2.2.4) > > > > > > > > We should either follow the same design choice here. Or we should make it > > > clear > > > > in the JavaDoc that the method may be called synchronously inside the > add() > > > > call. I think I would prefer the former, even though it means writing > extra > > > code > > > > to post the task onto the current message-loop. > > > > > > Yep. Running the callbacks in current task will have the risk of infinite > > > recursion if the callback raises the same event. > > This should not happen, raise is only called from providers instead of > > listeners. > > Well, you code doesn't disallow circular event dependency :P > I think that's a totally incorrect pattern. It looks weird if a component listen to another event just for raising its own event. The listener should listen to the original event instead. > > The asynchronous callback is a good suggestion, I will update. > > > > But I do find a potential issue. A posted task in current looper may change > > mLastParameter by raising the event. So a newly attached runnable may not be > > able to get the exact parameter before it attached. The test cases show the > > difference. > > And we also cannot simply record mLastParameter, it will impact the ordering > of > > parameters. > > I do not see this could make any real trouble for now. But I would prefer to > add > > some comments to explain the issue here. > > IIUC your concern is, when A,B is raised: > > 1. You get B,B if you capture `this` > 2. you get B,A if you capture mLastParameter > > Right? I'm okay with getting B,B since we only care about the latest value. Or > you can use a boolean to avoid double feeding the listener. Yes, reordering is definitely not acceptable, since most listeners care about the latest value. I cannot quite tell where is the right place to add the boolean value. But another scenario may be listeners care about how many times the event fired, say, a statistic component listens to onCanvasRendered event to calculate the FPS. The data is not important, but the number of times is. So current solution can satisfy both scenarios.
On 2016/10/06 19:01:34, Hzj_jie wrote: > https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... > File remoting/android/java/src/org/chromium/chromoting/Event.java (right): > > https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called if > the event has been raised before. > On 2016/10/06 01:32:24, Yuwei wrote: > > On 2016/10/06 00:08:09, Hzj_jie wrote: > > > On 2016/10/05 20:50:34, Yuwei wrote: > > > > On 2016/10/05 19:30:04, Lambros wrote: > > > > > You're saying that the implementation will call the Runnable in the > "same > > > > > thread". But actually this implementation calls it *immediately*. > > > > > This will come as a surprise to those who are expecting Promise-like > async > > > > > behavior. The Promise specification requires that the callback does not > > > > > immediately trigger: > > > > > https://promisesaplus.com/#point-34 (2.2.4) > > > > > > > > > > We should either follow the same design choice here. Or we should make > it > > > > clear > > > > > in the JavaDoc that the method may be called synchronously inside the > > add() > > > > > call. I think I would prefer the former, even though it means writing > > extra > > > > code > > > > > to post the task onto the current message-loop. > > > > > > > > Yep. Running the callbacks in current task will have the risk of infinite > > > > recursion if the callback raises the same event. > > > This should not happen, raise is only called from providers instead of > > > listeners. > > > > Well, you code doesn't disallow circular event dependency :P > > > I think that's a totally incorrect pattern. It looks weird if a component listen > to another event just for raising its own event. The listener should listen to > the original event instead. > > > The asynchronous callback is a good suggestion, I will update. > > > > > > But I do find a potential issue. A posted task in current looper may change > > > mLastParameter by raising the event. So a newly attached runnable may not be > > > able to get the exact parameter before it attached. The test cases show the > > > difference. > > > And we also cannot simply record mLastParameter, it will impact the ordering > > of > > > parameters. > > > I do not see this could make any real trouble for now. But I would prefer to > > add > > > some comments to explain the issue here. > > > > IIUC your concern is, when A,B is raised: > > > > 1. You get B,B if you capture `this` > > 2. you get B,A if you capture mLastParameter > > > > Right? I'm okay with getting B,B since we only care about the latest value. Or > > you can use a boolean to avoid double feeding the listener. > > Yes, reordering is definitely not acceptable, since most listeners care about > the latest value. I cannot quite tell where is the right place to add the > boolean value. But another scenario may be listeners care about how many times > the event fired, say, a statistic component listens to onCanvasRendered event to > calculate the FPS. The data is not important, but the number of times is. So > current solution can satisfy both scenarios. Well, there can be many scenarios with different behavioral requirements. Say a running sample averager will require all samples to be fed exactly once in any order, and an FPS counter will require keeping the interval between two events. Since current implementation works well for current use case, maybe just clearly document the current behavior so that people know what to change when they have different use case.
https://codereview.chromium.org/2385593002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/2385593002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:55: execute(obj, parameter); On 2016/10/06 01:32:24, Yuwei wrote: > Will you also make Raisable.raise() async? That seems not necessary. raise function is not controlled by listeners.
On 2016/10/06 21:03:12, Yuwei wrote: > On 2016/10/06 19:01:34, Hzj_jie wrote: > > > https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... > > File remoting/android/java/src/org/chromium/chromoting/Event.java (right): > > > > > https://codereview.chromium.org/2385593002/diff/20001/remoting/android/java/s... > > remoting/android/java/src/org/chromium/chromoting/Event.java:73: * is called > if > > the event has been raised before. > > On 2016/10/06 01:32:24, Yuwei wrote: > > > On 2016/10/06 00:08:09, Hzj_jie wrote: > > > > On 2016/10/05 20:50:34, Yuwei wrote: > > > > > On 2016/10/05 19:30:04, Lambros wrote: > > > > > > You're saying that the implementation will call the Runnable in the > > "same > > > > > > thread". But actually this implementation calls it *immediately*. > > > > > > This will come as a surprise to those who are expecting Promise-like > > async > > > > > > behavior. The Promise specification requires that the callback does > not > > > > > > immediately trigger: > > > > > > https://promisesaplus.com/#point-34 (2.2.4) > > > > > > > > > > > > We should either follow the same design choice here. Or we should make > > it > > > > > clear > > > > > > in the JavaDoc that the method may be called synchronously inside the > > > add() > > > > > > call. I think I would prefer the former, even though it means writing > > > extra > > > > > code > > > > > > to post the task onto the current message-loop. > > > > > > > > > > Yep. Running the callbacks in current task will have the risk of > infinite > > > > > recursion if the callback raises the same event. > > > > This should not happen, raise is only called from providers instead of > > > > listeners. > > > > > > Well, you code doesn't disallow circular event dependency :P > > > > > I think that's a totally incorrect pattern. It looks weird if a component > listen > > to another event just for raising its own event. The listener should listen to > > the original event instead. > > > > The asynchronous callback is a good suggestion, I will update. > > > > > > > > But I do find a potential issue. A posted task in current looper may > change > > > > mLastParameter by raising the event. So a newly attached runnable may not > be > > > > able to get the exact parameter before it attached. The test cases show > the > > > > difference. > > > > And we also cannot simply record mLastParameter, it will impact the > ordering > > > of > > > > parameters. > > > > I do not see this could make any real trouble for now. But I would prefer > to > > > add > > > > some comments to explain the issue here. > > > > > > IIUC your concern is, when A,B is raised: > > > > > > 1. You get B,B if you capture `this` > > > 2. you get B,A if you capture mLastParameter > > > > > > Right? I'm okay with getting B,B since we only care about the latest value. > Or > > > you can use a boolean to avoid double feeding the listener. > > > > Yes, reordering is definitely not acceptable, since most listeners care about > > the latest value. I cannot quite tell where is the right place to add the > > boolean value. But another scenario may be listeners care about how many times > > the event fired, say, a statistic component listens to onCanvasRendered event > to > > calculate the FPS. The data is not important, but the number of times is. So > > current solution can satisfy both scenarios. > > Well, there can be many scenarios with different behavioral requirements. Say a > running sample averager will require all samples to be fed exactly once in any > order, and an FPS counter will require keeping the interval between two events. > Since current implementation works well for current use case, maybe just clearly > document the current behavior so that people know what to change when they have > different use case. The parameters a listener received are always correct, after the listener attached to the event.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/2385593002/#ps40001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] PromisedRaisable in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a PromisedRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ========== to ========== [Chromoting] PromisedRaisable in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a PromisedRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] PromisedRaisable in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a PromisedRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 ========== to ========== [Chromoting] PromisedRaisable in Android client Some events, such as RenderStub.onClientSizeChanged(), triggers only once during its lifetime. Though we can ensure to add a ParameterRunnable before the event is triggered, it seems a little bit inconvenient. So this change adds a PromisedRaisable Event derived class to ensure newly added ParameterRunnable can at least be executed once. BUG=615277 Committed: https://crrev.com/dcc5d04905bb29d9a1786adcae1332d447ef6332 Cr-Commit-Position: refs/heads/master@{#423800} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dcc5d04905bb29d9a1786adcae1332d447ef6332 Cr-Commit-Position: refs/heads/master@{#423800} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
