Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2570433007:
[OfflinePages] Adds StartImmediateProcessing() api to RequestCoordinator (Closed)
|
Created:
4 years ago by dougarnett Modified:
4 years ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OfflinePages] Adds StartImmediateProcessing() api to RequestCoordinator
Adds api to Coordinator. Does not yet add Java bridge api support.
Also, cleans up some terminology wrt immediate*callback when not
necessarily immediate mode only.
BUG=672584
Committed: https://crrev.com/c2f0e3cf89272ee6a6511731a32008f4eede692f
Cr-Commit-Position: refs/heads/master@{#439496}
Patch Set 1 #Patch Set 2 : Some comment grammar fixes #
Total comments: 16
Patch Set 3 : Addressed a couple comments #Patch Set 4 : Naming tweak plus expanded comments from feedback #Patch Set 5 : Merge #
Total comments: 4
Patch Set 6 : SetInternalStartProcessingCallbackForTest name #
Messages
Total messages: 41 (24 generated)
The CQ bit was checked by dougarnett@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 dougarnett@chromium.org to run a CQ dry run
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org, romax@chromium.org
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.
lgtm https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:181: internal_processing_callback_(base::Bind(&EmptySchedulerCallback)), would this be better as immediate_processing_callback to be more parallel with the other uses of immediate? https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:577: if (device_conditions.GetNetConnectionType() == I think this should be using the C++ connection type instead of the Java type, at least until we get live device conditions from Java land. (As opposed to the fake conditions we are using now). https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:125: processing_callback_result_ = result; I think the old name is better here to carry the distinction between immediate and scheduled. maybe immediate_processing_callback is better still, because it makes sure that Immediate and Scheduled aren't in the same name. https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:443: TEST_F(RequestCoordinatorTest, StartImmediateProcessingStarted) { Maybe StartImmediateProcessingWithNoRequests ? https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:502: // Now trying to start processing on another request should return false. Should we have a second request in the queue before calling this?
not lgtm Sorry, I meant to hit the "publish" button, not the "Quick LGTM" button. I don't have any specific problems, I just want to hear the answers before I approve.
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:181: internal_processing_callback_(base::Bind(&EmptySchedulerCallback)), On 2016/12/13 21:38:41, Pete Williamson wrote: > would this be better as immediate_processing_callback to be more parallel with > the other uses of immediate? So the top level question here is whether or not we want to have callback passed into the StartImmediateProcessing() api. If not, then immediate_processing_callback is good name. But I do have a callback being passed in (in this CL iteration) so this member is for internally triggered processing which happens to be immediate but now there is different callback for externally triggered immediate processing. It's main purpose for being a member is to be overridden for testing so we should look at the SetInternalProcessingCallbackForTest() test method name too to decide on naming. I think that name is accurate but certainly true that it does not indicate the internally triggered processing happens to be immediate processing. I think SetImmediateProcessingCallbackForTest() would be misleading since it will not set the callback for externally triggered immediate processing. Combine the naming? SetInternalImmediateProcessingCallbackForTest() or SetImmediateInternalCallbackForTest() ? But really we should first settle on whether we want callback parm in new public api. https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:577: if (device_conditions.GetNetConnectionType() == On 2016/12/13 21:38:41, Pete Williamson wrote: > I think this should be using the C++ connection type instead of the Java type, > at least until we get live device conditions from Java land. (As opposed to the > fake conditions we are using now). The fake condition from the StartImmediatelyIfConnected() call is only fake wrt battery/power conditions, it provides network from GetConnectionType() so there is no functional difference here with that path. The difference is that now we provide an API that we can plumb with proper battery conditions as well. It felt wrong to take the DeviceConditions but then ignore them here with direct GetConnectionType(). We did find GetConnectionType() to sometimes be stale wrt scheduled path - I don't expect that to be the case for immediate calls - but I do wonder if Java is the fresher source. So I think we should take what is passed in. Or else remove DeviceConditions from the API would be an alternative. https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:125: processing_callback_result_ = result; On 2016/12/13 21:38:41, Pete Williamson wrote: > I think the old name is better here to carry the distinction between immediate > and scheduled. > > maybe immediate_processing_callback is better still, because it makes sure that > Immediate and Scheduled aren't in the same name. This function has been used with many StartScheduledProcessing() calls in these tests so it hasn't been "immediate" for some time. Now it is used in all 3 paths: StartScheduledProcessing(), StartImmediateProcessing(), and for intercepting internally triggered processing via RC::SetProcessingCallbackForTest() If we want to have separate callbacks for different paths, that will be a bit different change. https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:443: TEST_F(RequestCoordinatorTest, StartImmediateProcessingStarted) { On 2016/12/13 21:38:41, Pete Williamson wrote: > Maybe StartImmediateProcessingWithNoRequests ? Done. https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:502: // Now trying to start processing on another request should return false. On 2016/12/13 21:38:41, Pete Williamson wrote: > Should we have a second request in the queue before calling this? Fixed comment. We are really testing the logic about whether we will try to start processing (try to pick request) or not so don't actually need another request.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:181: internal_processing_callback_(base::Bind(&EmptySchedulerCallback)), On 2016/12/14 16:59:11, dougarnett wrote: > On 2016/12/13 21:38:41, Pete Williamson wrote: > > would this be better as immediate_processing_callback to be more parallel with > > the other uses of immediate? > > So the top level question here is whether or not we want to have callback passed > into the StartImmediateProcessing() api. If not, then > immediate_processing_callback is good name. But I do have a callback being > passed in (in this CL iteration) so this member is for internally triggered > processing which happens to be immediate but now there is different callback for > externally triggered immediate processing. > > It's main purpose for being a member is to be overridden for testing so we > should look at the SetInternalProcessingCallbackForTest() test method name too > to decide on naming. I think that name is accurate but certainly true that it > does not indicate the internally triggered processing happens to be immediate > processing. > > I think SetImmediateProcessingCallbackForTest() would be misleading since it > will not set the callback for externally triggered immediate processing. > > Combine the naming? > SetInternalImmediateProcessingCallbackForTest() > or > SetImmediateInternalCallbackForTest() > ? > > But really we should first settle on whether we want callback parm in new public > api. In that case, I misunderstood the intent of the callback. I was thinking we were having one top level callback for scheduled, and one for immediate. Let's chat in person to clear up my understanding of what this callback is for, then I'll have workable naming suggestions if I properly understand its purpose. https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:577: if (device_conditions.GetNetConnectionType() == On 2016/12/14 16:59:11, dougarnett wrote: > On 2016/12/13 21:38:41, Pete Williamson wrote: > > I think this should be using the C++ connection type instead of the Java type, > > at least until we get live device conditions from Java land. (As opposed to > the > > fake conditions we are using now). > > The fake condition from the StartImmediatelyIfConnected() call is only fake wrt > battery/power conditions, it provides network from GetConnectionType() so there > is no functional difference here with that path. The difference is that now we > provide an API that we can plumb with proper battery conditions as well. > > It felt wrong to take the DeviceConditions but then ignore them here with direct > GetConnectionType(). We did find GetConnectionType() to sometimes be stale wrt > scheduled path - I don't expect that to be the case for immediate calls - but I > do wonder if Java is the fresher source. So I think we should take what is > passed in. Or else remove DeviceConditions from the API would be an alternative. > > > Oh, I missed the fact that we were setting the connection type from the C++ code. In that case, this is fine as i. I think it is better to leave the device_conditions in because someday we plan to get them fresh from Java (but it will make this operation into an async chain.). https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:125: processing_callback_result_ = result; On 2016/12/14 16:59:11, dougarnett wrote: > On 2016/12/13 21:38:41, Pete Williamson wrote: > > I think the old name is better here to carry the distinction between immediate > > and scheduled. > > > > maybe immediate_processing_callback is better still, because it makes sure > that > > Immediate and Scheduled aren't in the same name. > > This function has been used with many StartScheduledProcessing() calls in these > tests so it hasn't been "immediate" for some time. Now it is used in all 3 > paths: StartScheduledProcessing(), StartImmediateProcessing(), and for > intercepting internally triggered processing via > RC::SetProcessingCallbackForTest() > > If we want to have separate callbacks for different paths, that will be a bit > different change. Let's discuss this in person too, so I correctly understand the purpose of the callback before suggesting the name.
https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:181: internal_processing_callback_(base::Bind(&EmptySchedulerCallback)), On 2016/12/14 18:32:54, Pete Williamson wrote: > On 2016/12/14 16:59:11, dougarnett wrote: > > On 2016/12/13 21:38:41, Pete Williamson wrote: > > > would this be better as immediate_processing_callback to be more parallel > with > > > the other uses of immediate? > > > > So the top level question here is whether or not we want to have callback > passed > > into the StartImmediateProcessing() api. If not, then > > immediate_processing_callback is good name. But I do have a callback being > > passed in (in this CL iteration) so this member is for internally triggered > > processing which happens to be immediate but now there is different callback > for > > externally triggered immediate processing. > > > > It's main purpose for being a member is to be overridden for testing so we > > should look at the SetInternalProcessingCallbackForTest() test method name too > > to decide on naming. I think that name is accurate but certainly true that it > > does not indicate the internally triggered processing happens to be immediate > > processing. > > > > I think SetImmediateProcessingCallbackForTest() would be misleading since it > > will not set the callback for externally triggered immediate processing. > > > > Combine the naming? > > SetInternalImmediateProcessingCallbackForTest() > > or > > SetImmediateInternalCallbackForTest() > > ? > > > > But really we should first settle on whether we want callback parm in new > public > > api. > > In that case, I misunderstood the intent of the callback. I was thinking we > were having one top level callback for scheduled, and one for immediate. Let's > chat in person to clear up my understanding of what this callback is for, then > I'll have workable naming suggestions if I properly understand its purpose. Perhaps the one above should be renamed from scheduler_callback_ to start_processing_callback_. This one could still be internal_processing_callback_ or perhaps internal_start_processing_callback_?
https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:181: internal_processing_callback_(base::Bind(&EmptySchedulerCallback)), On 2016/12/15 01:25:43, dougarnett wrote: > On 2016/12/14 18:32:54, Pete Williamson wrote: > > On 2016/12/14 16:59:11, dougarnett wrote: > > > On 2016/12/13 21:38:41, Pete Williamson wrote: > > > > would this be better as immediate_processing_callback to be more parallel > > with > > > > the other uses of immediate? > > > > > > So the top level question here is whether or not we want to have callback > > passed > > > into the StartImmediateProcessing() api. If not, then > > > immediate_processing_callback is good name. But I do have a callback being > > > passed in (in this CL iteration) so this member is for internally triggered > > > processing which happens to be immediate but now there is different callback > > for > > > externally triggered immediate processing. > > > > > > It's main purpose for being a member is to be overridden for testing so we > > > should look at the SetInternalProcessingCallbackForTest() test method name > too > > > to decide on naming. I think that name is accurate but certainly true that > it > > > does not indicate the internally triggered processing happens to be > immediate > > > processing. > > > > > > I think SetImmediateProcessingCallbackForTest() would be misleading since it > > > will not set the callback for externally triggered immediate processing. > > > > > > Combine the naming? > > > SetInternalImmediateProcessingCallbackForTest() > > > or > > > SetImmediateInternalCallbackForTest() > > > ? > > > > > > But really we should first settle on whether we want callback parm in new > > public > > > api. > > > > In that case, I misunderstood the intent of the callback. I was thinking we > > were having one top level callback for scheduled, and one for immediate. > Let's > > chat in person to clear up my understanding of what this callback is for, then > > I'll have workable naming suggestions if I properly understand its purpose. > > Perhaps the one above should be renamed from scheduler_callback_ to > start_processing_callback_. This one could still be > internal_processing_callback_ or perhaps internal_start_processing_callback_? I still like the name scheduler callback because it is used to return control to the scheduler. However, I'm fine with internal_start_processing_callback_ for the other one. Let's document it well in the header file what it is and what it does (and the fact that it exists to indirect whether or not we have a callback).
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2570433007/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:181: internal_processing_callback_(base::Bind(&EmptySchedulerCallback)), On 2016/12/15 01:39:03, Pete Williamson wrote: > On 2016/12/15 01:25:43, dougarnett wrote: > > On 2016/12/14 18:32:54, Pete Williamson wrote: > > > On 2016/12/14 16:59:11, dougarnett wrote: > > > > On 2016/12/13 21:38:41, Pete Williamson wrote: > > > > > would this be better as immediate_processing_callback to be more > parallel > > > with > > > > > the other uses of immediate? > > > > > > > > So the top level question here is whether or not we want to have callback > > > passed > > > > into the StartImmediateProcessing() api. If not, then > > > > immediate_processing_callback is good name. But I do have a callback being > > > > passed in (in this CL iteration) so this member is for internally > triggered > > > > processing which happens to be immediate but now there is different > callback > > > for > > > > externally triggered immediate processing. > > > > > > > > It's main purpose for being a member is to be overridden for testing so we > > > > should look at the SetInternalProcessingCallbackForTest() test method name > > too > > > > to decide on naming. I think that name is accurate but certainly true that > > it > > > > does not indicate the internally triggered processing happens to be > > immediate > > > > processing. > > > > > > > > I think SetImmediateProcessingCallbackForTest() would be misleading since > it > > > > will not set the callback for externally triggered immediate processing. > > > > > > > > Combine the naming? > > > > SetInternalImmediateProcessingCallbackForTest() > > > > or > > > > SetImmediateInternalCallbackForTest() > > > > ? > > > > > > > > But really we should first settle on whether we want callback parm in new > > > public > > > > api. > > > > > > In that case, I misunderstood the intent of the callback. I was thinking we > > > were having one top level callback for scheduled, and one for immediate. > > Let's > > > chat in person to clear up my understanding of what this callback is for, > then > > > I'll have workable naming suggestions if I properly understand its purpose. > > > > Perhaps the one above should be renamed from scheduler_callback_ to > > start_processing_callback_. This one could still be > > internal_processing_callback_ or perhaps internal_start_processing_callback_? > > I still like the name scheduler callback because it is used to return control to > the scheduler. However, I'm fine with internal_start_processing_callback_ for > the other one. Let's document it well in the header file what it is and what it > does (and the fact that it exists to indirect whether or not we have a > callback). Done - please check the member comment wording
lgtm https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:152: void SetInternalProcessingCallbackForTest( Nit: This might be better as SetInternalProcessingCallbackForTest()
https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:152: void SetInternalProcessingCallbackForTest( On 2016/12/15 17:52:42, Pete Williamson wrote: > Nit: This might be better as SetInternalProcessingCallbackForTest() Looks like you recommended same name, did you mean SetInternalStartProcessingCallbackForTest()?
https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:152: void SetInternalProcessingCallbackForTest( On 2016/12/15 18:07:27, dougarnett wrote: > On 2016/12/15 17:52:42, Pete Williamson wrote: > > Nit: This might be better as SetInternalProcessingCallbackForTest() > > Looks like you recommended same name, did you mean > SetInternalStartProcessingCallbackForTest()? Oops, yes, I meant to add the word "Start".
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2570433007/diff/80001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:152: void SetInternalProcessingCallbackForTest( On 2016/12/15 18:09:28, Pete Williamson wrote: > On 2016/12/15 18:07:27, dougarnett wrote: > > On 2016/12/15 17:52:42, Pete Williamson wrote: > > > Nit: This might be better as SetInternalProcessingCallbackForTest() > > > > Looks like you recommended same name, did you mean > > SetInternalStartProcessingCallbackForTest()? > > Oops, yes, I meant to add the word "Start". Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2570433007/#ps100001 (title: "SetInternalStartProcessingCallbackForTest name")
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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dougarnett@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": 100001, "attempt_start_ts": 1482165948694220,
"parent_rev": "a7e98b8d025ff1c94df698da862daac07a02a915", "commit_rev":
"616fbd5415c19ae971d86015417082e19de9a5df"}
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Adds StartImmediateProcessing() api to RequestCoordinator Adds api to Coordinator. Does not yet add Java bridge api support. Also, cleans up some terminology wrt immediate*callback when not necessarily immediate mode only. BUG=672584 ========== to ========== [OfflinePages] Adds StartImmediateProcessing() api to RequestCoordinator Adds api to Coordinator. Does not yet add Java bridge api support. Also, cleans up some terminology wrt immediate*callback when not necessarily immediate mode only. BUG=672584 Review-Url: https://codereview.chromium.org/2570433007 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Adds StartImmediateProcessing() api to RequestCoordinator Adds api to Coordinator. Does not yet add Java bridge api support. Also, cleans up some terminology wrt immediate*callback when not necessarily immediate mode only. BUG=672584 Review-Url: https://codereview.chromium.org/2570433007 ========== to ========== [OfflinePages] Adds StartImmediateProcessing() api to RequestCoordinator Adds api to Coordinator. Does not yet add Java bridge api support. Also, cleans up some terminology wrt immediate*callback when not necessarily immediate mode only. BUG=672584 Committed: https://crrev.com/c2f0e3cf89272ee6a6511731a32008f4eede692f Cr-Commit-Position: refs/heads/master@{#439496} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c2f0e3cf89272ee6a6511731a32008f4eede692f Cr-Commit-Position: refs/heads/master@{#439496} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
