Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2504723003:
Add Bookmarks.BookmarksInFolder histogram to bookmark manager. (Closed)
DescriptionAdd Bookmarks.BookmarksInFolder histogram to bookmark manager.
This CL adds a histogram to track the number of items that are inside
a folder when a folder is opened in the bookmark manager.
BUG=665660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/17e0323adfc252df5696bc824267f6c2d22ce598
Cr-Commit-Position: refs/heads/master@{#432407}
Patch Set 1 #
Total comments: 8
Patch Set 2 : oops #Patch Set 3 : #
Total comments: 5
Patch Set 4 : address comments #
Dependent Patchsets: Messages
Total messages: 18 (6 generated)
Description was changed from ========== Add Bookmarks.BookmarksInFolder histogram to bookmark manager. This CL adds a histogram to track the number of items that are inside a folder when a folder is opened in the bookmark manager. BUG=665660 ========== to ========== Add Bookmarks.BookmarksInFolder histogram to bookmark manager. This CL adds a histogram to track the number of items that are inside a folder when a folder is opened in the bookmark manager. BUG=665660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
calamity@chromium.org changed reviewers: + dbeam@chromium.org
calamity@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2504723003/diff/1/chrome/browser/resources/bo... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/2504723003/diff/1/chrome/browser/resources/bo... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:157: 'buckets': 10 Are you sure 10 buckets is sufficient? https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4847: +<histogram name="Bookmarks.BookmarksInFolder"> Nit: units="bookmarks"? https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4848: + <owner>yfriedman@chromium.org</owner> You probably want yourself here? https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4850: + Logs the depth of the bookmark in the bookmark tree hierarchy every time a This description doesn't seem to match the metric name?
Ugh, ran pretty_print before saving. #shamecube https://codereview.chromium.org/2504723003/diff/1/chrome/browser/resources/bo... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/2504723003/diff/1/chrome/browser/resources/bo... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:157: 'buckets': 10 On 2016/11/16 00:45:30, Alexei Svitkine (very slow) wrote: > Are you sure 10 buckets is sufficient? For our purposes, probably, but I guess more would me more future-proof? Any recommendations? Our intent is to gauge the scale of items in folders so we can understand the performance of the new bookmark manager UI with real world data loads. https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4847: +<histogram name="Bookmarks.BookmarksInFolder"> On 2016/11/16 00:45:31, Alexei Svitkine (very slow) wrote: > Nit: units="bookmarks"? Done. https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4848: + <owner>yfriedman@chromium.org</owner> On 2016/11/16 00:45:31, Alexei Svitkine (very slow) wrote: > You probably want yourself here? Done. https://codereview.chromium.org/2504723003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4850: + Logs the depth of the bookmark in the bookmark tree hierarchy every time a On 2016/11/16 00:45:31, Alexei Svitkine (very slow) wrote: > This description doesn't seem to match the metric name? Done.
https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:156: 'max': 200, wait, why a max of 200? https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:159: console.log(this.dataModel.length); nit: remove the log()
https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:156: 'max': 200, On 2016/11/16 01:26:51, Dan Beam wrote: > wait, why a max of 200? Suggest using equivalent of UMA_HISTOGRAM_COUNTS_1000(). That is, min:1, max:1000, buckets:50 You can also mention that macro in the comment above. (Note: The min:1 will still give you a 0-1 bucket since there's always a 0 bucket.)
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... File chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js (right): https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:156: 'max': 200, On 2016/11/16 01:34:09, Alexei Svitkine (very slow) wrote: > On 2016/11/16 01:26:51, Dan Beam wrote: > > wait, why a max of 200? > > Suggest using equivalent of UMA_HISTOGRAM_COUNTS_1000(). > > That is, min:1, max:1000, buckets:50 > > You can also mention that macro in the comment above. > > (Note: The min:1 will still give you a 0-1 bucket since there's always a 0 > bucket.) Done. https://codereview.chromium.org/2504723003/diff/40001/chrome/browser/resource... chrome/browser/resources/bookmark_manager/js/bmm/bookmark_list.js:159: console.log(this.dataModel.length); On 2016/11/16 01:26:51, Dan Beam wrote: > nit: remove the log() Done.
lgtm
lgtm
The CQ bit was checked by calamity@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 #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add Bookmarks.BookmarksInFolder histogram to bookmark manager. This CL adds a histogram to track the number of items that are inside a folder when a folder is opened in the bookmark manager. BUG=665660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add Bookmarks.BookmarksInFolder histogram to bookmark manager. This CL adds a histogram to track the number of items that are inside a folder when a folder is opened in the bookmark manager. BUG=665660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/17e0323adfc252df5696bc824267f6c2d22ce598 Cr-Commit-Position: refs/heads/master@{#432407} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/17e0323adfc252df5696bc824267f6c2d22ce598 Cr-Commit-Position: refs/heads/master@{#432407} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
