Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2434473002:
refactor waitUntilCriteria out of CallbackHelper (Closed)
|
Created:
4 years, 2 months ago by the real yoland Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionrefactor waitUntilCriteria out of CallbackHelper
waitUntilCriteria is only used by one class, and in general, it seems redundant to
provide mechanism to both do a polling wait and lock on test thread until being
notified. The waitForCallback method should be enough.
BUG=657162
Committed: https://crrev.com/4fbb52a7c4d3ea3445b510c97c4114a1b6799a20
Cr-Commit-Position: refs/heads/master@{#426363}
Patch Set 1 #Patch Set 2 : Delete waitUntilCriteria method #
Total comments: 9
Patch Set 3 : Changes after comments #
Total comments: 3
Patch Set 4 : no while loop #
Total comments: 4
Patch Set 5 : minor fixes #
Total comments: 1
Patch Set 6 : Add comment #
Total comments: 2
Patch Set 7 : nit #
Messages
Total messages: 28 (10 generated)
yolandyan@chromium.org changed reviewers: + boliu@chromium.org, jbudorick@chromium.org
Description was changed from ========== refactor waitUntilCriteria out of CallbackHelper BUG=657162 ========== to ========== refactor waitUntilCriteria out of CallbackHelper waitUntilCriteria is only used by one class, and in general, it seems redundant to provide mechanism to both do a polling wait and lock on test thread until being notified. The waitForCallback method should be enough. BUG=657162 ==========
oh good, you had the same thought :) https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java (right): https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java:212: protected void block(long timeout, TimeUnit timeoutUnits) this makes makes no sense.. https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:165: public Criteria getHasValueCriteria() { you can remove this and just call hasValue everywhere https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:178: public boolean waitUntilHasValue(long timeout, TimeUnit timeoutUnits) nothing actually calls this version, can collapse it into the method below as well https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:182: boolean isSatisfied = criteria.isSatisfied(); so this isn't thread safe in general if you don't do it within with the synchronization you dropped (although java reference assignment is atomic..) I *think* this doesn't need any polling at all, you can just do count = getCount if (hasValue()) return; waitForCallback(count) although I guess let the bots confirm if I'm right :p
https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java (right): https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java:212: protected void block(long timeout, TimeUnit timeoutUnits) On 2016/10/19 at 04:06:26, boliu wrote: > this makes makes no sense.. lool, I would argue neither does waitUntilCriteria. Just kidding, I thought this would be a way to "expose" the lock to its subclass, allowing TestCallbackHelperContainer to do its crazy stuff https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:178: public boolean waitUntilHasValue(long timeout, TimeUnit timeoutUnits) On 2016/10/19 at 04:06:26, boliu wrote: > nothing actually calls this version, can collapse it into the method below as well This does: https://cs.chromium.org/chromium/src/content/public/test/android/javatests/sr... https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:182: boolean isSatisfied = criteria.isSatisfied(); On 2016/10/19 at 04:06:26, boliu wrote: > so this isn't thread safe in general if you don't do it within with the synchronization you dropped (although java reference assignment is atomic..) > > I *think* this doesn't need any polling at all, you can just do > > count = getCount > if (hasValue()) return; > waitForCallback(count) > > although I guess let the bots confirm if I'm right :p wait, but if a null JSON is returned by handleJavaScriptResult in the JavaScriptCallback, then this will not wait until a non-null JSON being set as mJsonResult and simply return null. we still need to awhile loop to do this. If `block()` method seems unintuitive, then I can just do ``` while (!isSatistied && time - startTime < timeout) { count = getCount() waitForCallback(count) isSatistifed = criteria.isSatisfied() } if (!isSatisfied) throw new TimeoutException(criteria.getFailureReason()); return hasValue(); ``` https://codereview.chromium.org/2434473002/diff/40001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/40001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:185: block(timeout, unit); hmmm, this potentially can run forever, the timeout argument here should be recalculated
https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/20001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:182: boolean isSatisfied = criteria.isSatisfied(); On 2016/10/19 04:38:09, the real yoland wrote: > On 2016/10/19 at 04:06:26, boliu wrote: > > so this isn't thread safe in general if you don't do it within with the > synchronization you dropped (although java reference assignment is atomic..) > > > > I *think* this doesn't need any polling at all, you can just do > > > > count = getCount > > if (hasValue()) return; > > waitForCallback(count) > > > > although I guess let the bots confirm if I'm right :p > > wait, but if a null JSON is returned by handleJavaScriptResult in the > JavaScriptCallback, then this will not wait until a non-null JSON being set as > mJsonResult and simply return null. > > we still need to awhile loop to do this. If `block()` method seems unintuitive, > then I can just do > ``` > while (!isSatistied && time - startTime < timeout) { > count = getCount() > waitForCallback(count) > isSatistifed = criteria.isSatisfied() > } > > if (!isSatisfied) throw new TimeoutException(criteria.getFailureReason()); > return hasValue(); > ``` Is that distinction important? Does any test actually rely on watching *two* evaluate calls, where only the second sets a non-null result? If yes, I'd argue that test should be fixed.
https://codereview.chromium.org/2434473002/diff/40001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/40001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:181: Criteria criteria = getHasValueCriteria(); This is basically a handrolled version of CriteriaHelper.pollInstrumentationThread. If we're going to do that, can we just refactor this to use CriteriaHelper?
https://codereview.chromium.org/2434473002/diff/40001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/40001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:181: Criteria criteria = getHasValueCriteria(); On 2016/10/19 16:16:58, jbudorick wrote: > This is basically a handrolled version of > CriteriaHelper.pollInstrumentationThread. If we're going to do that, can we just > refactor this to use CriteriaHelper? Not quite. This one is waiting for callback signals between checks, whereas CriteriaHelper one is time-based polling
https://chromiumcodereview.appspot.com/2434473002/diff/20001/content/public/t... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://chromiumcodereview.appspot.com/2434473002/diff/20001/content/public/t... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:182: boolean isSatisfied = criteria.isSatisfied(); On 2016/10/19 at 04:49:14, boliu wrote: > On 2016/10/19 04:38:09, the real yoland wrote: > > On 2016/10/19 at 04:06:26, boliu wrote: > > > so this isn't thread safe in general if you don't do it within with the > > synchronization you dropped (although java reference assignment is atomic..) > > > > > > I *think* this doesn't need any polling at all, you can just do > > > > > > count = getCount > > > if (hasValue()) return; > > > waitForCallback(count) > > > > > > although I guess let the bots confirm if I'm right :p > > > > wait, but if a null JSON is returned by handleJavaScriptResult in the > > JavaScriptCallback, then this will not wait until a non-null JSON being set as > > mJsonResult and simply return null. > > > > we still need to awhile loop to do this. If `block()` method seems unintuitive, > > then I can just do > > ``` > > while (!isSatistied && time - startTime < timeout) { > > count = getCount() > > waitForCallback(count) > > isSatistifed = criteria.isSatisfied() > > } > > > > if (!isSatisfied) throw new TimeoutException(criteria.getFailureReason()); > > return hasValue(); > > ``` > > Is that distinction important? Does any test actually rely on watching *two* evaluate calls, where only the second sets a non-null result? If yes, I'd argue that test should be fixed. Done
run through the android bots to make sure these changes are actually ok? https://chromiumcodereview.appspot.com/2434473002/diff/60001/content/public/t... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java (right): https://chromiumcodereview.appspot.com/2434473002/diff/60001/content/public/t... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java:212: protected void block(long timeout, TimeUnit timeoutUnits) remove https://chromiumcodereview.appspot.com/2434473002/diff/60001/content/public/t... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://chromiumcodereview.appspot.com/2434473002/diff/60001/content/public/t... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:166: if (hasValue()) { nit: you can put this on one line, without braces
https://codereview.chromium.org/2434473002/diff/60001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java (right): https://codereview.chromium.org/2434473002/diff/60001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/CallbackHelper.java:212: protected void block(long timeout, TimeUnit timeoutUnits) On 2016/10/19 at 17:28:53, boliu wrote: > remove Done https://codereview.chromium.org/2434473002/diff/60001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/60001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:166: if (hasValue()) { On 2016/10/19 at 17:28:53, boliu wrote: > nit: you can put this on one line, without braces Done
lgtm % bots are happy https://codereview.chromium.org/2434473002/diff/80001/content/public/test/and... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://codereview.chromium.org/2434473002/diff/80001/content/public/test/and... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:166: if (hasValue()) return true; Should add a comment here that this is thread safe only because reference assignment in java is atomic
The CQ bit was checked by yolandyan@chromium.org
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2434473002/#ps100001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://chromiumcodereview.appspot.com/2434473002/diff/100001/content/public/... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://chromiumcodereview.appspot.com/2434473002/diff/100001/content/public/... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:166: //Reads and writes are atomic for reference variables in java, this is thread safe nit: space between "//" and "Reads". period at the end
https://chromiumcodereview.appspot.com/2434473002/diff/100001/content/public/... File content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java (right): https://chromiumcodereview.appspot.com/2434473002/diff/100001/content/public/... content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:166: //Reads and writes are atomic for reference variables in java, this is thread safe On 2016/10/19 at 20:44:54, boliu wrote: > nit: space between "//" and "Reads". period at the end Done
The CQ bit was checked by yolandyan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2434473002/#ps120001 (title: "nit")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by yolandyan@chromium.org
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.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== refactor waitUntilCriteria out of CallbackHelper waitUntilCriteria is only used by one class, and in general, it seems redundant to provide mechanism to both do a polling wait and lock on test thread until being notified. The waitForCallback method should be enough. BUG=657162 ========== to ========== refactor waitUntilCriteria out of CallbackHelper waitUntilCriteria is only used by one class, and in general, it seems redundant to provide mechanism to both do a polling wait and lock on test thread until being notified. The waitForCallback method should be enough. BUG=657162 Committed: https://crrev.com/4fbb52a7c4d3ea3445b510c97c4114a1b6799a20 Cr-Commit-Position: refs/heads/master@{#426363} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4fbb52a7c4d3ea3445b510c97c4114a1b6799a20 Cr-Commit-Position: refs/heads/master@{#426363} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
