Updater: Add CancelInstalls RPC. [chromium/src : main]

Joshua Pawlicki (Gerrit)

unread,

Jul 1, 2022, 5:26:00 PM7/1/22

to mac-r...@chromium.org, Sorin Jianu, Xiaoling Bao, S. Ganesh, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Sorin Jianu.

Patch set 2:Auto-Submit +1

View Change

1 comment:

Patchset:
- Patch Set #2:
  Sorin, PTAL, thank you!

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

S. Ganesh (Gerrit)

unread,

Jul 1, 2022, 6:53:22 PM7/1/22

to Joshua Pawlicki, mac-r...@chromium.org, Sorin Jianu, Xiaoling Bao, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Joshua Pawlicki, Sorin Jianu.

View Change

5 comments:

File chrome/updater/app/server/mac/service_delegate.mm:
- Patch Set #2, Line 256: cancelInstallsWithAppID
  Perhaps `cancelInstallsForAppID`?
File chrome/updater/app/server/win/com_classes.cc:
- Patch Set #2, Line 383: kMaxStringLen
  I see `kMaxStringLen` is repeated multiple times in this file, perhaps just have a single constant declared for the file?
- Patch Set #2, Line 389: WideToUTF8
  Can `app_id` contain unicode characters, or is it going to be an ASCII string? If the latter, can use `base::WideToASCII` which would be much faster.
- Patch Set #2, Line 384:
```
  if (wcsnlen_s(app_id, kMaxStringLen) >= kMaxStringLen) {
    return E_INVALIDARG;
  }

  std::string app_id_str;
  if (!app_id || !base::WideToUTF8(app_id, wcslen(app_id), &app_id_str)) {
    return E_INVALIDARG;
  }
```
  Perhaps combine these 2 `if` statements?
File third_party/win_build_output/midl/chrome/updater/app/server/win/x64/updater_idl.h:
- Patch Set #2, Line 730: virtual HRESULT STDMETHODCALLTYPE CancelInstalls(
  Need to also compile MIDL output for x86 and arm64.

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Sorin Jianu (Gerrit)

unread,

Jul 2, 2022, 1:04:43 AM7/2/22

to Joshua Pawlicki, mac-r...@chromium.org, Xiaoling Bao, S. Ganesh, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Joshua Pawlicki.

View Change

5 comments:

Patchset:
- Patch Set #2:
  thank you!
File chrome/updater/update_service.h:
- Patch Set #2, Line 275: will
  Please use present tense if possible.
File chrome/updater/update_service_impl.cc:
- Patch Set #2, Line 404: )
  missing bug number
File chrome/updater/win/update_service_proxy.cc:
- Patch Set #2, Line 447: Reposts
  Comment not needed, maybe delete similar comment in other places too.
- Patch Set #2, Line 693: HRESULT hr
  Maybe use the same `if` syntax as above, for consistency. Or could use conditional vlog to print.

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Joshua Pawlicki (Gerrit)

unread,

Jul 6, 2022, 12:03:14 PM7/6/22

to mac-r...@chromium.org, Sorin Jianu, Xiaoling Bao, S. Ganesh, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: S. Ganesh, Sorin Jianu.

Patch set 3:Auto-Submit +1

View Change

9 comments:

File chrome/updater/app/server/mac/service_delegate.mm:

- Patch Set #2, Line 256: cancelInstallsWithAppID
  Perhaps `cancelInstallsForAppID`?

- I think that makes sense but With is idiomatic here (e.g. as below in runInstaller).
File chrome/updater/app/server/win/com_classes.cc:

- Patch Set #2, Line 383: kMaxStringLen

- I see `kMaxStringLen` is repeated multiple times in this file, perhaps just have a single constant d […]
  Done

- Patch Set #2, Line 389: WideToUTF8

- Can `app_id` contain unicode characters, or is it going to be an ASCII string? If the latter, can us […]
  I'm not sure but I thought it's best to be consistent with the checks in RunInstaller. Let me know if that seems wrong.

Patch Set #2, Line 384:

  if (wcsnlen_s(app_id, kMaxStringLen) >= kMaxStringLen) {
    return E_INVALIDARG;
  }

  std::string app_id_str;
  if (!app_id || !base::WideToUTF8(app_id, wcslen(app_id), &app_id_str)) {
    return E_INVALIDARG;
  }

Perhaps combine these 2 `if` statements?

- Done
File chrome/updater/update_service.h:

- Patch Set #2, Line 275: will
  Please use present tense if possible.

- Done
File chrome/updater/update_service_impl.cc:

- Patch Set #2, Line 404: )
  missing bug number

- Done
File chrome/updater/win/update_service_proxy.cc:

