Adjust the message for Roll-forward update #4540

yuying-y · 2024-12-06T00:11:11Z

When roll-forward update happens, adjust the message to "Update installed, pending restart", as it's more appropriate than the previous message "Failed to update, error code is 21".

b/382561808

yell0wd0g · 2024-12-06T05:12:19Z

chrome/updater/updater_module.cc

+      // pending restart"
+      if (crx_update_item_.error_code == 21) {
+        status = std::string(
+            updater_status_string_map.find(UpdaterStatus::kUpdated)->second);


If |updater_status_string_map| is a map, why not just do here updater_status_string_map[UpdaterStatus::kUpdated] ?

Thanks for the suggestion. Similar to the single-line statement w/ or w/o braces, I saw both styles in our code base:

Using ->second: https://source.corp.google.com/search?q=%22-%3Esecond%22&sq=repo:lbshell-internal%2Fcobalt_src%20AND%20branch:25.lts.1%2B

Using map[: https://source.corp.google.com/search?q=%22map%5B%22&sq=repo:lbshell-internal%2Fcobalt_src%20AND%20branch:25.lts.1%2B

I don't have a strong preference of one way to another. If you strongly suggest the other way, I can change this reference in this file. But to systematically improve consistency about the usage, we should use Deco Tech Talk to align on the style, clearly document which style is preferred and use follow-up CLs to normalize the usage/format.

I personally prefer the second (operator[]); the find()->second makes a map look like is a normal container filled up with pairs (this happens to be often the implementation, but this should be abstracted away). It's true that operator[] performs an insertion if the key is not present (which I think it's a huge mistake by STL), in that case std::map::at() is still more semantically "meaningful" than find().
FTR the only cases of direct find() I remember in Chromium are those when both the key and the value are needed.

OTOH, do I hear you volunteering to present this question to cobalt in a document or (mini) style tech talk? : )

Thanks for the explanation. Makes sense, I'll use operator[] or std::map::at() here. Since the function already checks if the passed in map key exists, it'll be fine to use both.

About code styles, I found this Cobalt page: https://g3doc.corp.google.com/company/teams/cobalt/team/coding_style.md?cl=head. I'll see how to incorporate our discussion here.

yell0wd0g

Where're the unit test for this new behaviour?

yuying-y · 2024-12-07T00:10:10Z

Where're the unit test for this new behaviour?

Same to #4521 (comment). The unit tests on this file is not enabled yet in https://source.corp.google.com/h/lbshell-internal/cobalt_src/+/COBALT:chrome/updater/BUILD.gn;l=108. It's tracked as a P2 task to enable all unit tests of the updater of Chrobalt in the 2025 workstreams doc.

yell0wd0g · 2024-12-07T05:43:10Z

Where're the unit test for this new behaviour?

Same to #4521 (comment). The unit tests on this file is not enabled yet in https://source.corp.google.com/h/lbshell-internal/cobalt_src/+/COBALT:chrome/updater/BUILD.gn;l=108. It's tracked as a P2 task to enable all unit tests of the updater of Chrobalt in the 2025 workstreams doc.

There might be plans for larger test/coverage improvement but that should not prevent this CL from adding a small unittest file to test the new behaviour (only). IOW, we shouldn't let the perfect be the enemy of the good; a bit of testing is better than no testing. And as a corollary, I think is less bad if there are some unittests that are not run by CI (which would be the case here IIUC), than having no unittests at all.

In general writing test in any code delta brings more clarity to the code review and the landed code, so I think it's totally worth it.

yell0wd0g · 2024-12-10T00:33:26Z

chrome/updater/updater_module.cc

-          ", 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 == 21) {


It took me a while, but is this 21 the one we're talking about?

https://source.corp.google.com/h/lbshell-internal/cobalt_src/+/main:components/update_client/update_client_errors.h;l=74;drc=ac02e200288bef9c817fad9b06763463e4e9493d

If so, please use the enum. If that's not the 21, try to use the original value, and if that's not defined ( maybe is from the OS), at least define a constexpr int kBlaBla = 21.

Good point! Done.

chrome/updater/updater_module.cc

yuying-y · 2024-12-10T01:53:15Z

Where're the unit test for this new behaviour?

Same to #4521 (comment). The unit tests on this file is not enabled yet in source.corp.google.com/h/lbshell-internal/cobalt_src/+/COBALT:chrome/updater/BUILD.gn;l=108. It's tracked as a P2 task to enable all unit tests of the updater of Chrobalt in the 2025 workstreams doc.

There might be plans for larger test/coverage improvement but that should not prevent this CL from adding a small unittest file to test the new behaviour (only). IOW, we shouldn't let the perfect be the enemy of the good; a bit of testing is better than no testing. And as a corollary, I think is less bad if there are some unittests that are not run by CI (which would be the case here IIUC), than having no unittests at all.

