Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168233002:
Un-obsolete Memory.{Browser,Renderer} metrics (Closed)
DescriptionUn-obsolete Memory.{Browser,Renderer} metrics
Per-request from rkaplow
BUG=
Committed: https://crrev.com/79d5065f63f6ac095a520eadeca5fe2fdc412cc5
Cr-Commit-Position: refs/heads/master@{#407696}
Patch Set 1 #
Total comments: 9
Patch Set 2 : comments #Messages
Total messages: 12 (3 generated)
bashi@chromium.org changed reviewers: + isherman@chromium.org, rkaplow@chromium.org
https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... chrome/browser/metrics/metrics_memory_details.cc:92: // TODO(rkaplow): Remove later. This metric is capped at .5GB. nit: Please provide a target milestone, probably "Remove after M56 is branched". https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... chrome/browser/metrics/metrics_memory_details.cc:113: // TODO(rkaplow): Remove later. This metric is capped at .5GB. Ditto. https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23359: + Memory.Browser.Large2. Please provide the same target milestone here and below.
https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... chrome/browser/metrics/metrics_memory_details.cc:92: // TODO(rkaplow): Remove later. This metric is capped at .5GB. On 2016/07/21 22:25:53, Ilya Sherman wrote: > nit: Please provide a target milestone, probably "Remove after M56 is branched". Yes, but I don't know a target milestone. rkaplow@: can you answer this? https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23359: + Memory.Browser.Large2. On 2016/07/21 22:25:53, Ilya Sherman wrote: > Please provide the same target milestone here and below. rkaplow@ ?
https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... chrome/browser/metrics/metrics_memory_details.cc:92: // TODO(rkaplow): Remove later. This metric is capped at .5GB. On 2016/07/21 22:29:02, bashi1 wrote: > On 2016/07/21 22:25:53, Ilya Sherman wrote: > > nit: Please provide a target milestone, probably "Remove after M56 is > branched". > > Yes, but I don't know a target milestone. rkaplow@: can you answer this? You can actually remove the actual recording of the metrics. That's actually fine, since we can just replace with the new ones. It's more just the obsolete tag so we can access the old metrics for now. https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23359: + Memory.Browser.Large2. On 2016/07/21 22:29:02, bashi1 wrote: > On 2016/07/21 22:25:53, Ilya Sherman wrote: > > Please provide the same target milestone here and below. > > rkaplow@ ? Agreed with adjusting the comment here though. M54 should be fine.
https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... File chrome/browser/metrics/metrics_memory_details.cc (right): https://codereview.chromium.org/2168233002/diff/1/chrome/browser/metrics/metr... chrome/browser/metrics/metrics_memory_details.cc:92: // TODO(rkaplow): Remove later. This metric is capped at .5GB. On 2016/07/25 18:52:05, rkaplow wrote: > On 2016/07/21 22:29:02, bashi1 wrote: > > On 2016/07/21 22:25:53, Ilya Sherman wrote: > > > nit: Please provide a target milestone, probably "Remove after M56 is > > branched". > > > > Yes, but I don't know a target milestone. rkaplow@: can you answer this? > > You can actually remove the actual recording of the metrics. That's actually > fine, since we can just replace with the new ones. It's more just the obsolete > tag so we can access the old metrics for now. Done. https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2168233002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:23359: + Memory.Browser.Large2. On 2016/07/25 18:52:05, rkaplow wrote: > On 2016/07/21 22:29:02, bashi1 wrote: > > On 2016/07/21 22:25:53, Ilya Sherman wrote: > > > Please provide the same target milestone here and below. > > > > rkaplow@ ? > > Agreed with adjusting the comment here though. M54 should be fine. Done.
LGTM
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from
==========
Un-obsolete Memory.{Browser,Renderer} metrics
Per-request from rkaplow
BUG=
==========
to
==========
Un-obsolete Memory.{Browser,Renderer} metrics
Per-request from rkaplow
BUG=
Committed: https://crrev.com/79d5065f63f6ac095a520eadeca5fe2fdc412cc5
Cr-Commit-Position: refs/heads/master@{#407696}
==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/79d5065f63f6ac095a520eadeca5fe2fdc412cc5 Cr-Commit-Position: refs/heads/master@{#407696} |