- Patch Set #2, Line 447: Reposts
  Comment not needed, maybe delete similar comment in other places too.

- Done

- Patch Set #2, Line 693: HRESULT hr
  Maybe use the same `if` syntax as above, for consistency. Or could use conditional vlog to print.

- Done
File third_party/win_build_output/midl/chrome/updater/app/server/win/x64/updater_idl.h:

- Patch Set #2, Line 730: virtual HRESULT STDMETHODCALLTYPE CancelInstalls(
  Need to also compile MIDL output for x86 and arm64.

- Done

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Sorin Jianu (Gerrit)

unread,

Jul 6, 2022, 1:52:04 PM7/6/22

to Joshua Pawlicki, mac-r...@chromium.org, Xiaoling Bao, S. Ganesh, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Joshua Pawlicki, S. Ganesh.

Patch set 3:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #3:
  Thank you!
File chrome/updater/update_service_impl.cc:
- Patch Set #3, Line 415: VLOG(1) << __func__;
  Is there a reason for swapping the lines?

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Joshua Pawlicki (Gerrit)

unread,

Jul 6, 2022, 2:01:21 PM7/6/22

to mac-r...@chromium.org, Sorin Jianu, Xiaoling Bao, S. Ganesh, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: S. Ganesh.

View Change

1 comment:

File chrome/updater/update_service_impl.cc:

- Patch Set #3, Line 415: VLOG(1) << __func__;
  Is there a reason for swapping the lines?

- I wanted to make it consistent with all the other functions in this file.
  If the DCHECK fires we should log a sequence violation.

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Joshua Pawlicki (Gerrit)

unread,

Jul 6, 2022, 2:01:29 PM7/6/22

to mac-r...@chromium.org, Sorin Jianu, Xiaoling Bao, S. Ganesh, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: S. Ganesh.

Patch set 3:Commit-Queue +2

View Change

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Jul 6, 2022, 2:05:02 PM7/6/22

to Joshua Pawlicki, mac-r...@chromium.org, Sorin Jianu, Xiaoling Bao, S. Ganesh, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Joshua Pawlicki: Send CL to CQ automatically after approval; Commit
  Sorin Jianu: Looks good to me

Updater: Add CancelInstalls RPC.

This CL is just the RPC boilerplate.
The RPC is neither called nor implemented yet.

Bug: 1290331
Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3738504
Commit-Queue: Joshua Pawlicki <waf...@chromium.org>
Reviewed-by: Sorin Jianu <so...@chromium.org>
Auto-Submit: Joshua Pawlicki <waf...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1021258}
---
M chrome/updater/app/server/mac/service_delegate.mm
M chrome/updater/app/server/mac/service_protocol.h
M chrome/updater/app/server/win/com_classes.cc
M chrome/updater/app/server/win/com_classes.h
M chrome/updater/app/server/win/updater_idl.template
M chrome/updater/mac/update_service_proxy.h
M chrome/updater/mac/update_service_proxy.mm
M chrome/updater/update_service.h
M chrome/updater/update_service_impl.cc
M chrome/updater/update_service_impl.h
M chrome/updater/update_service_impl_inactive.cc
M chrome/updater/win/update_service_proxy.cc
M chrome/updater/win/update_service_proxy.h
M third_party/win_build_output/midl/chrome/updater/app/server/win/arm64/updater_idl.h
M third_party/win_build_output/midl/chrome/updater/app/server/win/arm64/updater_idl.tlb
M third_party/win_build_output/midl/chrome/updater/app/server/win/arm64/updater_idl_p.c
M third_party/win_build_output/midl/chrome/updater/app/server/win/x64/updater_idl.h
M third_party/win_build_output/midl/chrome/updater/app/server/win/x64/updater_idl.tlb
M third_party/win_build_output/midl/chrome/updater/app/server/win/x64/updater_idl_p.c
M third_party/win_build_output/midl/chrome/updater/app/server/win/x86/updater_idl.h
M third_party/win_build_output/midl/chrome/updater/app/server/win/x86/updater_idl.tlb
M third_party/win_build_output/midl/chrome/updater/app/server/win/x86/updater_idl_p.c
22 files changed, 340 insertions(+), 95 deletions(-)

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Sorin Jianu (Gerrit)

unread,

Jul 6, 2022, 2:21:48 PM7/6/22

to Chromium LUCI CQ, Joshua Pawlicki, mac-r...@chromium.org, Xiaoling Bao, S. Ganesh, chromium...@chromium.org

View Change

1 comment:

File chrome/updater/update_service_impl.cc:

- Patch Set #3, Line 415: VLOG(1) << __func__;

- I wanted to make it consistent with all the other functions in this file. […]
  There are functions in this file where VLOG occurs first.

To view, visit change 3738504. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward