| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
thank you Joshua!
[submodule "components/activity_reporter/internal"]What is this sorcery?
SEQUENCE_CHECKER(sequence_checker_);protected data members are not cool with the coding style: https://google.github.io/styleguide/cppguide.html#Access_Control
Maybe just define a sequence checker in all the derived classes, not sure how many they are.
~ActivityReporterImpl() override = default;Needed?
const base::Time now = base::Time::Now();time.h
ActivityReporterImplTest() = default;
~ActivityReporterImplTest() override = default;Needed?
std::unique_ptr<ActivityReporter> activity_reporter_;
scoped_refptr<MockUpdateClient> mock_update_client_;Can initialize here?
.Times(1)I typically elide this to avoid clutter but gtest docs suggests that it is useful for people to see the explicit Times(1).
EXPECT_EQ(call_count, 1);Do we need to if we have an expectation for the call?
TEST_F(ActivityReporterImplTest, ReportActive_Throttling) {Maybe tests can be combined.
~ActivityService() override = default;needed?
FROM_HERE, base::BindOnce(std::move(callback),<utility>
base::OnceCallback<void(const std::set<std::string>&)> callback)<set>
// GetActiveBits assumes activity, so there is nothing to clear.comment is not clear, also maybe ``.
return -2;maybe define constant and document?
std::unique_ptr<ActivityService> CreateActivityDataService() {motivation for factory function?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
[submodule "components/activity_reporter/internal"]Joshua PawlickiWhat is this sorcery?
I'm adding a new internal-only submodule to the repository. We already landed a change in it at https://chrome-internal-review.googlesource.com/c/activity_reporter_internal/+/8922519
protected data members are not cool with the coding style: https://google.github.io/styleguide/cppguide.html#Access_Control
Maybe just define a sequence checker in all the derived classes, not sure how many they are.
Done
~ActivityReporterImpl() override = default;Joshua PawlickiNeeded?
Done
const base::Time now = base::Time::Now();Joshua Pawlickitime.h
Done
const std::vector<std::optional<Joshua Pawlicki<optional>
Done
ActivityReporterImplTest() = default;
~ActivityReporterImplTest() override = default;Joshua PawlickiNeeded?
Done
std::unique_ptr<ActivityReporter> activity_reporter_;
scoped_refptr<MockUpdateClient> mock_update_client_;Joshua PawlickiCan initialize here?
Done
I typically elide this to avoid clutter but gtest docs suggests that it is useful for people to see the explicit Times(1).
Ack
Do we need to if we have an expectation for the call?
Ack
TEST_F(ActivityReporterImplTest, ReportActive_Throttling) {Maybe tests can be combined.
Done
~ActivityService() override = default;Joshua Pawlickineeded?
Done
FROM_HERE, base::BindOnce(std::move(callback),Joshua Pawlicki<utility>
Done
base::OnceCallback<void(const std::set<std::string>&)> callback)Joshua Pawlicki<set>
Done
// GetActiveBits assumes activity, so there is nothing to clear.comment is not clear, also maybe ``.
Done
maybe define constant and document?
Done
std::unique_ptr<ActivityService> CreateActivityDataService() {Joshua Pawlickimotivation for factory function?
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rohit, PTAL @ ios
Tommy, PTAL @ chrome/browser/browser_process_impl.cc
Thank you!
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
//chrome/browser/browser_process_impl.cc LGTM
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
//ios lgtm
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |