Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust the message for Roll-forward update #4540

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Adjust the message for Roll-forward update #4540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions chrome/updater/updater_module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "cobalt/browser/switches.h"
#include "components/crx_file/crx_verifier.h"
#include "components/update_client/cobalt_slot_management.h"
#include "components/update_client/update_client_errors.h"
#include "components/update_client/utils.h"
#include "starboard/configuration_constants.h"
#include "starboard/extension/installation_manager.h"
Expand All @@ -46,6 +47,7 @@ namespace {

using update_client::CobaltSlotManagement;
using update_client::ComponentState;
using update_client::UpdateCheckError;

// The SHA256 hash of the "cobalt_evergreen_public" key.
constexpr uint8_t kCobaltPublicKeyHash[] = {
Expand Down Expand Up @@ -98,17 +100,24 @@ void Observer::OnEvent(Events event, const std::string& id) {
status = "Status is unknown.";
} else if (crx_update_item_.state == ComponentState::kUpToDate &&
updater_configurator_->GetPreviousUpdaterStatus().compare(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might have brought this up, but since this is a string comparison, using compare() is strange (made me feel this was not a string, but a special object with special comparison). So I'd say:

    updater_configurator_->GetPreviousUpdaterStatus() == 
        updater_status_string_map.at(UpdaterStatus::kUpdated)

is more idiomatic.

updater_status_string_map.find(UpdaterStatus::kUpdated)
->second) == 0) {
updater_status_string_map.at(UpdaterStatus::kUpdated)) == 0) {
status = std::string(
updater_status_string_map.find(UpdaterStatus::kUpdated)->second);
updater_status_string_map.at(UpdaterStatus::kUpdated));
} else {
status = std::string(
updater_status_string_map.find(status_iterator->second)->second);
}
if (crx_update_item_.state == ComponentState::kUpdateError) {
status +=
", error code is " + std::to_string(crx_update_item_.error_code);
// QUICK_ROLL_FORWARD update, adjust the message to "Update installed,
// pending restart"
if (crx_update_item_.error_code == static_cast<int>(UpdateCheckError::QUICK_ROLL_FORWARD)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line is longer than 80 characters, is that allowed?

status = std::string(
updater_status_string_map.at(UpdaterStatus::kUpdated));
} else {
status +=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the error category was good, but it'd be even better to add a string to each error category and error code. (That's what I meant with "stringification", not just turn an int into a string). This would come at a binary size cost, but that's your tradeoff, since this would only print numbers which then someone would have to convert to human-readable messages (and those messages/enums might change per versions).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. As I mentioned, the error codes are from multiple sources. It's a bit complicated to stringify each error code to get a meaningful string. The Eng team is the main consumer of these codes, and QA/Partners just report them back to us for investigation. The Eng team has to look into the source code to understand the issue anyway, and we have not had difficulty understanding the error codes.

", error category is " + std::to_string(static_cast<int>(crx_update_item_.error_category)) +
", error code is " + std::to_string(crx_update_item_.error_code);
}
}
if (updater_notification_ext_ != nullptr) {
updater_notification_ext_->UpdaterState(
Expand Down
Loading