Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754023002:
Add a throttle wrapper to OfflineContentProvider (Closed)
|
Created:
3 years, 9 months ago by David Trainor- moved to gerrit Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a throttle wrapper to OfflineContentProvider
This adds a class that supports automatically throttling updates driven
by OfflineContentProviders. This allows the UI to limit the amount of
updates sent by underlying providers to a reasonable rate.
BUG=691805
Review-Url: https://codereview.chromium.org/2754023002
Cr-Commit-Position: refs/heads/master@{#458994}
Committed: https://chromium.googlesource.com/chromium/src/+/cd5c1457b96b49938e484359839a5e62b13ac5c4
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed comments #
Total comments: 2
Patch Set 3 : (Hopefully) fix DCHECK and sad test compile error #Patch Set 4 : Rebased #
Depends on Patchset: Messages
Total messages: 34 (20 generated)
dtrainor@chromium.org changed reviewers: + fgorski@chromium.org, qinmin@chromium.org
ptal thanks! doesn't hook up to any particular provider yet. I'm planning on putting it in between the aggregator and the java layer to throttle all updates from all providers the same way.
mostly looks good. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider.cc (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:32: wrapped_provider_->AddObserver(this); DCHECK(wrapped_provider_); above, please. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:80: OfflineContentProvider::Observer* observer) { dcheck(observer), please. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:97: OfflineContentProvider* provider) { you could check that wrapped_provider_ == provider. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:145: for (auto item_pair : updates) nit, add {} because you have 2 lines below. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider.h (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:28: ThrottledOfflineContentProvider(OfflineContentProvider* provider); mark explicit, please https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:40: // These two OfflineItem getter methods have a side effect. If there are any nit: two spaces after period to match the style of file. Or reverse everywhere. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:41: // updates pending for |id| the contents of that pending update will be I don't understand this document. Can we iterate on it a bit until it gets simpler/more understandable? https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider_unittest.cc (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. drop the (c) https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider_unittest.cc:71: EXPECT_EQ(false, provider_.AreItemsAvailable()); is there a good reason not to use EXPECT_TRUE and EXPECT_FALSE?
The CQ bit was checked by dtrainor@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/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider.cc (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:32: wrapped_provider_->AddObserver(this); On 2017/03/17 04:03:19, fgorski wrote: > DCHECK(wrapped_provider_); above, please. I've been told to remove DCHECK()s before dereferencing a value, so I'm always unsure whether or not to add them. But I'm happy to add them :). https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:80: OfflineContentProvider::Observer* observer) { On 2017/03/17 04:03:19, fgorski wrote: > dcheck(observer), please. Done. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:97: OfflineContentProvider* provider) { On 2017/03/17 04:03:19, fgorski wrote: > you could check that wrapped_provider_ == provider. Done as DCHECK_EQ. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.cc:145: for (auto item_pair : updates) On 2017/03/17 04:03:19, fgorski wrote: > nit, add {} because you have 2 lines below. Done. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider.h (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:28: ThrottledOfflineContentProvider(OfflineContentProvider* provider); On 2017/03/17 04:03:19, fgorski wrote: > mark explicit, please Done. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:40: // These two OfflineItem getter methods have a side effect. If there are any On 2017/03/17 04:03:19, fgorski wrote: > nit: two spaces after period to match the style of file. Or reverse everywhere. Done. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:41: // updates pending for |id| the contents of that pending update will be On 2017/03/17 04:03:19, fgorski wrote: > I don't understand this document. Can we iterate on it a bit until it gets > simpler/more understandable? Sorry just to confirm, you want me to iterate on the comment or the whole document? Updated the comment. Might be worth just getting rid of it since the behavior makes sense given that this class queues updates. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider_unittest.cc (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/17 04:03:19, fgorski wrote: > drop the (c) Done. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider_unittest.cc:71: EXPECT_EQ(false, provider_.AreItemsAvailable()); On 2017/03/17 04:03:19, fgorski wrote: > is there a good reason not to use EXPECT_TRUE and EXPECT_FALSE? d'oh! Not sure why I did this. Changing :).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
lgtm % comments. https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... File components/offline_items_collection/core/throttled_offline_content_provider.h (right): https://codereview.chromium.org/2754023002/diff/1/components/offline_items_co... components/offline_items_collection/core/throttled_offline_content_provider.h:41: // updates pending for |id| the contents of that pending update will be On 2017/03/20 06:27:53, David Trainor-ping if over 24h wrote: > On 2017/03/17 04:03:19, fgorski wrote: > > I don't understand this document. Can we iterate on it a bit until it gets > > simpler/more understandable? > > Sorry just to confirm, you want me to iterate on the comment or the whole > document? > > Updated the comment. Might be worth just getting rid of it since the behavior > makes sense given that this class queues updates. I meant the documentation (but my typing fu failed me). Updated comment looks ok. https://codereview.chromium.org/2754023002/diff/20001/components/offline_item... File components/offline_items_collection/core/throttled_offline_content_provider.cc (right): https://codereview.chromium.org/2754023002/diff/20001/components/offline_item... components/offline_items_collection/core/throttled_offline_content_provider.cc:100: DCHECK_EQ(this, wrapped_provider_); Are you sure this check is correct? You didn't use the input variable...
The CQ bit was checked by dtrainor@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.
lgtm
https://codereview.chromium.org/2754023002/diff/20001/components/offline_item... File components/offline_items_collection/core/throttled_offline_content_provider.cc (right): https://codereview.chromium.org/2754023002/diff/20001/components/offline_item... components/offline_items_collection/core/throttled_offline_content_provider.cc:100: DCHECK_EQ(this, wrapped_provider_); On 2017/03/20 20:51:08, fgorski wrote: > Are you sure this check is correct? You didn't use the input variable... O_o wow. tyvm
The CQ bit was checked by dtrainor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2754023002/#ps40001 (title: "(Hopefully) fix DCHECK and sad test compile error")
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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by dtrainor@chromium.org
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
Failed to apply patch for components/offline_items_collection/core/BUILD.gn:
While running git apply --index -p1;
error: patch failed: components/offline_items_collection/core/BUILD.gn:14
error: components/offline_items_collection/core/BUILD.gn: patch does not apply
Patch: components/offline_items_collection/core/BUILD.gn
Index: components/offline_items_collection/core/BUILD.gn
diff --git a/components/offline_items_collection/core/BUILD.gn
b/components/offline_items_collection/core/BUILD.gn
index
501adb8b8539272c1beb5bebb76c423d020a8a49..8337e702f4549e6024d255777d9b123703cf8d5a
100644
--- a/components/offline_items_collection/core/BUILD.gn
+++ b/components/offline_items_collection/core/BUILD.gn
@@ -14,11 +14,10 @@ static_library("core") {
"offline_content_provider.h",
"offline_item.cc",
"offline_item.h",
-
- # TODO(dtrainor): Generate the Java version of these enums when the Android
- # bridge is built.
"offline_item_filter.h",
"offline_item_state.h",
+ "throttled_offline_content_provider.cc",
+ "throttled_offline_content_provider.h",
]
public_deps = [
@@ -45,6 +44,7 @@ source_set("unit_tests") {
sources = [
"offline_content_aggregator_unittest.cc",
+ "throttled_offline_content_provider_unittest.cc",
]
deps = [
The CQ bit was checked by dtrainor@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2754023002/#ps60001 (title: "Rebased")
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 dtrainor@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": 1490235863803790,
"parent_rev": "1045a99b538d0dff67ebc514ceb1e850f6d60cde", "commit_rev":
"cd5c1457b96b49938e484359839a5e62b13ac5c4"}
Message was sent while issue was closed.
Description was changed from ========== Add a throttle wrapper to OfflineContentProvider This adds a class that supports automatically throttling updates driven by OfflineContentProviders. This allows the UI to limit the amount of updates sent by underlying providers to a reasonable rate. BUG=691805 ========== to ========== Add a throttle wrapper to OfflineContentProvider This adds a class that supports automatically throttling updates driven by OfflineContentProviders. This allows the UI to limit the amount of updates sent by underlying providers to a reasonable rate. BUG=691805 Review-Url: https://codereview.chromium.org/2754023002 Cr-Commit-Position: refs/heads/master@{#458994} Committed: https://chromium.googlesource.com/chromium/src/+/cd5c1457b96b49938e484359839a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/cd5c1457b96b49938e484359839a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
