Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2665133005:
[Omaha] Make the whole pipeline run each time (Closed)
Description[Omaha] Make the whole pipeline run each time
* Instead of making OmahaClient run as an IntentService that handles individual
actions, run the whole pipeline, including registering new requests and
POSTing to the server. This has several advantages, including that the
pipeline is somewhat easier to understand.
* The pipeline now uses one single-shot alarm to determine when to either
generate a new active user request or to trigger a POST. This requires
canceling existing repeating alarms.
* Redoes the "isOverude" check in handleRegisterActiveRequest to just check
current timestamps instead of also checking that a request doesn't exist.
This is more logical for an "isOverdue" check, and prevents the OmahaClient
from trying to reschedule itself repeatedly until an existing request's
timestamps are screwed up. This should be alright because the request
timestamp and the timestamp for the new request should match in all cases
where the SharedPrerences are valid.
* Introduces the OmahaDelegate, which masks out calls to various classes that
OmahaClient requires.
* Replaces all old OmahaClientTests that tested the old pipeline with one that
is more black-boxy and tests the new pipeline. Significant cleanups are still
required here, but the pipeline will be changing further and basic tests are
better than no tests.
BUG=680817
Review-Url: https://codereview.chromium.org/2665133005
Cr-Commit-Position: refs/heads/master@{#448803}
Committed: https://chromium.googlesource.com/chromium/src/+/c9625f069b73578ab45f49be70a830d48f6141bc
Patch Set 1 #Patch Set 2 : fix compile #
Total comments: 18
Patch Set 3 : Comments #
Total comments: 16
Patch Set 4 : Comments #
Messages
Total messages: 34 (22 generated)
The CQ bit was checked by dfalcantara@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by dfalcantara@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...
dfalcantara@chromium.org changed reviewers: + nyquist@chromium.org
So... the change to OmahaClient is large, but a lot of code has been moved around. The tests had to be completely scrapped; the new ones could probably be condensed with common functions but I wanted their intentions to be clearer. More tests will be following in future CLs, which unwind most of the OmahaClient into OmahaBase.
Description was changed from ========== [Omaha] Make the whole pipeline run each time * Instead of making OmahaClient run as an IntentService that handles individual actions, run the whole pipeline, including registering new requests and POSTing to the server. This has several advantages, including that the pipeline is somewhat easier to understand. * Redoes the "isOverude" check in handleRegisterActiveRequest to just check current timestamps instead of also checking that a request doesn't exist. This is more logical for an "isOverdue" check, and prevents the OmahaClient from trying to reschedule itself repeatedly until an existing request's timestamps are screwed up. This should be alright because the request timestamp and the timestamp for the new request should match in all cases where the SharedPrerences are valid. * Introduces the OmahaDelegate, which masks out calls to various classes that OmahaClient requires. * Replaces all old OmahaClientTests that tested the old pipeline with one that is more black-boxy and tests the new pipeline. Significant cleanups are still required here, but the pipeline will be changing further and basic tests are better than no tests. BUG=680817 ========== to ========== [Omaha] Make the whole pipeline run each time * Instead of making OmahaClient run as an IntentService that handles individual actions, run the whole pipeline, including registering new requests and POSTing to the server. This has several advantages, including that the pipeline is somewhat easier to understand. * The pipeline now uses a single alarm to determine when to either generate a new active user request or to trigger a POST. * Redoes the "isOverude" check in handleRegisterActiveRequest to just check current timestamps instead of also checking that a request doesn't exist. This is more logical for an "isOverdue" check, and prevents the OmahaClient from trying to reschedule itself repeatedly until an existing request's timestamps are screwed up. This should be alright because the request timestamp and the timestamp for the new request should match in all cases where the SharedPrerences are valid. * Introduces the OmahaDelegate, which masks out calls to various classes that OmahaClient requires. * Replaces all old OmahaClientTests that tested the old pipeline with one that is more black-boxy and tests the new pipeline. Significant cleanups are still required here, but the pipeline will be changing further and basic tests are better than no tests. BUG=680817 ==========
Description was changed from ========== [Omaha] Make the whole pipeline run each time * Instead of making OmahaClient run as an IntentService that handles individual actions, run the whole pipeline, including registering new requests and POSTing to the server. This has several advantages, including that the pipeline is somewhat easier to understand. * The pipeline now uses a single alarm to determine when to either generate a new active user request or to trigger a POST. * Redoes the "isOverude" check in handleRegisterActiveRequest to just check current timestamps instead of also checking that a request doesn't exist. This is more logical for an "isOverdue" check, and prevents the OmahaClient from trying to reschedule itself repeatedly until an existing request's timestamps are screwed up. This should be alright because the request timestamp and the timestamp for the new request should match in all cases where the SharedPrerences are valid. * Introduces the OmahaDelegate, which masks out calls to various classes that OmahaClient requires. * Replaces all old OmahaClientTests that tested the old pipeline with one that is more black-boxy and tests the new pipeline. Significant cleanups are still required here, but the pipeline will be changing further and basic tests are better than no tests. BUG=680817 ========== to ========== [Omaha] Make the whole pipeline run each time * Instead of making OmahaClient run as an IntentService that handles individual actions, run the whole pipeline, including registering new requests and POSTing to the server. This has several advantages, including that the pipeline is somewhat easier to understand. * The pipeline now uses one single-shot alarm to determine when to either generate a new active user request or to trigger a POST. This requires canceling existing repeating alarms. * Redoes the "isOverude" check in handleRegisterActiveRequest to just check current timestamps instead of also checking that a request doesn't exist. This is more logical for an "isOverdue" check, and prevents the OmahaClient from trying to reschedule itself repeatedly until an existing request's timestamps are screwed up. This should be alright because the request timestamp and the timestamp for the new request should match in all cases where the SharedPrerences are valid. * Introduces the OmahaDelegate, which masks out calls to various classes that OmahaClient requires. * Replaces all old OmahaClientTests that tested the old pipeline with one that is more black-boxy and tests the new pipeline. Significant cleanups are still required here, but the pipeline will be changing further and basic tests are better than no tests. BUG=680817 ==========
For reference: the next CL is a nearly straightforward move of all the OmahaClient code to OmahaBase: https://codereview.chromium.org/2664253005
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry, didn't get to the tests yet. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:130: // TODO(dfalcantara): Prevent Omaha code from repeatedly rescheduling itself immediately in Should we also do some clamping? Maybe |>= 0| in case the code above fails horribly? https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:386: // Remove any repeating alarms in favor of the new scheduling setup on M58 and up. Nit: Could you extract this new block to its own method with an excellent name? https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:42: /** Schedules the Service to run again at the given time. */ Nit: {@link Service} https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:43: abstract void scheduleService(Service service, long nextTimestamp); Could you add a suffix or something to nextTimestamp to give a hint to the reader of the value type? Whether it's epoch in ms, nanos, seconds, etc. Or maybe not epoch at all, etc. Worst case; if you fail to give it a helpful name, maybe add a @param documentation instead? https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:45: /** Creates a RequestGenerator. */ Nit: {@link RequestGenerator} https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:49: void onRegisterNewRequest(long timestampRequest, long timestampPost) { } 1) It's a little bit unclear without reading the comment that this is about something having finished. How about adding a "Finished" suffix to the method name? 2) Nit: Same comment about timestamp as above. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:52: void onHandlePostRequest(int result, boolean installEventWasSent) { } 1) Finished-suffix to method name? 2) Should |result| be an int-def-enum thingy? https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:55: void onGenerateAndPostRequest(boolean result) { } 1) Finished-suffix to method name? 2) What does result mean? That something worked? Or that a result was received? Is it resultOk? Or something with a better name? Maybe |succeeded|? https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:58: void onSaveState(long timestampRequest, long timestampPost) { } Nit: Also timestamp-comment
https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:130: // TODO(dfalcantara): Prevent Omaha code from repeatedly rescheduling itself immediately in On 2017/02/01 08:29:36, nyquist wrote: > Should we also do some clamping? Maybe |>= 0| in case the code above fails > horribly? Added what I'm guessing you're suggesting. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:386: // Remove any repeating alarms in favor of the new scheduling setup on M58 and up. On 2017/02/01 08:29:36, nyquist wrote: > Nit: Could you extract this new block to its own method with an excellent name? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:42: /** Schedules the Service to run again at the given time. */ On 2017/02/01 08:29:36, nyquist wrote: > Nit: {@link Service} Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:43: abstract void scheduleService(Service service, long nextTimestamp); On 2017/02/01 08:29:36, nyquist wrote: > Could you add a suffix or something to nextTimestamp to give a hint to the > reader of the value type? Whether it's epoch in ms, nanos, seconds, etc. Or > maybe not epoch at all, etc. Worst case; if you fail to give it a helpful name, > maybe add a @param documentation instead? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:45: /** Creates a RequestGenerator. */ On 2017/02/01 08:29:36, nyquist wrote: > Nit: {@link RequestGenerator} Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:49: void onRegisterNewRequest(long timestampRequest, long timestampPost) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) It's a little bit unclear without reading the comment that this is about > something having finished. How about adding a "Finished" suffix to the method > name? > 2) Nit: Same comment about timestamp as above. Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:52: void onHandlePostRequest(int result, boolean installEventWasSent) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) Finished-suffix to method name? > 2) Should |result| be an int-def-enum thingy? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:55: void onGenerateAndPostRequest(boolean result) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) Finished-suffix to method name? > 2) What does result mean? That something worked? Or that a result was received? > Is it resultOk? Or something with a better name? Maybe |succeeded|? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:58: void onSaveState(long timestampRequest, long timestampPost) { } On 2017/02/01 08:29:36, nyquist wrote: > Nit: Also timestamp-comment Done.
https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:130: // TODO(dfalcantara): Prevent Omaha code from repeatedly rescheduling itself immediately in On 2017/02/01 08:29:36, nyquist wrote: > Should we also do some clamping? Maybe |>= 0| in case the code above fails > horribly? Added what I'm guessing you're suggesting. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:386: // Remove any repeating alarms in favor of the new scheduling setup on M58 and up. On 2017/02/01 08:29:36, nyquist wrote: > Nit: Could you extract this new block to its own method with an excellent name? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:42: /** Schedules the Service to run again at the given time. */ On 2017/02/01 08:29:36, nyquist wrote: > Nit: {@link Service} Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:43: abstract void scheduleService(Service service, long nextTimestamp); On 2017/02/01 08:29:36, nyquist wrote: > Could you add a suffix or something to nextTimestamp to give a hint to the > reader of the value type? Whether it's epoch in ms, nanos, seconds, etc. Or > maybe not epoch at all, etc. Worst case; if you fail to give it a helpful name, > maybe add a @param documentation instead? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:45: /** Creates a RequestGenerator. */ On 2017/02/01 08:29:36, nyquist wrote: > Nit: {@link RequestGenerator} Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:49: void onRegisterNewRequest(long timestampRequest, long timestampPost) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) It's a little bit unclear without reading the comment that this is about > something having finished. How about adding a "Finished" suffix to the method > name? > 2) Nit: Same comment about timestamp as above. Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:52: void onHandlePostRequest(int result, boolean installEventWasSent) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) Finished-suffix to method name? > 2) Should |result| be an int-def-enum thingy? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:55: void onGenerateAndPostRequest(boolean result) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) Finished-suffix to method name? > 2) What does result mean? That something worked? Or that a result was received? > Is it resultOk? Or something with a better name? Maybe |succeeded|? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:58: void onSaveState(long timestampRequest, long timestampPost) { } On 2017/02/01 08:29:36, nyquist wrote: > Nit: Also timestamp-comment Done.
https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:130: // TODO(dfalcantara): Prevent Omaha code from repeatedly rescheduling itself immediately in On 2017/02/01 08:29:36, nyquist wrote: > Should we also do some clamping? Maybe |>= 0| in case the code above fails > horribly? Added what I'm guessing you're suggesting. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaClient.java:386: // Remove any repeating alarms in favor of the new scheduling setup on M58 and up. On 2017/02/01 08:29:36, nyquist wrote: > Nit: Could you extract this new block to its own method with an excellent name? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java (right): https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:42: /** Schedules the Service to run again at the given time. */ On 2017/02/01 08:29:36, nyquist wrote: > Nit: {@link Service} Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:43: abstract void scheduleService(Service service, long nextTimestamp); On 2017/02/01 08:29:36, nyquist wrote: > Could you add a suffix or something to nextTimestamp to give a hint to the > reader of the value type? Whether it's epoch in ms, nanos, seconds, etc. Or > maybe not epoch at all, etc. Worst case; if you fail to give it a helpful name, > maybe add a @param documentation instead? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:45: /** Creates a RequestGenerator. */ On 2017/02/01 08:29:36, nyquist wrote: > Nit: {@link RequestGenerator} Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:49: void onRegisterNewRequest(long timestampRequest, long timestampPost) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) It's a little bit unclear without reading the comment that this is about > something having finished. How about adding a "Finished" suffix to the method > name? > 2) Nit: Same comment about timestamp as above. Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:52: void onHandlePostRequest(int result, boolean installEventWasSent) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) Finished-suffix to method name? > 2) Should |result| be an int-def-enum thingy? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:55: void onGenerateAndPostRequest(boolean result) { } On 2017/02/01 08:29:36, nyquist wrote: > 1) Finished-suffix to method name? > 2) What does result mean? That something worked? Or that a result was received? > Is it resultOk? Or something with a better name? Maybe |succeeded|? Done. https://codereview.chromium.org/2665133005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omaha/OmahaDelegate.java:58: void onSaveState(long timestampRequest, long timestampPost) { } On 2017/02/01 08:29:36, nyquist wrote: > Nit: Also timestamp-comment Done.
The CQ bit was checked by dfalcantara@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/2665133005/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java (right): https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:57: MockOmahaDelegate(Context context, DeviceType deviceType, boolean installedOnSystemImage) { Pretty plz I can haz private enum for installedOnSystemImage? https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:119: mTimestampsOnSaveState = new Pair<Long, Long>(nextRequestTimestamp, nextPostTimestamp); This is fine, but do you think it's more readable with named members instead of .first and .second? I don't feel strongly, as the comments in the test always explain. But any .first / .second makes me want to look and verify this one line every time, because I have the memory span of a goldfish. https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:324: final long time_generated_request = 0L; o_O https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:410: final long time_generated_request = 0L; O_o https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:413: final long time_register_new_request = OmahaClient.MS_BETWEEN_REQUESTS; This timestamp is something like 5 hours, but it's a little bit confusing, since I have no reference point to the values above (now, etc.). Should this be an increment of one of the previous values instead? https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:449: assertEquals(now + OmahaClient.MS_POST_BASE_DELAY, So we're skipping checkTimestamps here because we don't want to investigate the timestamp for the next request? https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:486: assertEquals(now + OmahaClient.MS_BETWEEN_REQUESTS, So we're skipping checkTimestamps here because we intentionally don't want to check when the next request is? https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:533: private void checkTimestamps(Pair<Long, Long> timestamps, long expectedRequestTimestamp, Not 100% sure about this, but for other things we have the expected part first. But then again, this is a wrapper method, and has a pair, etc. So I'm really not sure. Maybe just think once more about what you think, and go with either what you have or swap? I don't feel strongly either way. Also, since you're not using mDelegate here, static? Or maybe not pass in the value from mDelegate, but keep this as normal member method and just access mDelegate directly?
The CQ bit was checked by dfalcantara@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...
ptal https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java (right): https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:57: MockOmahaDelegate(Context context, DeviceType deviceType, boolean installedOnSystemImage) { On 2017/02/07 19:49:25, nyquist wrote: > Pretty plz I can haz private enum for installedOnSystemImage? Done. https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:119: mTimestampsOnSaveState = new Pair<Long, Long>(nextRequestTimestamp, nextPostTimestamp); On 2017/02/07 19:49:25, nyquist wrote: > This is fine, but do you think it's more readable with named members instead of > .first and .second? I don't feel strongly, as the comments in the test always > explain. But any .first / .second makes me want to look and verify this one line > every time, because I have the memory span of a goldfish. Done. https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:324: final long time_generated_request = 0L; On 2017/02/07 19:49:25, nyquist wrote: > o_O Yay accidental lowercasing after lint complained about local final long CAPS_NAMING. https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:410: final long time_generated_request = 0L; On 2017/02/07 19:49:25, nyquist wrote: > O_o -_- https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:413: final long time_register_new_request = OmahaClient.MS_BETWEEN_REQUESTS; On 2017/02/07 19:49:25, nyquist wrote: > This timestamp is something like 5 hours, but it's a little bit confusing, since > I have no reference point to the values above (now, etc.). Should this be an > increment of one of the previous values instead? now adds to timeGeneratedRequest https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:449: assertEquals(now + OmahaClient.MS_POST_BASE_DELAY, On 2017/02/07 19:49:25, nyquist wrote: > So we're skipping checkTimestamps here because we don't want to investigate the > timestamp for the next request? We could, but it's not important for this test. https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:486: assertEquals(now + OmahaClient.MS_BETWEEN_REQUESTS, On 2017/02/07 19:49:25, nyquist wrote: > So we're skipping checkTimestamps here because we intentionally don't want to > check when the next request is? Yeah, this is really just checking that the request timestamp gets reset, which means that a request is generated and then the next thing should be a post. I've added the check anyway. https://codereview.chromium.org/2665133005/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/omaha/OmahaClientTest.java:533: private void checkTimestamps(Pair<Long, Long> timestamps, long expectedRequestTimestamp, On 2017/02/07 19:49:25, nyquist wrote: > Not 100% sure about this, but for other things we have the expected part first. > But then again, this is a wrapper method, and has a pair, etc. So I'm really not > sure. Maybe just think once more about what you think, and go with either what > you have or swap? I don't feel strongly either way. > > Also, since you're not using mDelegate here, static? Or maybe not pass in the > value from mDelegate, but keep this as normal member method and just access > mDelegate directly? Can't just use mDelegate directly because mDelegate stores two different timestamps. I'll flip the expected values around.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486511280194860,
"parent_rev": "0591e6b5e4e2ded7d48edc18e20a6ed2466fd19a", "commit_rev":
"c9625f069b73578ab45f49be70a830d48f6141bc"}
Message was sent while issue was closed.
Description was changed from ========== [Omaha] Make the whole pipeline run each time * Instead of making OmahaClient run as an IntentService that handles individual actions, run the whole pipeline, including registering new requests and POSTing to the server. This has several advantages, including that the pipeline is somewhat easier to understand. * The pipeline now uses one single-shot alarm to determine when to either generate a new active user request or to trigger a POST. This requires canceling existing repeating alarms. * Redoes the "isOverude" check in handleRegisterActiveRequest to just check current timestamps instead of also checking that a request doesn't exist. This is more logical for an "isOverdue" check, and prevents the OmahaClient from trying to reschedule itself repeatedly until an existing request's timestamps are screwed up. This should be alright because the request timestamp and the timestamp for the new request should match in all cases where the SharedPrerences are valid. * Introduces the OmahaDelegate, which masks out calls to various classes that OmahaClient requires. * Replaces all old OmahaClientTests that tested the old pipeline with one that is more black-boxy and tests the new pipeline. Significant cleanups are still required here, but the pipeline will be changing further and basic tests are better than no tests. BUG=680817 ========== to ========== [Omaha] Make the whole pipeline run each time * Instead of making OmahaClient run as an IntentService that handles individual actions, run the whole pipeline, including registering new requests and POSTing to the server. This has several advantages, including that the pipeline is somewhat easier to understand. * The pipeline now uses one single-shot alarm to determine when to either generate a new active user request or to trigger a POST. This requires canceling existing repeating alarms. * Redoes the "isOverude" check in handleRegisterActiveRequest to just check current timestamps instead of also checking that a request doesn't exist. This is more logical for an "isOverdue" check, and prevents the OmahaClient from trying to reschedule itself repeatedly until an existing request's timestamps are screwed up. This should be alright because the request timestamp and the timestamp for the new request should match in all cases where the SharedPrerences are valid. * Introduces the OmahaDelegate, which masks out calls to various classes that OmahaClient requires. * Replaces all old OmahaClientTests that tested the old pipeline with one that is more black-boxy and tests the new pipeline. Significant cleanups are still required here, but the pipeline will be changing further and basic tests are better than no tests. BUG=680817 Review-Url: https://codereview.chromium.org/2665133005 Cr-Commit-Position: refs/heads/master@{#448803} Committed: https://chromium.googlesource.com/chromium/src/+/c9625f069b73578ab45f49be70a8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c9625f069b73578ab45f49be70a8...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2685053002/ by nyquist@chromium.org. The reason for reverting is: Causes crashes on startup in some cases.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
