[memory] Replace bool success with MemoryDumpRequestOutcome enum. [chromium/src : main]

0 views
Skip to first unread message

Francois Pierre Doray (Gerrit)

unread,
Oct 12, 2025, 8:00:55 PM (3 days ago) Oct 12
to Chromium Metrics Reviews, Zijie He, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, asvitkine...@chromium.org, fuchsia...@chromium.org, android-web...@chromium.org, asvitki...@chromium.org, blink-...@chromium.org, chrome-gr...@chromium.org, chromiumme...@microsoft.com, feature-me...@chromium.org, ipc-securi...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org, performance-m...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Joe Mason

Francois Pierre Doray added 1 comment

Patchset-level comments
File-level comment, Patchset 6 (Latest):
Francois Pierre Doray . resolved

Please take a look. Thanks.

Open in Gerrit

Related details

Attention is currently required from:
  • Joe Mason
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I28bad48931a80792a66b5ac9701f0ca88fae4832
Gerrit-Change-Number: 7029767
Gerrit-PatchSet: 6
Gerrit-Owner: Francois Pierre Doray <fdo...@chromium.org>
Gerrit-Reviewer: Francois Pierre Doray <fdo...@chromium.org>
Gerrit-Reviewer: Joe Mason <joenot...@google.com>
Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
Gerrit-CC: Zijie He <zij...@google.com>
Gerrit-Attention: Joe Mason <joenot...@google.com>
Gerrit-Comment-Date: Mon, 13 Oct 2025 00:00:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Francois Pierre Doray (Gerrit)

unread,
Oct 12, 2025, 8:55:44 PM (3 days ago) Oct 12
to Chromium Metrics Reviews, Zijie He, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, asvitkine...@chromium.org, fuchsia...@chromium.org, android-web...@chromium.org, asvitki...@chromium.org, blink-...@chromium.org, chrome-gr...@chromium.org, chromiumme...@microsoft.com, feature-me...@chromium.org, ipc-securi...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org, performance-m...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Joe Mason

Francois Pierre Doray voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Joe Mason
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I28bad48931a80792a66b5ac9701f0ca88fae4832
Gerrit-Change-Number: 7029767
Gerrit-PatchSet: 8
Gerrit-Owner: Francois Pierre Doray <fdo...@chromium.org>
Gerrit-Reviewer: Francois Pierre Doray <fdo...@chromium.org>
Gerrit-Reviewer: Joe Mason <joenot...@google.com>
Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
Gerrit-CC: Zijie He <zij...@google.com>
Gerrit-Attention: Joe Mason <joenot...@google.com>
Gerrit-Comment-Date: Mon, 13 Oct 2025 00:55:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Joe Mason (Gerrit)

unread,
Oct 14, 2025, 5:59:47 PM (yesterday) Oct 14
to Francois Pierre Doray, Chromium Metrics Reviews, Zijie He, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, asvitkine...@chromium.org, fuchsia...@chromium.org, android-web...@chromium.org, asvitki...@chromium.org, blink-...@chromium.org, chrome-gr...@chromium.org, chromiumme...@microsoft.com, feature-me...@chromium.org, ipc-securi...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org, performance-m...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
Attention needed from Francois Pierre Doray

Joe Mason added 5 comments

File base/trace_event/memory_dump_manager_unittest.cc
Line 211, Patchset 8 (Latest): std::optional<ProcessMemoryDumpOutcome> captured_outcome;
Joe Mason . unresolved

Optional nit: while you're changing this, it could use TestFuture:

```
TestFuture<ProcessMemoryDumpOutcome, uint64_t,
           std::unique_ptr<ProcessMemoryDump>> future;
mdb->CreateProcessDump(request_args, future.GetSequenceBoundCallback());
ASSERT_TRUE(future.Wait());
EXPECT_EQ(future.Get<uint64_t>(), test_guid);
return future.Get<ProcessMemoryDumpOutcome>();
```
File chrome/browser/autocomplete/autocomplete_browsertest.cc
Line 385, Patchset 8 (Latest): run_loop.QuitClosure()));
Joe Mason . unresolved

Optional nit: this could be simplified with TestFuture.

File services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc
Line 721, Patchset 8 (Latest): EXPECT_EQ(outcome, mojom::MemoryDumpRequestOutcome::kSuccess);
Joe Mason . unresolved

Nit: this isn't needed because EXPECT_CALL already matches only `outcome == kSuccess`.

File services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc
Line 520, Patchset 8 (Latest): ;
Joe Mason . unresolved

Nit: extra ;

File services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
Line 201, Patchset 8 (Latest): return captured_outcome.value();
Joe Mason . unresolved

Optional nit: this could be simplified further with TestFuture.

Open in Gerrit

Related details

Attention is currently required from:
  • Francois Pierre Doray
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I28bad48931a80792a66b5ac9701f0ca88fae4832
    Gerrit-Change-Number: 7029767
    Gerrit-PatchSet: 8
    Gerrit-Owner: Francois Pierre Doray <fdo...@chromium.org>
    Gerrit-Reviewer: Francois Pierre Doray <fdo...@chromium.org>
    Gerrit-Reviewer: Joe Mason <joenot...@google.com>
    Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
    Gerrit-CC: Zijie He <zij...@google.com>
    Gerrit-Attention: Francois Pierre Doray <fdo...@chromium.org>
    Gerrit-Comment-Date: Tue, 14 Oct 2025 21:59:41 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Joe Mason (Gerrit)

    unread,
    Oct 14, 2025, 6:01:16 PM (yesterday) Oct 14
    to Francois Pierre Doray, Chromium Metrics Reviews, Zijie He, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, asvitkine...@chromium.org, fuchsia...@chromium.org, android-web...@chromium.org, asvitki...@chromium.org, blink-...@chromium.org, chrome-gr...@chromium.org, chromiumme...@microsoft.com, feature-me...@chromium.org, ipc-securi...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org, performance-m...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
    Attention needed from Francois Pierre Doray

    Joe Mason voted and added 2 comments

    Votes added by Joe Mason

    Code-Review+1

    2 comments

    Patchset-level comments
    File-level comment, Patchset 8 (Latest):
    Joe Mason . resolved

    mojo and metrics changes LGTM.

    File chrome/browser/memory_details.cc
    Line 359, Patchset 8 (Latest): memory_instrumentation::mojom::MemoryDumpRequestOutcome outcome,
    Joe Mason . unresolved

    Nit: how about adding `kMemoryInstrumentationNotAvailable` to `outcome`, so this doesn't need to change?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Francois Pierre Doray
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I28bad48931a80792a66b5ac9701f0ca88fae4832
      Gerrit-Change-Number: 7029767
      Gerrit-PatchSet: 8
      Gerrit-Owner: Francois Pierre Doray <fdo...@chromium.org>
      Gerrit-Reviewer: Francois Pierre Doray <fdo...@chromium.org>
      Gerrit-Reviewer: Joe Mason <joenot...@google.com>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-CC: Zijie He <zij...@google.com>
      Gerrit-Attention: Francois Pierre Doray <fdo...@chromium.org>
      Gerrit-Comment-Date: Tue, 14 Oct 2025 22:01:08 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Joe Mason (Gerrit)

      unread,
      11:30 AM (10 hours ago) 11:30 AM
      to Francois Pierre Doray, Chromium Metrics Reviews, Zijie He, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, asvitkine...@chromium.org, fuchsia...@chromium.org, android-web...@chromium.org, asvitki...@chromium.org, blink-...@chromium.org, chrome-gr...@chromium.org, chromiumme...@microsoft.com, feature-me...@chromium.org, ipc-securi...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org, performance-m...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
      Attention needed from Francois Pierre Doray

      Joe Mason voted Code-Review+1

      Code-Review+1
      Open in Gerrit

      Related details

      Attention is currently required from:
      • Francois Pierre Doray
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I28bad48931a80792a66b5ac9701f0ca88fae4832
      Gerrit-Change-Number: 7029767
      Gerrit-PatchSet: 9
      Gerrit-Owner: Francois Pierre Doray <fdo...@chromium.org>
      Gerrit-Reviewer: Francois Pierre Doray <fdo...@chromium.org>
      Gerrit-Reviewer: Joe Mason <joenot...@google.com>
      Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>
      Gerrit-CC: Zijie He <zij...@google.com>
      Gerrit-Attention: Francois Pierre Doray <fdo...@chromium.org>
      Gerrit-Comment-Date: Wed, 15 Oct 2025 15:30:31 +0000
      Gerrit-HasComments: No
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Joe Mason (Gerrit)

      unread,
      11:32 AM (10 hours ago) 11:32 AM
      to Francois Pierre Doray, Chromium Metrics Reviews, Zijie He, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, devtools...@chromium.org, asvitkine...@chromium.org, fuchsia...@chromium.org, android-web...@chromium.org, asvitki...@chromium.org, blink-...@chromium.org, chrome-gr...@chromium.org, chromiumme...@microsoft.com, feature-me...@chromium.org, ipc-securi...@chromium.org, jdonnel...@chromium.org, omnibox-...@chromium.org, performance-m...@chromium.org, spang...@chromium.org, tracing...@chromium.org, wfh+...@chromium.org
      Attention needed from Francois Pierre Doray

      Joe Mason added 1 comment

      Patchset-level comments
      File-level comment, Patchset 9 (Latest):
      Joe Mason . resolved

      Outdated Votes:

      Code-Review+1 by Joe Mason (copy condition: "changekind:NO_CHANGE OR changekind:NO_CODE_CHANGE OR changekind:TRIVIAL_REBASE OR (has:unchanged-files AND **uploaderin:cria/project-chromium-committers**) OR is:MIN")

      If I'm reading the bolded part (which was in the original message) the account you uploaded with isn't in chromium-committers for some reason?

      Gerrit-Comment-Date: Wed, 15 Oct 2025 15:32:40 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy
      Reply all
      Reply to author
      Forward
      0 new messages