Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1005873011:
Only record the execution time during startup. (Closed)
|
Created:
5 years, 9 months ago by yao Modified:
5 years, 8 months ago CC:
chromium-reviews, browser-components-watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly record the execution time during startup.
Also record the delta size between cached and up-to-date top sites to a histogram.
BUG=223430
Committed: https://crrev.com/a96dcd5baf3ff6cd28dbf002cc22b6c794afa389
Cr-Commit-Position: refs/heads/master@{#325656}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Record time & delta size at startup, address comments. #Patch Set 3 : Rebase #
Total comments: 7
Patch Set 4 : Address comments. #
Total comments: 6
Patch Set 5 : Address comments. #
Total comments: 8
Patch Set 6 : Address comments. #Patch Set 7 : Rebase #Patch Set 8 : Use a static member to make sure the histogram is recorded only once. #
Total comments: 1
Patch Set 9 : Move the static to TopSitesImpl #
Total comments: 4
Patch Set 10 : Rebase #Patch Set 11 : Update the histogram name. #
Messages
Total messages: 42 (9 generated)
yiyaoliu@chromium.org changed reviewers: + rkaplow@chromium.org
PTAL, thanks.
yiyaoliu@chromium.org changed reviewers: + jwd@chromium.org
+jwd for review as well, Thanks.
sorry for slow review. https://codereview.chromium.org/1005873011/diff/1/chrome/browser/history/top_... File chrome/browser/history/top_sites_impl.h (right): https://codereview.chromium.org/1005873011/diff/1/chrome/browser/history/top_... chrome/browser/history/top_sites_impl.h:198: // The bool param is used for knowing weather this function is called during whether https://codereview.chromium.org/1005873011/diff/1/chrome/browser/history/top_... chrome/browser/history/top_sites_impl.h:201: void SetTopSites(const MostVisitedURLList& new_top_sites, bool startup); it seems kind of messy to me to measure it with a bool like this. Is it possible to just wrap the startup calls in a seperate histogram (outside the settopsites call itself)? https://codereview.chromium.org/1005873011/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1005873011/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:11361: + history::TopSitesBackend::UpdateTopSitesOnDBThread to execute when this I would put this into a different histogram since you're changing what this is measuring.
Patchset #2 (id:20001) has been deleted
I changed how the histogram is recorded, after some discution with people. Now we only record it during startup, and both execution time and a delta size are recorded. Thanks for the review! https://codereview.chromium.org/1005873011/diff/1/chrome/browser/history/top_... File chrome/browser/history/top_sites_impl.h (right): https://codereview.chromium.org/1005873011/diff/1/chrome/browser/history/top_... chrome/browser/history/top_sites_impl.h:198: // The bool param is used for knowing weather this function is called during On 2015/03/27 02:33:28, rkaplow wrote: > whether Done. https://codereview.chromium.org/1005873011/diff/1/chrome/browser/history/top_... chrome/browser/history/top_sites_impl.h:201: void SetTopSites(const MostVisitedURLList& new_top_sites, bool startup); On 2015/03/27 02:33:28, rkaplow wrote: > it seems kind of messy to me to measure it with a bool like this. > Is it possible to just wrap the startup calls in a seperate histogram (outside > the settopsites call itself)? It's possible if we only care about execution time, but now we also record the delta size within function settopsites. I can't think of a good way without using an extra param. https://codereview.chromium.org/1005873011/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1005873011/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:11361: + history::TopSitesBackend::UpdateTopSitesOnDBThread to execute when this On 2015/03/27 02:33:28, rkaplow wrote: > I would put this into a different histogram since you're changing what this is > measuring. Deleted the old one.
https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... chrome/browser/history/top_sites_impl.cc:884: void TopSitesImpl::OnGotMostVisitedThumbnails( I'm a bit concerned that this *could* be called during a non-startup scenario. Would it make sense to plumb that boolean out even further? So that it gets set in the bind in TopSitesImpl::Init(..)? https://codereview.chromium.org/1005873011/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1005873011/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11365: +<histogram name="History.UpdateTopSitesOnDBThread_Startup_DeltaSize"> Is there a unit for this?
https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... chrome/browser/history/top_sites_impl.cc:884: void TopSitesImpl::OnGotMostVisitedThumbnails( On 2015/03/30 16:06:47, Jesse Doherty wrote: > I'm a bit concerned that this *could* be called during a non-startup scenario. > Would it make sense to plumb that boolean out even further? So that it gets set > in the bind in TopSitesImpl::Init(..)? But OnGotMostVisitedThumbnails is only called from TopSitesImpl::Init. https://codereview.chromium.org/1005873011/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1005873011/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11365: +<histogram name="History.UpdateTopSitesOnDBThread_Startup_DeltaSize"> On 2015/03/30 16:06:48, Jesse Doherty wrote: > Is there a unit for this? Just integer.
https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... chrome/browser/history/top_sites_impl.cc:884: void TopSitesImpl::OnGotMostVisitedThumbnails( On 2015/03/30 16:06:47, Jesse Doherty wrote: > I'm a bit concerned that this *could* be called during a non-startup scenario. > Would it make sense to plumb that boolean out even further? So that it gets set > in the bind in TopSitesImpl::Init(..)? Ah, ic what you are saying, but OnGotMostVisitedThumbnails is 30 functions deep from BrowserMainRunnerImpl::Run, that would be a overkill I feel.
lgtm https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/60001/chrome/browser/history/... chrome/browser/history/top_sites_impl.cc:884: void TopSitesImpl::OnGotMostVisitedThumbnails( On 2015/03/30 22:18:35, yao wrote: > On 2015/03/30 16:06:47, Jesse Doherty wrote: > > I'm a bit concerned that this *could* be called during a non-startup scenario. > > Would it make sense to plumb that boolean out even further? So that it gets > set > > in the bind in TopSitesImpl::Init(..)? > > Ah, ic what you are saying, but OnGotMostVisitedThumbnails is 30 functions deep > from BrowserMainRunnerImpl::Run, that would be a overkill I feel. As per offline conversation, I'd be comfortable with just pushing it up one level into Init. https://codereview.chromium.org/1005873011/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1005873011/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:11365: +<histogram name="History.UpdateTopSitesOnDBThread_Startup_DeltaSize"> On 2015/03/30 22:16:18, yao wrote: > On 2015/03/30 16:06:48, Jesse Doherty wrote: > > Is there a unit for this? > > Just integer. I see, can you add to the description to make it clear that it is a delta of the number of top sites. Sizes makes me think too much of storage size (which made sense to me since it's talking about a DB).
yiyaoliu@chromium.org changed reviewers: + sky@chromium.org
Hi Scott, Could you an owner's review? Thanks? Also, Jesse is a bit worried about OnGotMostVisitedThumbnails will be called during non-startup later, and he recommends me to move the bool param up 1 level and pass startup=true inside Init. Do you think this is something we should worry about? Thanks, Yiyao
On 2015/03/31 19:41:35, yao wrote: > Hi Scott, > > Could you an owner's review? Thanks? > > Also, Jesse is a bit worried about OnGotMostVisitedThumbnails will be called > during non-startup later, and he recommends me to move the bool param up 1 level > and pass startup=true inside Init. Do you think this is something we should > worry about? Not sure I understand. OnGotMostVisitedThumbnails is only called from Init, right? -Scott > > Thanks, > Yiyao
On 2015/03/31 23:56:26, sky wrote: > On 2015/03/31 19:41:35, yao wrote: > > Hi Scott, > > > > Could you an owner's review? Thanks? > > > > Also, Jesse is a bit worried about OnGotMostVisitedThumbnails will be called > > during non-startup later, and he recommends me to move the bool param up 1 > level > > and pass startup=true inside Init. Do you think this is something we should > > worry about? > > Not sure I understand. OnGotMostVisitedThumbnails is only called from Init, > right? Or more correctly, only called once as the result of someone invoking Init().
https://codereview.chromium.org/1005873011/diff/80001/chrome/browser/history/... File chrome/browser/history/top_sites_impl.h (right): https://codereview.chromium.org/1005873011/diff/80001/chrome/browser/history/... chrome/browser/history/top_sites_impl.h:201: void SetTopSites(const MostVisitedURLList& new_top_sites, bool startup); Please use an enum to avoid a mysterious true/false parameter. Also, it's not startup, it's more of whether this is the call from when history is obtained. https://codereview.chromium.org/1005873011/diff/80001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1005873011/diff/80001/components/history/core... components/history/core/browser/top_sites_backend.cc:126: UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThread_Startup_Time", Didn't you want to put the size of delta in the name? https://codereview.chromium.org/1005873011/diff/80001/components/history/core... File components/history/core/browser/top_sites_backend.h (right): https://codereview.chromium.org/1005873011/diff/80001/components/history/core... components/history/core/browser/top_sites_backend.h:51: void UpdateTopSites(const TopSitesDelta& delta, bool startup); Same thing about an enum here too.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1005873011/diff/80001/chrome/browser/history/... File chrome/browser/history/top_sites_impl.h (right): https://codereview.chromium.org/1005873011/diff/80001/chrome/browser/history/... chrome/browser/history/top_sites_impl.h:201: void SetTopSites(const MostVisitedURLList& new_top_sites, bool startup); On 2015/03/31 23:58:49, sky wrote: > Please use an enum to avoid a mysterious true/false parameter. Also, it's not > startup, it's more of whether this is the call from when history is obtained. I was having debating on the naming of the enum, and also where should the enum be put. Not sure if this looks good to you. https://codereview.chromium.org/1005873011/diff/80001/components/history/core... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1005873011/diff/80001/components/history/core... components/history/core/browser/top_sites_backend.cc:126: UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThread_Startup_Time", On 2015/03/31 23:58:49, sky wrote: > Didn't you want to put the size of delta in the name? The delta size is recorded in TopSitesImpl::SetTopSites, so that the delta size is also recorded when it's 0. This way we know what percentage of times delta is zero/not zero. https://codereview.chromium.org/1005873011/diff/80001/components/history/core... File components/history/core/browser/top_sites_backend.h (right): https://codereview.chromium.org/1005873011/diff/80001/components/history/core... components/history/core/browser/top_sites_backend.h:51: void UpdateTopSites(const TopSitesDelta& delta, bool startup); On 2015/03/31 23:58:49, sky wrote: > Same thing about an enum here too. Done.
https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... chrome/browser/history/top_sites_impl.cc:776: "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); The name seems to indicate you want this only at startup (same for other histogram). If you have multiple profiles this'll be hit more than once. If you want these values only reported once you need to protect against that. https://codereview.chromium.org/1005873011/diff/120001/components/history/cor... File components/history/core/browser/top_sites_backend.h (right): https://codereview.chromium.org/1005873011/diff/120001/components/history/cor... components/history/core/browser/top_sites_backend.h:37: enum TopSitesCalledLocation { This is in the topsites files, so prefixing with topsites just makes this more verbose. How about CallLocation https://codereview.chromium.org/1005873011/diff/120001/components/history/cor... components/history/core/browser/top_sites_backend.h:40: fromOnGotMostVisitedThumbnails, Style guide says enums are ALL_CAPS. And generally we prefix with name of enum, eg CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS.
https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... chrome/browser/history/top_sites_impl.cc:776: "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); On 2015/04/01 22:51:48, sky wrote: > The name seems to indicate you want this only at startup (same for other > histogram). If you have multiple profiles this'll be hit more than once. If you > want these values only reported once you need to protect against that. Right, I guess we could get the number of profiles from ProfileInfoCache to decide whether to record or not. Is there a better way? I feel it's not the end of the world if we don't exclude this case, cause having more than 1 profile is rather rare case. WDYT? https://codereview.chromium.org/1005873011/diff/120001/components/history/cor... File components/history/core/browser/top_sites_backend.h (right): https://codereview.chromium.org/1005873011/diff/120001/components/history/cor... components/history/core/browser/top_sites_backend.h:37: enum TopSitesCalledLocation { On 2015/04/01 22:51:48, sky wrote: > This is in the topsites files, so prefixing with topsites just makes this more > verbose. How about CallLocation Done. https://codereview.chromium.org/1005873011/diff/120001/components/history/cor... components/history/core/browser/top_sites_backend.h:40: fromOnGotMostVisitedThumbnails, On 2015/04/01 22:51:48, sky wrote: > Style guide says enums are ALL_CAPS. And generally we prefix with name of enum, > eg CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS. Done.
https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... chrome/browser/history/top_sites_impl.cc:776: "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); On 2015/04/02 17:20:31, yao wrote: > On 2015/04/01 22:51:48, sky wrote: > > The name seems to indicate you want this only at startup (same for other > > histogram). If you have multiple profiles this'll be hit more than once. If > you > > want these values only reported once you need to protect against that. > > Right, I guess we could get the number of profiles from ProfileInfoCache to > decide whether to record or not. Is there a better way? > > I feel it's not the end of the world if we don't exclude this case, cause having > more than 1 profile is rather rare case. > > WDYT? What about a static?
https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... chrome/browser/history/top_sites_impl.cc:776: "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); On 2015/04/02 18:22:18, sky wrote: > On 2015/04/02 17:20:31, yao wrote: > > On 2015/04/01 22:51:48, sky wrote: > > > The name seems to indicate you want this only at startup (same for other > > > histogram). If you have multiple profiles this'll be hit more than once. If > > you > > > want these values only reported once you need to protect against that. > > > > Right, I guess we could get the number of profiles from ProfileInfoCache to > > decide whether to record or not. Is there a better way? > > > > I feel it's not the end of the world if we don't exclude this case, cause > having > > more than 1 profile is rather rare case. > > > > WDYT? > > What about a static? Like a static class member, that indicates whether this histogram has been recorded, to make sure it can only be recorded once?
Exactly. On Thu, Apr 2, 2015 at 12:37 PM, <yiyaoliu@chromium.org> wrote: > > https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... > File chrome/browser/history/top_sites_impl.cc (right): > > https://codereview.chromium.org/1005873011/diff/120001/chrome/browser/history... > chrome/browser/history/top_sites_impl.cc:776: > "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); > On 2015/04/02 18:22:18, sky wrote: >> >> On 2015/04/02 17:20:31, yao wrote: >> > On 2015/04/01 22:51:48, sky wrote: >> > > The name seems to indicate you want this only at startup (same for > > other >> >> > > histogram). If you have multiple profiles this'll be hit more than > > once. If >> >> > you >> > > want these values only reported once you need to protect against > > that. >> >> > >> > Right, I guess we could get the number of profiles from > > ProfileInfoCache to >> >> > decide whether to record or not. Is there a better way? >> > >> > I feel it's not the end of the world if we don't exclude this case, > > cause >> >> having >> > more than 1 profile is rather rare case. >> > >> > WDYT? > > >> What about a static? > > > Like a static class member, that indicates whether this histogram has > been recorded, to make sure it can only be recorded once? > > https://codereview.chromium.org/1005873011/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I thought static is usually not preferred in Chromium code base, but apparently you know this better than me. If you think it's a good idea, I will make it use a static.
On 2015/04/07 14:42:30, yao wrote: > I thought static is usually not preferred in Chromium code base, but apparently > you know this better than me. If you think it's a good idea, I will make it use > a static. Any more comment? Should I go ahead and use a static instead?
I'm not sure what makes you say statics aren't allowed. They can make testing challenging, but statics are not disallowed. -Scott On Wed, Apr 8, 2015 at 11:50 AM, <yiyaoliu@chromium.org> wrote: > On 2015/04/07 14:42:30, yao wrote: >> >> I thought static is usually not preferred in Chromium code base, but > > apparently >> >> you know this better than me. If you think it's a good idea, I will make >> it > > use >> >> a static. > > > Any more comment? Should I go ahead and use a static instead? > > https://codereview.chromium.org/1005873011/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #8 (id:180001) has been deleted
Patchset #8 (id:200001) has been deleted
Added a static member. Sorry for the delay.
https://codereview.chromium.org/1005873011/diff/220001/components/history/cor... File components/history/core/browser/top_sites_backend.h (right): https://codereview.chromium.org/1005873011/diff/220001/components/history/cor... components/history/core/browser/top_sites_backend.h:63: static void IncraseHistogramRecordingStatus(); Increase Why does this need to be public? Doesn't TopSitesBackend::UpdateTopSitesOnDBThread have enough information to know it can increase?
Hi Scott, I'm afraid this static member method won't work. Let me name TopSitesImpl::SetTopSites as A and TopSitesBackend::UpdateTopSite as B as short names. For example, in my implementation, when A is called the first time, it changes the enum from NOT_YET to IN_PROGRESS, and there's no delta, so B is not called. Then enum keeps the value IN_PROGRESS. At this time, a new profile is opened and there is delta, so B is called and will record the histogram. This is incorrect. Due to the fact that: 1.) A might call B and might not, 2.) when B is called, it is just scheduled, but not executed immediately. 3.) A can be called (from that specific location) multiple times at any time. I can't think of a way to make this work for all cases. Maybe we should just switch to the ProfileInfoCache method? Even if there's a way to make it correct, I feel it would be tricky. Yiyao
Don't you want to only log the histogram the first time CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS is supplied? On Thu, Apr 16, 2015 at 10:48 AM, <yiyaoliu@chromium.org> wrote: > Hi Scott, > > I'm afraid this static member method won't work. > > Let me name TopSitesImpl::SetTopSites as A and > TopSitesBackend::UpdateTopSite as > B as short names. > > For example, in my implementation, when A is called the first time, it > changes > the enum from NOT_YET to IN_PROGRESS, and there's no delta, so B is not > called. > Then enum keeps the value IN_PROGRESS. At this time, a new profile is opened > and > there is delta, so B is called and will record the histogram. This is > incorrect. > > > Due to the fact that: > 1.) A might call B and might not, > 2.) when B is called, it is just scheduled, but not executed immediately. > 3.) A can be called (from that specific location) multiple times at any > time. > I can't think of a way to make this work for all cases. > > Maybe we should just switch to the ProfileInfoCache method? Even if there's > a > way to make it correct, I feel it would be tricky. > > Yiyao > > https://codereview.chromium.org/1005873011/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes, but I don't see how that makes things different. CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS is supplied once whenever a new profile is opened. How can you you guarantee both classes know which one is the first time that CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS is supplied? I don't think the static member can serve to this requirement. On Thu, Apr 16, 2015 at 3:12 PM, Scott Violet <sky@chromium.org> wrote: > Don't you want to only log the histogram the first time > CALL_LOCATION_FROM_ON_GOT_MOST_VISITED_THUMBNAILS is supplied? > > On Thu, Apr 16, 2015 at 10:48 AM, <yiyaoliu@chromium.org> wrote: > > Hi Scott, > > > > I'm afraid this static member method won't work. > > > > Let me name TopSitesImpl::SetTopSites as A and > > TopSitesBackend::UpdateTopSite as > > B as short names. > > > > For example, in my implementation, when A is called the first time, it > > changes > > the enum from NOT_YET to IN_PROGRESS, and there's no delta, so B is not > > called. > > Then enum keeps the value IN_PROGRESS. At this time, a new profile is > opened > > and > > there is delta, so B is called and will record the histogram. This is > > incorrect. > > > > > > Due to the fact that: > > 1.) A might call B and might not, > > 2.) when B is called, it is just scheduled, but not executed immediately. > > 3.) A can be called (from that specific location) multiple times at any > > time. > > I can't think of a way to make this work for all cases. > > > > Maybe we should just switch to the ProfileInfoCache method? Even if > there's > > a > > way to make it correct, I feel it would be tricky. > > > > Yiyao > > > > https://codereview.chromium.org/1005873011/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Updated as per offline discussion. Thanks!
https://codereview.chromium.org/1005873011/diff/240001/chrome/browser/history... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/240001/chrome/browser/history... chrome/browser/history/top_sites_impl.cc:780: "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); Should this be named differently? This isn't on the db thread. https://codereview.chromium.org/1005873011/diff/240001/components/history/cor... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1005873011/diff/240001/components/history/cor... components/history/core/browser/top_sites_backend.cc:125: UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThread_Startup_Time", This name is a bit misleading too. Maybe something like History.FirstUpdateTime ?
https://codereview.chromium.org/1005873011/diff/240001/chrome/browser/history... File chrome/browser/history/top_sites_impl.cc (right): https://codereview.chromium.org/1005873011/diff/240001/chrome/browser/history... chrome/browser/history/top_sites_impl.cc:780: "History.UpdateTopSitesOnDBThread_Startup_DeltaSize", delta_size); On 2015/04/17 15:07:02, sky wrote: > Should this be named differently? This isn't on the db thread. Done. https://codereview.chromium.org/1005873011/diff/240001/components/history/cor... File components/history/core/browser/top_sites_backend.cc (right): https://codereview.chromium.org/1005873011/diff/240001/components/history/cor... components/history/core/browser/top_sites_backend.cc:125: UMA_HISTOGRAM_TIMES("History.UpdateTopSitesOnDBThread_Startup_Time", On 2015/04/17 15:07:02, sky wrote: > This name is a bit misleading too. Maybe something like History.FirstUpdateTime > ? Done.
LGTM
The CQ bit was checked by yiyaoliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org Link to the patchset: https://codereview.chromium.org/1005873011/#ps280001 (title: "Update the histogram name.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005873011/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a96dcd5baf3ff6cd28dbf002cc22b6c794afa389 Cr-Commit-Position: refs/heads/master@{#325656} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
