Delete DownloadActionDelegate::SetDownloadStatus() method.
This method is only used by the caller to detect a programming error in
the DownloadAction (calling BytesReceived when not downloading) and log
a message. This patch removes the method from the delegate interface
and makes sure it doesn't issue a call to BytesReceived when not
activelly downloading.
Bug: 25773375
TEST=FEATURES=test emerge-link update_engine
Change-Id: I8ff5c53f1fd40c0777b3f6523703a8bee71c019d
diff --git a/payload_consumer/download_action.cc b/payload_consumer/download_action.cc
index 0136f49..58e29b2 100644
--- a/payload_consumer/download_action.cc
+++ b/payload_consumer/download_action.cc
@@ -186,9 +186,7 @@
&install_plan_));
writer_ = delta_performer_.get();
}
- if (delegate_) {
- delegate_->SetDownloadStatus(true); // Set to active.
- }
+ download_active_= true;
if (system_state_ != nullptr) {
const PayloadStateInterface* payload_state = system_state_->payload_state();
@@ -236,9 +234,7 @@
writer_->Close();
writer_ = nullptr;
}
- if (delegate_) {
- delegate_->SetDownloadStatus(false); // Set to inactive.
- }
+ download_active_= false;
CloseP2PSharingFd(false); // Keep p2p file.
// Terminates the transfer. The action is terminated, if necessary, when the
// TransferTerminated callback is received.
@@ -258,7 +254,7 @@
}
bytes_received_ += length;
- if (delegate_)
+ if (delegate_ && download_active_)
delegate_->BytesReceived(bytes_received_, install_plan_.payload_size);
if (writer_ && !writer_->Write(bytes, length, &code_)) {
LOG(ERROR) << "Error " << code_ << " in DeltaPerformer's Write method when "
@@ -288,9 +284,7 @@
LOG_IF(WARNING, writer_->Close() != 0) << "Error closing the writer.";
writer_ = nullptr;
}
- if (delegate_) {
- delegate_->SetDownloadStatus(false); // Set to inactive.
- }
+ download_active_= false;
ErrorCode code =
successful ? ErrorCode::kSuccess : ErrorCode::kDownloadTransferError;
if (code == ErrorCode::kSuccess && delta_performer_.get()) {
diff --git a/payload_consumer/download_action.h b/payload_consumer/download_action.h
index c0f0688..e57ffb3 100644
--- a/payload_consumer/download_action.h
+++ b/payload_consumer/download_action.h
@@ -42,15 +42,9 @@
public:
virtual ~DownloadActionDelegate() = default;
- // Called right before starting the download with |active| set to
- // true. Called after completing the download with |active| set to
- // false.
- virtual void SetDownloadStatus(bool active) = 0;
-
- // Called periodically after bytes are received. This method will be
- // invoked only if the download is active. |bytes_received| is the
- // number of bytes downloaded thus far. |total| is the number of
- // bytes expected.
+ // Called periodically after bytes are received. This method will be invoked
+ // only if the DownloadAction is running. |bytes_received| is the number of
+ // bytes downloaded thus far. |total| is the number of bytes expected.
virtual void BytesReceived(uint64_t bytes_received, uint64_t total) = 0;
};
@@ -142,6 +136,7 @@
// For reporting status to outsiders
DownloadActionDelegate* delegate_;
uint64_t bytes_received_;
+ bool download_active_{false};
// The file-id for the file we're sharing or the empty string
// if we're not using p2p to share.
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index 3bef196..901dbd6 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -62,7 +62,6 @@
namespace {
class DownloadActionDelegateMock : public DownloadActionDelegate {
public:
- MOCK_METHOD1(SetDownloadStatus, void(bool active));
MOCK_METHOD2(BytesReceived, void(uint64_t bytes_received, uint64_t total));
};
@@ -177,12 +176,10 @@
if (use_download_delegate) {
InSequence s;
download_action.set_delegate(&download_delegate);
- EXPECT_CALL(download_delegate, SetDownloadStatus(true)).Times(1);
if (data.size() > kMockHttpFetcherChunkSize)
EXPECT_CALL(download_delegate,
BytesReceived(1 + kMockHttpFetcherChunkSize, _));
EXPECT_CALL(download_delegate, BytesReceived(_, _)).Times(AtLeast(1));
- EXPECT_CALL(download_delegate, SetDownloadStatus(false)).Times(1);
}
ErrorCode expected_code = ErrorCode::kSuccess;
if (fail_write > 0)
@@ -291,10 +288,8 @@
download_action.SetTestFileWriter(&writer);
DownloadActionDelegateMock download_delegate;
if (use_download_delegate) {
- InSequence s;
download_action.set_delegate(&download_delegate);
- EXPECT_CALL(download_delegate, SetDownloadStatus(true)).Times(1);
- EXPECT_CALL(download_delegate, SetDownloadStatus(false)).Times(1);
+ EXPECT_CALL(download_delegate, BytesReceived(_, _)).Times(0);
}
TerminateEarlyTestProcessorDelegate delegate;
ActionProcessor processor;
diff --git a/update_attempter.cc b/update_attempter.cc
index ce2acbb..df29a0d 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -1036,16 +1036,7 @@
}
}
-void UpdateAttempter::SetDownloadStatus(bool active) {
- download_active_ = active;
- LOG(INFO) << "Download status: " << (active ? "active" : "inactive");
-}
-
void UpdateAttempter::BytesReceived(uint64_t bytes_received, uint64_t total) {
- if (!download_active_) {
- LOG(ERROR) << "BytesReceived called while not downloading.";
- return;
- }
double progress = static_cast<double>(bytes_received) /
static_cast<double>(total);
// Self throttle based on progress. Also send notifications if
diff --git a/update_attempter.h b/update_attempter.h
index b08b8a8..c0e8de0 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -148,8 +148,7 @@
// UPDATED_NEED_REBOOT. Returns true on sucess, false otherwise.
bool RebootIfNeeded();
- // DownloadActionDelegate methods
- void SetDownloadStatus(bool active) override;
+ // DownloadActionDelegate method.
void BytesReceived(uint64_t bytes_received, uint64_t total) override;
// Broadcasts the current status over D-Bus.
@@ -418,10 +417,6 @@
brillo::MessageLoop::TaskId manage_shares_id_{
brillo::MessageLoop::kTaskIdNull};
- // Set to true if an update download is active (and BytesReceived
- // will be called), set to false otherwise.
- bool download_active_ = false;
-
// For status:
UpdateStatus status_{UpdateStatus::IDLE};
double download_progress_ = 0.0;
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index e549e38..1a61f7c 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -135,7 +135,6 @@
EXPECT_EQ(0, attempter_.http_response_code_);
EXPECT_EQ(utils::kCpuSharesNormal, attempter_.shares_);
EXPECT_EQ(MessageLoop::kTaskIdNull, attempter_.manage_shares_id_);
- EXPECT_FALSE(attempter_.download_active_);
EXPECT_EQ(UpdateStatus::IDLE, attempter_.status_);
EXPECT_EQ(0.0, attempter_.download_progress_);
EXPECT_EQ(0, attempter_.last_checked_time_);