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

0 views
Skip to first unread message

Joshua Pawlicki (Gerrit)

unread,
Jul 1, 2022, 5:26:00 PMJul 1
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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Gerrit-Change-Number: 3738504
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
Gerrit-CC: S. Ganesh <gan...@chromium.org>
Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
Gerrit-Attention: Sorin Jianu <so...@chromium.org>
Gerrit-Comment-Date: Fri, 01 Jul 2022 21:25:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

S. Ganesh (Gerrit)

unread,
Jul 1, 2022, 6:53:22 PMJul 1
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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Gerrit-Change-Number: 3738504
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
Gerrit-CC: S. Ganesh <gan...@chromium.org>
Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
Gerrit-Attention: Sorin Jianu <so...@chromium.org>
Gerrit-Attention: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Comment-Date: Fri, 01 Jul 2022 22:53:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Sorin Jianu (Gerrit)

unread,
Jul 2, 2022, 1:04:43 AMJul 2
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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Gerrit-Change-Number: 3738504
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
Gerrit-CC: S. Ganesh <gan...@chromium.org>
Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
Gerrit-Attention: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Comment-Date: Sat, 02 Jul 2022 05:04:31 +0000

Joshua Pawlicki (Gerrit)

unread,
Jul 6, 2022, 12:03:14 PMJul 6
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:

    • 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:

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

      Done

    • 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:

    • Done

  • File chrome/updater/update_service_impl.cc:

    • Done

  • File chrome/updater/win/update_service_proxy.cc:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Gerrit-Change-Number: 3738504
Gerrit-PatchSet: 3
Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
Gerrit-CC: S. Ganesh <gan...@chromium.org>
Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
Gerrit-Attention: S. Ganesh <gan...@chromium.org>
Gerrit-Attention: Sorin Jianu <so...@chromium.org>
Gerrit-Comment-Date: Wed, 06 Jul 2022 16:02:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: S. Ganesh <gan...@chromium.org>
Comment-In-Reply-To: Sorin Jianu <so...@chromium.org>
Gerrit-MessageType: comment

Sorin Jianu (Gerrit)

unread,
Jul 6, 2022, 1:52:04 PMJul 6
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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Gerrit-Change-Number: 3738504
Gerrit-PatchSet: 3
Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
Gerrit-CC: S. Ganesh <gan...@chromium.org>
Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
Gerrit-Attention: S. Ganesh <gan...@chromium.org>
Gerrit-Attention: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Comment-Date: Wed, 06 Jul 2022 17:51:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Joshua Pawlicki (Gerrit)

unread,
Jul 6, 2022, 2:01:21 PMJul 6
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:

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

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
Gerrit-Change-Number: 3738504
Gerrit-PatchSet: 3
Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
Gerrit-CC: S. Ganesh <gan...@chromium.org>
Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
Gerrit-Attention: S. Ganesh <gan...@chromium.org>
Gerrit-Comment-Date: Wed, 06 Jul 2022 18:01:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Joshua Pawlicki (Gerrit)

unread,
Jul 6, 2022, 2:01:29 PMJul 6
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.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
    Gerrit-Change-Number: 3738504
    Gerrit-PatchSet: 3
    Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
    Gerrit-CC: S. Ganesh <gan...@chromium.org>
    Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
    Gerrit-Attention: S. Ganesh <gan...@chromium.org>
    Gerrit-Comment-Date: Wed, 06 Jul 2022 18:01:20 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    Gerrit-MessageType: comment

    Chromium LUCI CQ (Gerrit)

    unread,
    Jul 6, 2022, 2:05:02 PMJul 6
    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.

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
    Gerrit-Change-Number: 3738504
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
    Gerrit-CC: S. Ganesh <gan...@chromium.org>
    Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
    Gerrit-MessageType: merged

    Sorin Jianu (Gerrit)

    unread,
    Jul 6, 2022, 2:21:48 PMJul 6
    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:

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

    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If937d2d529789d0799faee68d5893b8b2b664569
    Gerrit-Change-Number: 3738504
    Gerrit-PatchSet: 4
    Gerrit-Owner: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-Reviewer: Sorin Jianu <so...@chromium.org>
    Gerrit-CC: S. Ganesh <gan...@chromium.org>
    Gerrit-CC: Xiaoling Bao <xiaol...@chromium.org>
    Gerrit-Comment-Date: Wed, 06 Jul 2022 18:21:37 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Sorin Jianu <so...@chromium.org>
    Comment-In-Reply-To: Joshua Pawlicki <waf...@chromium.org>
    Gerrit-MessageType: comment
    Reply all
    Reply to author
    Forward
    0 new messages