Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790723003:
[Memory Infra] Update the location of the file where the types are redefined (Closed)
Description[Memory Infra] Update the location of the file where the types are redefined
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface and the memory_instrumentation_struct_traits.{h,cc}
Review-Url: https://codereview.chromium.org/2790723003
Cr-Commit-Position: refs/heads/master@{#462480}
Committed: https://chromium.googlesource.com/chromium/src/+/2e4574583790ae0feb7dc324e4b8157524b55bab
Patch Set 1 #
Total comments: 4
Patch Set 2 : Instead of removing, point to the right files #Messages
Total messages: 19 (9 generated)
fmeawad@chromium.org changed reviewers: + hjd@chromium.org, primiano@chromium.org
PTAL
On 2017/03/30 22:51:04, fmeawad wrote: > PTAL non-owner lgtm, looks like the file has been moved several times since this was last updated and now lives at services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.cc
LGTM with comment https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_request_args.h (left): https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_request_args.h:25: // services/resource_coordinator/public/cpp/memory/memory_infra_traits.cc. As hjd pointed out, this is just moved to services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.cc
https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_request_args.h (left): https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_request_args.h:25: // services/resource_coordinator/public/cpp/memory/memory_infra_traits.cc. On 2017/03/31 10:47:20, Primiano Tucci wrote: > As hjd pointed out, this is just moved to > services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.cc Do you want to replace it or remove it? That file does not include definitions, it includes the mapping. The definition is in services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom I can change it to the mojo file, delete it (I think there is enough compile time checks), or update to the correct traits file. Please advice.
https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_request_args.h (left): https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_request_args.h:25: // services/resource_coordinator/public/cpp/memory/memory_infra_traits.cc. On 2017/03/31 15:07:27, fmeawad wrote: > On 2017/03/31 10:47:20, Primiano Tucci wrote: > > As hjd pointed out, this is just moved to > > > services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.cc > > Do you want to replace it or remove it? That file does not include definitions, > it includes the mapping. > The definition is in > services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom > I can change it to the mojo file, delete it (I think there is enough compile > time checks), or update to the correct traits file. Please advice. Ah I see we need to update both really. I'd just keep this short and say: Keep conssitent with memory_instrumentation_struct_traits.{mojo,cc} (without specifigyin the full path, can be searched)
Description was changed from
==========
[Memory Infra] Remove comment concenring keeping the types consistent with a
deleted file
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface, if the definitions divert, the mojo typemap will fail.
==========
to
==========
[Memory Infra] Update the location of the file where the types are redefined
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface and the memory_instrumentation_struct_traits{.h,.cc}
==========
Done. https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_request_args.h (left): https://codereview.chromium.org/2790723003/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_request_args.h:25: // services/resource_coordinator/public/cpp/memory/memory_infra_traits.cc. On 2017/03/31 17:04:01, Primiano Tucci wrote: > On 2017/03/31 15:07:27, fmeawad wrote: > > On 2017/03/31 10:47:20, Primiano Tucci wrote: > > > As hjd pointed out, this is just moved to > > > > > > services/resource_coordinator/public/cpp/memory/memory_instrumentation_struct_traits.cc > > > > Do you want to replace it or remove it? That file does not include > definitions, > > it includes the mapping. > > The definition is in > > > services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom > > I can change it to the mojo file, delete it (I think there is enough compile > > time checks), or update to the correct traits file. Please advice. > > Ah I see we need to update both really. I'd just keep this short and say: Keep > conssitent with memory_instrumentation_struct_traits.{mojo,cc} (without > specifigyin the full path, can be searched) Done.
Description was changed from
==========
[Memory Infra] Update the location of the file where the types are redefined
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface and the memory_instrumentation_struct_traits{.h,.cc}
==========
to
==========
[Memory Infra] Update the location of the file where the types are redefined
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface and the memory_instrumentation_struct_traits.{h,cc}
==========
The CQ bit was checked by fmeawad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, hjd@chromium.org Link to the patchset: https://codereview.chromium.org/2790723003/#ps20001 (title: "Instead of removing, point to the right files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fmeawad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491489324003430,
"parent_rev": "db58f9674bc5b92d87b22ec41fb825a66be62f77", "commit_rev":
"2e4574583790ae0feb7dc324e4b8157524b55bab"}
Message was sent while issue was closed.
Description was changed from
==========
[Memory Infra] Update the location of the file where the types are redefined
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface and the memory_instrumentation_struct_traits.{h,cc}
==========
to
==========
[Memory Infra] Update the location of the file where the types are redefined
The file referred to no longer exists, making the comment confusing.
Currently those types are maintained in the memory_instrumentation.mojo
interface and the memory_instrumentation_struct_traits.{h,cc}
Review-Url: https://codereview.chromium.org/2790723003
Cr-Commit-Position: refs/heads/master@{#462480}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e4574583790ae0feb7dc324e4b8...
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2e4574583790ae0feb7dc324e4b8... |