In general writing test in any code delta brings more clarity to the code review and the landed code, so I think it's totally worth it.

Thanks for the comment. I agree that some tests is better than no tests, but I really don't think it's wise to do throw-away work considering that Chrobalt may change how the updater works drastically, and also this new behavior to change a status message is not a critical risky feature that may break the app.

yell0wd0g · 2024-12-10T03:40:50Z

Where're the unit test for this new behaviour?

Same to #4521 (comment). The unit tests on this file is not enabled yet in source.corp.google.com/h/lbshell-internal/cobalt_src/+/COBALT:chrome/updater/BUILD.gn;l=108. It's tracked as a P2 task to enable all unit tests of the updater of Chrobalt in the 2025 workstreams doc.

There might be plans for larger test/coverage improvement but that should not prevent this CL from adding a small unittest file to test the new behaviour (only). IOW, we shouldn't let the perfect be the enemy of the good; a bit of testing is better than no testing. And as a corollary, I think is less bad if there are some unittests that are not run by CI (which would be the case here IIUC), than having no unittests at all.
In general writing test in any code delta brings more clarity to the code review and the landed code, so I think it's totally worth it.

Thanks for the comment. I agree that some tests is better than no tests, but I really don't think it's wise to do throw-away work considering that Chrobalt may change how the updater works drastically, and also this new behavior to change a status message is not a critical risky feature that may break the app.

That makes more sense: there's no cobalt_unittests and this is landing on a dead-end branch, so landing unit tests would be little ROI. That argument bears more weight to me than the others :)

yell0wd0g

LGTM with a question for careful consideration.

yell0wd0g · 2024-12-11T20:03:39Z

chrome/updater/updater_module.cc

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


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).

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.

When roll-forward update happens, adjust the message to "Update installed, pending restart", as it's more appropriate than the previous message "Failed to update, error code is 21". b/382561808 Change-Id: I72a6e31052f3894c53ea1e3344c19b910764a205

Created b/383396304 to track the request, as it's out of scope of this PR and the target bug b/382561808.

yell0wd0g · 2024-12-12T19:06:56Z

chrome/updater/updater_module.cc

@@ -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(


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.

yell0wd0g · 2024-12-12T19:08:11Z

chrome/updater/updater_module.cc

-          ", 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)) {


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

yuying-y requested review from johnxwork and yjzhang111 December 6, 2024 00:14

yell0wd0g reviewed Dec 6, 2024

View reviewed changes

yell0wd0g requested changes Dec 6, 2024

View reviewed changes

yuying-y force-pushed the roll_forward branch 2 times, most recently from c2aaca6 to db33c21 Compare December 10, 2024 00:16

yell0wd0g requested changes Dec 10, 2024

View reviewed changes

yuying-y force-pushed the roll_forward branch from db33c21 to bac0320 Compare December 10, 2024 02:07

yuying-y force-pushed the roll_forward branch from bac0320 to db6f0c6 Compare December 11, 2024 01:23

yell0wd0g previously requested changes Dec 11, 2024

View reviewed changes

yuying-y requested a review from yell0wd0g December 12, 2024 00:08

yuying-y force-pushed the roll_forward branch from db6f0c6 to 9cc8fec Compare December 12, 2024 00:18

johnxwork approved these changes Dec 12, 2024

View reviewed changes

yuying-y merged commit 8986f16 into youtube:25.lts.1+ Dec 12, 2024
300 of 301 checks passed

yell0wd0g reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust the message for Roll-forward update #4540

Adjust the message for Roll-forward update #4540

yuying-y commented Dec 6, 2024

yell0wd0g Dec 6, 2024

yuying-y Dec 7, 2024

yell0wd0g Dec 7, 2024

yuying-y Dec 9, 2024

yell0wd0g left a comment

yuying-y commented Dec 7, 2024

yell0wd0g commented Dec 7, 2024

yell0wd0g Dec 10, 2024

yuying-y Dec 10, 2024

yuying-y commented Dec 10, 2024

yell0wd0g commented Dec 10, 2024

yell0wd0g left a comment

yell0wd0g Dec 11, 2024

yuying-y Dec 11, 2024

yell0wd0g Dec 12, 2024

yell0wd0g Dec 12, 2024

Adjust the message for Roll-forward update #4540

Adjust the message for Roll-forward update #4540

Conversation

yuying-y commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yell0wd0g left a comment

Choose a reason for hiding this comment

yuying-y commented Dec 7, 2024

yell0wd0g commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuying-y commented Dec 10, 2024

yell0wd0g commented Dec 10, 2024

yell0wd0g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment