Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1579433002:
Make profile statistics tasks inspectable by tests (Closed)
DescriptionProfileStatisticsAggregator in profile_statistics.cc was added in
previous CLs without tests except for the accessors of ProfileInfoCache.
Now the necessary tests are added. Some classes are refactored to
improve testability.
Previous CLs: 1248613003, 1415223002, 1428973003
BUG=501916
Committed: https://crrev.com/db92ff9fb8ee68d3d77c3f4f77bd62f7b41fd747
Cr-Commit-Position: refs/heads/master@{#382202}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Respond to comments #
Total comments: 15
Patch Set 3 : Make ProfileStatisticsAggregator tasks trackable #
Total comments: 4
Patch Set 4 : Refactor ProfileStatistics into profile KeyedService #Patch Set 5 : Updated browser tests #Patch Set 6 : Changed gyp/gn files, also a few small changes #
Total comments: 14
Patch Set 7 : Respond to comments, fix bugs #Patch Set 8 : Fixed bugs #Patch Set 9 : Removed a DCHECK which caused unit tests to crash #Patch Set 10 : Used a WeakPtr in DeregisterAggregator callback to address lifetime issue #Patch Set 11 : Added out-of-line destructor in ProfileStatistics #Patch Set 12 : Renamed ProfileAttributesStorage related functions in profile_statistics.* #Messages
Total messages: 67 (27 generated)
Description was changed from ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests. This CL add the necessary tests. BUG=501916 ========== to ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. This CL add the necessary tests. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 ==========
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org, mlerman@chromium.org
Dear all, Please comment. I'll try to add all the tests as described in the previous CLs. BTW, I want to ask if it is possible to simulate a crash in a test. Regards, WC Leung.
A few initial comments. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:28: bool operator<(const ProfileCategoryStat& a, const ProfileCategoryStat& b) { it would be kinda nice if we didn't need these operators. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:33: void PrintTo(const ProfileCategoryStat& a, ::std::ostream* os) { What is this for? https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:98: void ExpectProfileStatisticsEqual(profiles::ProfileCategoryStats actual_stats, these should be ref's (&) https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:108: EXPECT_EQ(expected_stats[i], actual_stats[i]); Can we compare the members of ProfileCategoryStats individually rather than needing to define operator== above? This will additionally let us determine what aspect of the objects aren't the same. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:166: WaitMilliseconds(200); We really really should properly wait for the statistics to complete via Callback. This will almost certainly be flaky.
Thanks for the comment. Anyway I didn't recon that putting "private" function in anywhere out of anonymous namespace is a very bad idea. Anyway, I'll submit another patch today. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:28: bool operator<(const ProfileCategoryStat& a, const ProfileCategoryStat& b) { On 2016/01/11 18:12:20, Mike Lerman wrote: > it would be kinda nice if we didn't need these operators. Didn't recognize that the need to put operators/functions in ::profiles is a deal-breaker. (If two files define the same operator/function the linker will fail.) I was simply too amused with Google test's features. Looks like I still have lots to learn. So I'll turn ::profiles::*, operator or not, into something else. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:33: void PrintTo(const ProfileCategoryStat& a, ::std::ostream* os) { On 2016/01/11 18:12:20, Mike Lerman wrote: > What is this for? It would be nice if we can print out a whole structure (category, count, success) if any part of it is different. BTW, I'll try to achieve this in another way without polluting ::profiles. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:98: void ExpectProfileStatisticsEqual(profiles::ProfileCategoryStats actual_stats, On 2016/01/11 18:12:20, Mike Lerman wrote: > these should be ref's (&) I sort the statistics before comparison, and doing so modifies the original content of the arguments. As the user does not expect a modified ProfileCategoryStatistics by calling this function, so I played safe and copied the contents of both stats objects. Anyway, if you're not into the sorting implementation, I can use std::find instead to match the results, that is less scalable, but it shouldn't matter since there are just a few statistics anyway. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:108: EXPECT_EQ(expected_stats[i], actual_stats[i]); On 2016/01/11 18:12:20, Mike Lerman wrote: > Can we compare the members of ProfileCategoryStats individually rather than > needing to define operator== above? This will additionally let us determine what > aspect of the objects aren't the same. Actually I want to compare the whole thing, and print the whole stat out if there is any difference. So readers will be able to see what exactly is wrong about the statistics in the log, instead of only knowing the count of an unknown stat should be 24 instead of 23. BTW, I'll replace those namespace cursed operator== and PrintTo by predicate formatter assertions. Maybe replace this function too so readers will know which line did the comparison fail, but it will be a little bit complex. https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:166: WaitMilliseconds(200); On 2016/01/11 18:12:20, Mike Lerman wrote: > We really really should properly wait for the statistics to complete via > Callback. This will almost certainly be flaky. I'd like to use a callback, but it is not possible as the writes of ProfileInfoCache is triggered by CloseBrowserSynchronously, for which I cannot inject a callback into. Anyway, RunLoop::RunUntilIdle is also not a solution because statistics callbacks may fire after RunUntilIdle return. If you have any brilliant ideas injecting a good callback then I'd like to know.
Patchset #2 (id:20001) has been deleted
New patch uploaded. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:27: Do you know why this empty row is marked as different? https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:62: const profiles::ProfileCategoryStats& expected_value) { Here's some sample output of the function. e:\chromium\src\chrome\browser\profiles\profile_statistics_browsertest.cc(243): error: ProfileCategoryStats are not equal. Actual: stats Unmatched rows: category = Passwords, count = 0, success = false category = Settings, count = 26, success = true category = Testing, count = 5, success = true Expected: profiles::GetProfileStatisticsFromCache(profile->GetPath()) Unmatched rows: category = Passwords, count = 0, success = true category = Settings, count = 23, success = true
Really interesting approach! Cool stuff :) https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:33: void PrintTo(const ProfileCategoryStat& a, ::std::ostream* os) { On 2016/01/13 12:58:47, lwchkg wrote: > On 2016/01/11 18:12:20, Mike Lerman wrote: > > What is this for? > > It would be nice if we can print out a whole structure (category, count, > success) if any part of it is different. > > BTW, I'll try to achieve this in another way without polluting ::profiles. I'd conversely prefer to test each individual part (whenever we want to verify that each part IS indeed correct); that way, the assertion will automatically just tell us what went wrong. https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:166: WaitMilliseconds(200); On 2016/01/13 12:58:47, lwchkg wrote: > On 2016/01/11 18:12:20, Mike Lerman wrote: > > We really really should properly wait for the statistics to complete via > > Callback. This will almost certainly be flaky. > > I'd like to use a callback, but it is not possible as the writes of > ProfileInfoCache is triggered by CloseBrowserSynchronously, for which I cannot > inject a callback into. > > Anyway, RunLoop::RunUntilIdle is also not a solution because statistics > callbacks may fire after RunUntilIdle return. > > If you have any brilliant ideas injecting a good callback then I'd like to know. What are we really trying to test here? We're trying to test that closing the Browser (specifically, the last browser, which is a more interesting test, so try opening multiple) causes the ProfileStatistics to start to be gathered. We really don't need to verify the accuracy of that process in this test - that's for another unit test. Which means we probably need a way to know if we have a current executing ProfileStatisticsAggregator running for a Profile. This makes sense anyways - we probably don't want to be executing multiple ProfileStatisticsAggregators at the same time for a Profile; we should cancel the currently executing one (if possible?) and create a new one. This implies there SHOULD be some way to track this, and then verify that in this test. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:27: On 2016/01/13 17:21:55, lwchkg wrote: > Do you know why this empty row is marked as different? It's been added; it doesn't exist on the left (e.g. there's no empty line there, just the two const declarations) https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:61: const profiles::ProfileCategoryStats& actual_value, don't use "value", it conveys nothing. actual_categories is better. (and expected_categories) https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:62: const profiles::ProfileCategoryStats& expected_value) { On 2016/01/13 17:21:55, lwchkg wrote: > Here's some sample output of the function. > > e:\chromium\src\chrome\browser\profiles\profile_statistics_browsertest.cc(243): > error: ProfileCategoryStats are not equal. > Actual: stats > Unmatched rows: category = Passwords, count = 0, success = false > category = Settings, count = 26, success = true > category = Testing, count = 5, success = true > Expected: profiles::GetProfileStatisticsFromCache(profile->GetPath()) > Unmatched rows: category = Passwords, count = 0, success = true > category = Settings, count = 23, success = true Why only print the unmatched rows?? Why not just print out Everything about the current state of each ProfileCategoryStats? I imagine, as someone debugging a problem, that will be useful information to know. I'd either print the first thing that you noticed broke (hey, we're missing a category!) or print out the whole state of the ProfileCategoryStats. That should also make the function easier. You can just verify one aspect of the equality at a time, and then report as necessary (all or just that piece of informatoin). https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:68: bool match_failed = actual_count != expected_count; match of what? of counts. Perhaps call this counts_match (and invert the meaning true/false)
Thanks for comment. Making profile statistics trackable is definitely a good idea. I need to have the expected behavior defined before writing the code though. (The question is in one of the comments below.) https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_browsertest.cc:166: WaitMilliseconds(200); On 2016/01/14 15:21:53, Mike Lerman wrote: > What are we really trying to test here? We're trying to test that closing the > Browser (specifically, the last browser, which is a more interesting test, so > try opening multiple) causes the ProfileStatistics to start to be gathered. We > really don't need to verify the accuracy of that process in this test - that's > for another unit test. > > Which means we probably need a way to know if we have a current executing > ProfileStatisticsAggregator running for a Profile. This makes sense anyways - we > probably don't want to be executing multiple ProfileStatisticsAggregators at the > same time for a Profile; we should cancel the currently executing one (if > possible?) and create a new one. This implies there SHOULD be some way to track > this, and then verify that in this test. Interesting. I have a question though: if we execute two ProfileStatisticsAggregators with the same Profile but with a different callback, what should be the result? Should I call both callbacks when the statistics arrive? https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:27: On 2016/01/14 15:21:53, Mike Lerman wrote: > On 2016/01/13 17:21:55, lwchkg wrote: > > Do you know why this empty row is marked as different? > > It's been added; it doesn't exist on the left (e.g. there's no empty line there, > just the two const declarations) Thanks. Didn't notice this one. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:61: const profiles::ProfileCategoryStats& actual_value, On 2016/01/14 15:21:53, Mike Lerman wrote: > don't use "value", it conveys nothing. > actual_categories is better. (and expected_categories) Acknowledged. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:62: const profiles::ProfileCategoryStats& expected_value) { On 2016/01/14 15:21:53, Mike Lerman wrote: > > Why only print the unmatched rows?? Why not just print out Everything about the > current state of each ProfileCategoryStats? I imagine, as someone debugging a > problem, that will be useful information to know. > > I'd either print the first thing that you noticed broke (hey, we're missing a > category!) or print out the whole state of the ProfileCategoryStats. > > That should also make the function easier. You can just verify one aspect of the > equality at a time, and then report as necessary (all or just that piece of > informatoin). I see. I'd print out the whole statistics since comparison cannot be made simpler. Anyway, I don't see either option can make the function easier. The statistics may be added in a different order, so comparison must be done using an algorithm like lines 68-83 below or the one in patch #1. (The algorithm is #1 is more scalable, but that's not too important since we have only 4 statistics.) For both algorithms, stopping at the first match failure doesn't make the function much easier. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:68: bool match_failed = actual_count != expected_count; On 2016/01/14 15:21:53, Mike Lerman wrote: > match of what? of counts. Perhaps call this counts_match (and invert the meaning > true/false) Looks like I've named the variable incorrectly, so I'll rewrite to make it clearer. Anyway, the sets of statistics A and B are equal if and only if (1) count of A = count of B, and also (2) if a value exists in set A, it must also be exist in set B. This line checks for (1). (If the logic is not obvious to you, please tell because it means that I need to add some comments to the source code.)
https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:68: bool match_failed = actual_count != expected_count; On 2016/01/16 18:07:51, lwchkg wrote: > On 2016/01/14 15:21:53, Mike Lerman wrote: > > match of what? of counts. Perhaps call this counts_match (and invert the > meaning > > true/false) > > Looks like I've named the variable incorrectly, so I'll rewrite to make it > clearer. > > Anyway, the sets of statistics A and B are equal if and only if > (1) count of A = count of B, and also > (2) if a value exists in set A, it must also be exist in set B. You could probably just add a comment with the above in the function, it's super clear when worded that way :) > > This line checks for (1). > (If the logic is not obvious to you, please tell because it means that I need to > add some comments to the source code.) https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:222: // Wait for at most 2 seconds for the statistics to be gathered. This looks to me like it has the potential to be flaky. Is there a different way to do it? Is there a way to observe this process/be notified when the stats are done being gathered?
Just some replies. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:68: bool match_failed = actual_count != expected_count; On 2016/01/18 15:47:08, anthonyvd wrote: > On 2016/01/16 18:07:51, lwchkg wrote: > > On 2016/01/14 15:21:53, Mike Lerman wrote: > > > match of what? of counts. Perhaps call this counts_match (and invert the > > meaning > > > true/false) > > > > Looks like I've named the variable incorrectly, so I'll rewrite to make it > > clearer. > > > > Anyway, the sets of statistics A and B are equal if and only if > > (1) count of A = count of B, and also > > (2) if a value exists in set A, it must also be exist in set B. > > You could probably just add a comment with the above in the function, it's super > clear when worded that way :) > > > > > This line checks for (1). > > (If the logic is not obvious to you, please tell because it means that I need > to > > add some comments to the source code.) > Thanks. So comments will be added in the next patch. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:222: // Wait for at most 2 seconds for the statistics to be gathered. On 2016/01/18 15:47:08, anthonyvd wrote: > This looks to me like it has the potential to be flaky. Is there a different way > to do it? Is there a way to observe this process/be notified when the stats are > done being gathered? The same topic is still being discussed in patch #1, so let's continue there. Anyway, here's the summary: I'll modify profile_statistics.cc to allow inspecting from the outside.
Now in the middle of the edits. So I've changed the subject of the CL. I have a few questions to ask: (1) Can you start the try bots for me? I want to know what platforms does the CL fail. (2) And I plan to disable the code (in all files) for platforms that do not use it. (a) What platforms should I enable the code? Is it "WIN + OSX + X11 - CHROMEOS"? (b) Should I use IFDEFs or modify the gyp files? (Anyway, I don't understand the relationship between the gyp files and gn builds. Recently gyp Windows build are broken, so I've been forced to switch to gn.) (3) And also the questions marked in the code. Regards, WC Leung. https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:51: static std::map<void*, ProfileStatisticsAggregator*> aggregators_; Having a static member here is not good. Which of the following do you prefer? (1) Have this class owned by ProfileManager. (2) Make this class a KeyedService. (3) Make this class a singleton. https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_constants.h (right): https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_constants.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Should I write copyright 2016? (Note that the code is not new. It's moved from another file.)
The CQ bit was checked by mlerman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/60001
I'm slightly confused about the platforms comment, e.g. that this code is only used on some platforms. As far as I know, all platforms (even I think android) make some use of the profilemanager and profileinfocache. https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:51: static std::map<void*, ProfileStatisticsAggregator*> aggregators_; On 2016/01/25 21:35:53, lwchkg wrote: > Having a static member here is not good. Which of the following do you prefer? > > (1) Have this class owned by ProfileManager. > (2) Make this class a KeyedService. > (3) Make this class a singleton. I think this could be a ProfileKeyedService. https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_constants.h (right): https://codereview.chromium.org/1579433002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_constants.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/25 21:35:53, lwchkg wrote: > Should I write copyright 2016? (Note that the code is not new. It's moved from > another file.) When a file's renamed, then keep the original date. If this is a major refactor and you've made a new class/file (that stole a bunch of code from another file/class that still exists) then I think that's considered new and you can slap a 2016 on it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
On 2016/02/01 16:42:01, Mike Lerman wrote: > I'm slightly confused about the platforms comment, e.g. that this code is only > used on some platforms. As far as I know, all platforms (even I think android) > make some use of the profilemanager and profileinfocache. The statistics code is now used by DesktopUserPod only. That's why profile_statistics* should be platform specific.
On 2016/02/01 16:42:01, Mike Lerman wrote: > I'm slightly confused about the platforms comment, e.g. that this code is only > used on some platforms. As far as I know, all platforms (even I think android) > make some use of the profilemanager and profileinfocache. The statistics code is now used by DesktopUserPod only. That's why profile_statistics* should be platform specific.
On 2016/02/01 16:42:01, Mike Lerman wrote: > I'm slightly confused about the platforms comment, e.g. that this code is only > used on some platforms. As far as I know, all platforms (even I think android) > make some use of the profilemanager and profileinfocache. The statistics code is now used by DesktopUserPod only. That's why profile_statistics* should be platform specific.
On 2016/02/01 23:05:09, lwchkg wrote: > On 2016/02/01 16:42:01, Mike Lerman wrote: > > I'm slightly confused about the platforms comment, e.g. that this code is only > > used on some platforms. As far as I know, all platforms (even I think android) > > make some use of the profilemanager and profileinfocache. > > The statistics code is now used by DesktopUserPod only. That's why > profile_statistics* should be platform specific. Ah! Yes, then we should definitely make sure the code that activitates the ProfileAggregators only takes place on relevant platforms - no point take up cycles or logging info that isn't used. Because OS_LINUX includes chromeos, it's easier to ifdef out the opposite, so: #ifndef OS_ANDROID && ifndef OS_IOS && ifndef OS_CHROMEOS.
Patchset #5 (id:100001) has been deleted
Finished the refactoring, and updated tests according to the comments. I didn't add the ifndefs yet, but other than that the code is finished. So PTAL and start another CQ dry run. Thanks. https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics_browsertest.cc:68: bool match_failed = actual_count != expected_count; On 2016/01/18 18:01:19, lwchkg wrote: > On 2016/01/18 15:47:08, anthonyvd wrote: > > > > You could probably just add a comment with the above in the function, it's > super > > clear when worded that way :) > > Thanks. So comments will be added in the next patch. Turns out std::is_permutation() was approved, so I can switch to that function instead. Anyway std::is_permutation() should work exactly like the code here.
lwchkg@gmail.com changed reviewers: + sky@chromium.org
Description was changed from ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. This CL add the necessary tests. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 ========== to ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. Now the necessary tests are added. Some classes are refactored to improve testability. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 ==========
Patchset #6 (id:140001) has been deleted
Dear all, PTAL and thanks. Here are the files you need to review: Scott: all gyp/gn files. (FYI: most of the files in this CL are non-android, non-ios and non-chromeos.) Mike: everything else. Please also help me to start a CQ dry run. Regards, WC Leung.
The CQ bit was checked by mlerman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. don't change the year https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:35: bool ProfileStatistics::HasAggregator() { this can be const https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:50: explicit ProfileStatistics(Profile* profile) : profile_(profile) {} Set profile_ in the .cc file https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:55: ProfileStatisticsAggregator* aggregator_ = nullptr; set aggregator_ = nullptr in the .cc file's constructor https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics_constants.h (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics_constants.h:8: namespace profiles { this can go with profile_statistics_types I think. Likely never need one without the other.
LGTM
Uploaded another patch. Anyway you may want to wait until the trybots are green before further reviewing. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/03/15 18:54:22, Mike Lerman wrote: > don't change the year Done. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:35: bool ProfileStatistics::HasAggregator() { On 2016/03/15 18:54:22, Mike Lerman wrote: > this can be const Done. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:50: explicit ProfileStatistics(Profile* profile) : profile_(profile) {} On 2016/03/15 18:54:22, Mike Lerman wrote: > Set profile_ in the .cc file Done. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:55: ProfileStatisticsAggregator* aggregator_ = nullptr; On 2016/03/15 18:54:22, Mike Lerman wrote: > set aggregator_ = nullptr in the .cc file's constructor Done. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics_constants.h (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics_constants.h:8: namespace profiles { On 2016/03/15 18:54:22, Mike Lerman wrote: > this can go with profile_statistics_types I think. Likely never need one without > the other. Agree. Just struggled on the new name. Anyway, I'll name the new file profile_statistics_common.
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #8 (id:200001) has been deleted
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/240001
lwchkg@gmail.com changed reviewers: + avi@chromium.org
+avi@ Avi: Can you please check the comment (see below) regarding content::MessageLoopRunner? (Anyway, there is no need for you to review any code.) https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:92: if (!g_browser_process || !g_browser_process->local_state()) I added this check because it would seg-fault if local_state_ were nullptr and created_profile_manager_ were false. But it turns out that it should not be possible. local_state_ in browser_process_impl.cc is never unset. It may be unset in unit tests, but profile_manager() in testing_browser_process.cc does not depend on local_state_. Therefore I removed the check. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics_browsertest.cc:219: scoped_refptr<content::MessageLoopRunner> loop1a = Using content::MessageLoopRunner instead of base::RunLoop caused this test to fail. Symptom: after loop1a->Run(), all 4 stats are returned, instead of just 1. Avi: Do you know why does it happen?
https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics_browsertest.cc:219: scoped_refptr<content::MessageLoopRunner> loop1a = On 2016/03/16 17:26:03, lwchkg wrote: > Using content::MessageLoopRunner instead of base::RunLoop caused this test to > fail. Symptom: after loop1a->Run(), all 4 stats are returned, instead of just 1. > > Avi: Do you know why does it happen? Offhand, no, I've never used so many at once. BTW, why the scoped_refptrs rather than just making them inline local variables?
Mike: The bots are finally green now. PTAL. https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics_browsertest.cc (right): https://codereview.chromium.org/1579433002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics_browsertest.cc:219: scoped_refptr<content::MessageLoopRunner> loop1a = On 2016/03/16 17:39:04, Avi wrote: > On 2016/03/16 17:26:03, lwchkg wrote: > > Using content::MessageLoopRunner instead of base::RunLoop caused this test to > > fail. Symptom: after loop1a->Run(), all 4 stats are returned, instead of just > 1. > > > > Avi: Do you know why does it happen? > > Offhand, no, I've never used so many at once. > > BTW, why the scoped_refptrs rather than just making them inline local variables? content::MessageLoopRunner is a RefCounted, so we need to use a smart pointer that do refcounting to track it. It's use is documented in https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I like! LGTM
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1579433002/#ps300001 (title: "Renamed ProfileAttributesStorage related functions in profile_statistics.*")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1579433002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1579433002/300001
Message was sent while issue was closed.
Description was changed from ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. Now the necessary tests are added. Some classes are refactored to improve testability. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 ========== to ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. Now the necessary tests are added. Some classes are refactored to improve testability. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. Now the necessary tests are added. Some classes are refactored to improve testability. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 ========== to ========== ProfileStatisticsAggregator in profile_statistics.cc was added in previous CLs without tests except for the accessors of ProfileInfoCache. Now the necessary tests are added. Some classes are refactored to improve testability. Previous CLs: 1248613003, 1415223002, 1428973003 BUG=501916 Committed: https://crrev.com/db92ff9fb8ee68d3d77c3f4f77bd62f7b41fd747 Cr-Commit-Position: refs/heads/master@{#382202} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/db92ff9fb8ee68d3d77c3f4f77bd62f7b41fd747 Cr-Commit-Position: refs/heads/master@{#382202} |
