Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1218583002:
metrics: Add dbus interface for GetRandomPerfOutput (Closed)
|
Created:
5 years, 5 months ago by Simon Que Modified:
5 years, 5 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, asvitkine+watch_chromium.org, dhsharp, rapati Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmetrics: Add dbus interface for GetRandomPerfOutput
GetRandomPerfOutput is a new dbus method call that replaces the older
GetRichPerfData method call to debugd. The difference is that the new
method call can return either a PerfDataProto or a PerfStatProto.
Also updating PerfProvider to use the new dbus interface and parse and
store the result as either protobuf format.
BUG=chromium:478289
TEST=Set |kPerfProfilingIntervalMs| to something short (e.g. 15 sec)
and add logging to ParseOutputProtoIfValid() to verify that valid
PerfDataProto or PerfStatProto data was received.
Signed-off-by: Simon Que <sque@chromium.org>;
Committed: https://crrev.com/e0260fd891172eb94a614751b1c77596e476bab2
Cr-Commit-Position: refs/heads/master@{#337910}
Patch Set 1 #Patch Set 2 : Check that the dbus method returns some data #
Total comments: 11
Patch Set 3 : Added unit test for perf_provider_chromeos #Patch Set 4 : Responded to more comments #Patch Set 5 : Do not allow both perf_stat and perf_data to be passed in; treat it as an error #
Total comments: 45
Patch Set 6 : Clean up includes, add WindowedIncognitoObserver, use else for perf_stat case #Patch Set 7 : Replace friendship with derived class #Patch Set 8 : Addressed remaining comments #
Total comments: 26
Patch Set 9 : Disallow copy/assign, directly call GetSampledProfiles, remove raw perf data members #Patch Set 10 : Directly call ParseOutputProtoIfValid() #Patch Set 11 : Add comment for clarity #
Total comments: 12
Patch Set 12 : Addressed all comments on Patch Set 11 #
Total comments: 3
Patch Set 13 : Fixed CreateWithIncognitoLaunched() #
Total comments: 4
Patch Set 14 : Create a scoped_ptr<TestIncognitoObserver> and return as scoped_ptr<WindowedIncognitoObserver> #
Total comments: 6
Patch Set 15 : Return observer.Pass(), implicitly create returned scoped_ptr #Patch Set 16 : Responded to stevenjb's comments: removed {}s, NULL -> nullptr #Patch Set 17 : "virtual" is redundant with "override" #Patch Set 18 : Set login state during testing #Patch Set 19 : Create TestSimpleTaskRunner and handle #
Total comments: 2
Patch Set 20 : Check has_ms_after_login() instead of ms_after_login() value #Messages
Total messages: 56 (13 generated)
sque@chromium.org changed reviewers: + isherman@chromium.org
This patch cannot land until a few other CLs have landed: https://chromium-review.googlesource.com/#/c/280609/ https://chromium-review.googlesource.com/#/c/280914/ DEPS will also need to be updated to pull in the latest changes to system_api, once CrOS CL 280609 lands.
https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:375: if (result != 0 || (perf_data.empty() || perf_stat.empty())) { nit: Should the second || be an &&? If so, please add a regression test in addition to fixing the bug. https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:382: SampledProfile& sampled_profile = *sampled_profile_ptr.get(); Erm, why don't you just use the arrow operator? What am I missing? https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:388: return; Is it appropriate to discard the perf_stat array if the perf_data one can't be parsed? If so, why isn't the reverse also true? https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:402: // stat? Are you planning to address this TODO as part of this CL, or to commit it? I'd encourage you to decide sooner rather than later, unless there's data that you're waiting for. (FWIW, there's probably not much cost to setting it.) https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:105: int result, Please document the |result|.
https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:375: if (result != 0 || (perf_data.empty() || perf_stat.empty())) { On 2015/06/27 05:44:24, Ilya Sherman wrote: > nit: Should the second || be an &&? If so, please add a regression test in > addition to fixing the bug. Done. https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:105: int result, On 2015/06/27 05:44:24, Ilya Sherman wrote: > Please document the |result|. Done.
https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:382: SampledProfile& sampled_profile = *sampled_profile_ptr.get(); On 2015/06/27 05:44:24, Ilya Sherman wrote: > Erm, why don't you just use the arrow operator? What am I missing? Done. https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:388: return; On 2015/06/27 05:44:24, Ilya Sherman wrote: > Is it appropriate to discard the perf_stat array if the perf_data one can't be > parsed? If so, why isn't the reverse also true? Good question. That's easy enough to deal with, but I need to consult my teammates about it and see what the histogram behavior should be if one succeeds but the other fails. https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:402: // stat? On 2015/06/27 05:44:24, Ilya Sherman wrote: > Are you planning to address this TODO as part of this CL, or to commit it? I'd > encourage you to decide sooner rather than later, unless there's data that > you're waiting for. (FWIW, there's probably not much cost to setting it.) This is actually wrong. The timestamp is time since epoch, not since boot. I'm thinking that we should instead call BootTimesRecorder::Stats::GetCurrentStats().
On 2015/06/30 05:50:48, Simon Que wrote: > This is actually wrong. The timestamp is time since epoch, not since boot. > > I'm thinking that we should instead call > BootTimesRecorder::Stats::GetCurrentStats(). BootTimesRecorder doesn't expose the internals, unfortunately. How about I add a function to perf_provider_chromeos to read from /proc/uptime?
PTAL
https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1218583002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:402: // stat? On 2015/06/30 05:50:48, Simon Que wrote: > On 2015/06/27 05:44:24, Ilya Sherman wrote: > > Are you planning to address this TODO as part of this CL, or to commit it? > I'd > > encourage you to decide sooner rather than later, unless there's data that > > you're waiting for. (FWIW, there's probably not much cost to setting it.) > > This is actually wrong. The timestamp is time since epoch, not since boot. > > I'm thinking that we should instead call > BootTimesRecorder::Stats::GetCurrentStats(). Sorry, what is actually wrong? Is the ms_after_boot data being set incorrectly for the PerfDataProto? It should be easy enough to translate a timestamp since epoch to a timestamp since boot. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:403: if (!perf_stat.empty()) { nit: I think it would be clearer to simply write this case as "else", maybe with a DCHECK. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:29: class IncognitoObserverInterface { nit: If you really need this class, please omit the "Interface" part of the name. I'd prefer, though, that you simply extracted the WindowedIncognitoObserver class to a separate file, and marked the appropriate method as virtual if need be. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:35: virtual bool incognito_launched() const = 0; nit: virtual functions are never trivial/inline-able, so please name this using TitleCase. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:47: friend class PerfProviderTest; Please don't friend test classes -- it allows tests to muck arbitrarily with internal state. Instead, please expose only the necessary internal state via protected methods, which you expose or override in a test-specific subclass. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:27: PerfDataProto& proto = *result; Again, why don't you use the arrow operator? It can sometimes be clearer to use a reference than a longer name, but in this case, there's really no increased clarity to using the reference. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:30: PerfDataProto_PerfFileAttr& file_attr = *proto.add_file_attrs(); I'd use a pointer here, too. In general, bare references are less commonly used than pointers. Mostly, they're used where STL functions return bare references, e.g. std::vector::operator[]. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:83: std::vector<uint8_t>(result_string.begin(), result_string.end()); Why not either (a) return a string, or (b) call SerializeMessageToVector, which you've defined below? https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:100: class MockIncognitoObserver : public IncognitoObserverInterface { nit: Please name this "TestIncognitoObserver" or "FakeIncognitoObserver" -- there aren't any mocked methods here, in the gMock sense, which is what "MockFoo" classes usually refer to. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:115: }; nit: Please move this class to the anonymous namespace, rather than defining it as an inner class. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:129: EXPECT_TRUE(cached_perf_data().empty()); Rather than running this in the setup for each test case, please create a separate (tiny) test case that tests just this one thing. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:131: EXPECT_FALSE(incognito_observer_.incognito_launched());\ nit: Please remove the trailing "\" https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:131: EXPECT_FALSE(incognito_observer_.incognito_launched());\ It's a little weird to be setting expectations like this in the setup code, since it's not really expectations about the class under test. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:134: sampled_profile_->set_trigger_event(SampledProfile::PERIODIC_COLLECTION); Could all of this be done in the constructor, and all of TearDown() be done in the destructor? https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:138: perf_provider_.reset(NULL); nit: Just reset() is fine; but FYI, "nullptr" is preferred to "NULL", as it is more strongly typed. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:142: incognito_observer_.set_incognito_launched(false); No need to do this, as the observer will be recreated for each test. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:149: } nit: Tests should just access this directly. If the issue is that the cached perf data is private, think about whether it's really appropriate for tests to be accessing it in the first place. Most likely not -- it's better to test the public API, i.e. GetSampledProfiles(). https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:154: const SampledProfile& sampled_profile, Why not just pass scoped_ptr's to this function directly? https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:160: MockIncognitoObserver* incognito_observer_copy = new MockIncognitoObserver; nit: Please declare this as a scoped ptr. Also, can you not combine the construction and the assignment, i.e. by calling the copy constructor here? https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:161: SampledProfile* sampled_profile_copy = new SampledProfile; Ditto. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:183: // SetUp() resets it to the default value. The constructor runs precisely as often as SetUp(), so there's no sense in which SetUp() resets this value. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:184: scoped_ptr<SampledProfile> sampled_profile_; IMO it would be clearer not to define this as part of the fixture class at all, and instead to simply have each test create the SampledProfile that it needs. You could have a helper function to create the default SampledProfile if you really want, but it doesn't look like that would save much code, since the default SampledProfile seems to only have a single field set. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:191: std::vector<uint8_t> perf_stat_raw_; Why do you need both variants here? https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:199: 0, // Zero means successful. nit: Perhaps define constants like "const int kSuccesss = 0" at the top of the file.
https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.cc:403: if (!perf_stat.empty()) { On 2015/07/01 03:09:02, Ilya Sherman wrote: > nit: I think it would be clearer to simply write this case as "else", maybe with > a DCHECK. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:29: class IncognitoObserverInterface { On 2015/07/01 03:09:02, Ilya Sherman wrote: > nit: If you really need this class, please omit the "Interface" part of the > name. I'd prefer, though, that you simply extracted the > WindowedIncognitoObserver class to a separate file, and marked the appropriate > method as virtual if need be. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:35: virtual bool incognito_launched() const = 0; On 2015/07/01 03:09:02, Ilya Sherman wrote: > nit: virtual functions are never trivial/inline-able, so please name this using > TitleCase. Done.
https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos.h (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos.h:47: friend class PerfProviderTest; On 2015/07/01 03:09:02, Ilya Sherman wrote: > Please don't friend test classes -- it allows tests to muck arbitrarily with > internal state. Instead, please expose only the necessary internal state via > protected methods, which you expose or override in a test-specific subclass. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:100: class MockIncognitoObserver : public IncognitoObserverInterface { On 2015/07/01 03:09:03, Ilya Sherman wrote: > nit: Please name this "TestIncognitoObserver" or "FakeIncognitoObserver" -- > there aren't any mocked methods here, in the gMock sense, which is what > "MockFoo" classes usually refer to. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:115: }; On 2015/07/01 03:09:02, Ilya Sherman wrote: > nit: Please move this class to the anonymous namespace, rather than defining it > as an inner class. Done.
https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:27: PerfDataProto& proto = *result; On 2015/07/01 03:09:03, Ilya Sherman wrote: > Again, why don't you use the arrow operator? It can sometimes be clearer to use > a reference than a longer name, but in this case, there's really no increased > clarity to using the reference. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:30: PerfDataProto_PerfFileAttr& file_attr = *proto.add_file_attrs(); On 2015/07/01 03:09:02, Ilya Sherman wrote: > I'd use a pointer here, too. In general, bare references are less commonly used > than pointers. Mostly, they're used where STL functions return bare references, > e.g. std::vector::operator[]. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:83: std::vector<uint8_t>(result_string.begin(), result_string.end()); On 2015/07/01 03:09:03, Ilya Sherman wrote: > Why not either (a) return a string, or (b) call SerializeMessageToVector, which > you've defined below? Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:129: EXPECT_TRUE(cached_perf_data().empty()); On 2015/07/01 03:09:03, Ilya Sherman wrote: > Rather than running this in the setup for each test case, please create a > separate (tiny) test case that tests just this one thing. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:131: EXPECT_FALSE(incognito_observer_.incognito_launched());\ On 2015/07/01 03:09:03, Ilya Sherman wrote: > nit: Please remove the trailing "\" Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:131: EXPECT_FALSE(incognito_observer_.incognito_launched());\ On 2015/07/01 03:09:03, Ilya Sherman wrote: > It's a little weird to be setting expectations like this in the setup code, > since it's not really expectations about the class under test. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:138: perf_provider_.reset(NULL); On 2015/07/01 03:09:02, Ilya Sherman wrote: > nit: Just reset() is fine; but FYI, "nullptr" is preferred to "NULL", as it is > more strongly typed. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:142: incognito_observer_.set_incognito_launched(false); On 2015/07/01 03:09:03, Ilya Sherman wrote: > No need to do this, as the observer will be recreated for each test. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:149: } On 2015/07/01 03:09:02, Ilya Sherman wrote: > nit: Tests should just access this directly. If the issue is that the cached > perf data is private, think about whether it's really appropriate for tests to > be accessing it in the first place. Most likely not -- it's better to test the > public API, i.e. GetSampledProfiles(). Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:154: const SampledProfile& sampled_profile, On 2015/07/01 03:09:03, Ilya Sherman wrote: > Why not just pass scoped_ptr's to this function directly? Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:160: MockIncognitoObserver* incognito_observer_copy = new MockIncognitoObserver; On 2015/07/01 03:09:03, Ilya Sherman wrote: > nit: Please declare this as a scoped ptr. Also, can you not combine the > construction and the assignment, i.e. by calling the copy constructor here? Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:161: SampledProfile* sampled_profile_copy = new SampledProfile; On 2015/07/01 03:09:02, Ilya Sherman wrote: > Ditto. Done https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:183: // SetUp() resets it to the default value. On 2015/07/01 03:09:03, Ilya Sherman wrote: > The constructor runs precisely as often as SetUp(), so there's no sense in which > SetUp() resets this value. Acknowledged. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:184: scoped_ptr<SampledProfile> sampled_profile_; On 2015/07/01 03:09:02, Ilya Sherman wrote: > IMO it would be clearer not to define this as part of the fixture class at all, > and instead to simply have each test create the SampledProfile that it needs. > You could have a helper function to create the default SampledProfile if you > really want, but it doesn't look like that would save much code, since the > default SampledProfile seems to only have a single field set. Done. https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:191: std::vector<uint8_t> perf_stat_raw_; On 2015/07/01 03:09:03, Ilya Sherman wrote: > Why do you need both variants here? It's used by BothPerfDataProtoAndPerfStatProto(). https://codereview.chromium.org/1218583002/diff/70009/chrome/browser/metrics/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:199: 0, // Zero means successful. On 2015/07/01 03:09:03, Ilya Sherman wrote: > nit: Perhaps define constants like "const int kSuccesss = 0" at the top of the > file. Done.
https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:210: EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile.perf_data())); Calling ByteSize() in SerializeMessageToVector() from the line causes a segfault. I'm not sure why.
https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:95: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:98: class PerfProviderForTesting : public PerfProvider { Optional nit: I'd name this "TestPerfProvider", for consistency with the above class's name. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:101: const scoped_ptr<TestIncognitoObserver>& incognito_observer, I'd expect that you could replace this entire method with just "using PerfProvider::ParseOutputProtoIfValid". https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:109: new TestIncognitoObserver(*incognito_observer)); Why do you make a copy, rather than passing in the original object? https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:111: new SampledProfile(sampled_profile)); Ditto. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:128: } Please call GetSampledProfiles() directly from the individual test cases. That makes it easier for the reader to see what each test is doing. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:133: const std::vector<SampledProfile> stored_profiles() const { nit: I'd recommend omitting this method entirely; but if you keep it, please return by const-ref. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:139: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:177: std::vector<uint8_t> perf_stat_raw_; What I meant was: Why do you need to store the protobufs both as protos and as vectors? https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:210: EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile.perf_data())); On 2015/07/01 22:16:07, Simon Que wrote: > Calling ByteSize() in SerializeMessageToVector() from the line causes a > segfault. I'm not sure why. I'm also not sure. You could try gdb/lldb. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... File chrome/browser/metrics/windowed_incognito_observer.h (right): https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/windowed_incognito_observer.h:14: // This class must be created and used on the UI thread. It watches for any nit: I'd omit the sentence about being used on the UI thread. That's pretty much the default assumption for Chromium code. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/windowed_incognito_observer.h:31: bool incognito_launched_; Class members should always be private (outside of test code, where text fixture classes can have protected members). However, it's fine to expose a protected set_incognito_launched() method. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/windowed_incognito_observer.h:36: }; nit: DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:95: }; On 2015/07/02 00:40:16, Ilya Sherman wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:98: class PerfProviderForTesting : public PerfProvider { On 2015/07/02 00:40:16, Ilya Sherman wrote: > Optional nit: I'd name this "TestPerfProvider", for consistency with the above > class's name. Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:109: new TestIncognitoObserver(*incognito_observer)); On 2015/07/02 00:40:16, Ilya Sherman wrote: > Why do you make a copy, rather than passing in the original object? The object will get destroyed by ParseOutputProtoIfValid(). I'd have to reset it after each call. Is that an acceptable model? https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:128: } On 2015/07/02 00:40:16, Ilya Sherman wrote: > Please call GetSampledProfiles() directly from the individual test cases. That > makes it easier for the reader to see what each test is doing. Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:133: const std::vector<SampledProfile> stored_profiles() const { On 2015/07/02 00:40:16, Ilya Sherman wrote: > nit: I'd recommend omitting this method entirely; but if you keep it, please > return by const-ref. Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:139: }; On 2015/07/02 00:40:16, Ilya Sherman wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:177: std::vector<uint8_t> perf_stat_raw_; On 2015/07/02 00:40:16, Ilya Sherman wrote: > What I meant was: Why do you need to store the protobufs both as protos and as > vectors? Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:210: EXPECT_EQ(perf_data_raw_, SerializeMessageToVector(profile.perf_data())); On 2015/07/02 00:40:16, Ilya Sherman wrote: > On 2015/07/01 22:16:07, Simon Que wrote: > > Calling ByteSize() in SerializeMessageToVector() from the line causes a > > segfault. I'm not sure why. > > I'm also not sure. You could try gdb/lldb. Don't see this anymore. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... File chrome/browser/metrics/windowed_incognito_observer.h (right): https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/windowed_incognito_observer.h:14: // This class must be created and used on the UI thread. It watches for any On 2015/07/02 00:40:16, Ilya Sherman wrote: > nit: I'd omit the sentence about being used on the UI thread. That's pretty > much the default assumption for Chromium code. Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/windowed_incognito_observer.h:31: bool incognito_launched_; On 2015/07/02 00:40:16, Ilya Sherman wrote: > Class members should always be private (outside of test code, where text fixture > classes can have protected members). However, it's fine to expose a protected > set_incognito_launched() method. Done. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/windowed_incognito_observer.h:36: }; On 2015/07/02 00:40:16, Ilya Sherman wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done.
I've removed the wrapper method. I had to restructure some things, especially the TestIncognitoObserver class, to work better with the new code. https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/130001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:101: const scoped_ptr<TestIncognitoObserver>& incognito_observer, On 2015/07/02 00:40:16, Ilya Sherman wrote: > I'd expect that you could replace this entire method with just "using > PerfProvider::ParseOutputProtoIfValid". Done.
Thanks, Simon -- this is looking much better now. I think probably the final round of significant comments: https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:37: void GetExamplePerfDataProto(PerfDataProto* proto) { nit: Now that this function only produces a single result, please return that result rather than passing in an out-param. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:63: void GetExamplePerfStatProto(PerfStatProto* proto) { nit: Now that this function only produces a single result, please return that result rather than passing in an out-param. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:93: } Please either pass an enum value or name the method something more like CreateWithIncognitoLaunched. A call site like "Create(false)" is not very clear -- the reader has to check what the "false" means. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:240: ASSERT_TRUE(stored_profiles.empty()); nit: EXPECT_TRUE https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:421: stored_profiles3.end()); Rather than accumulating the stored_profiles vector within the test, please either change the test to call GetSampledProfiles just once, or test the individual profiles as they're added (i.e. right next to the individual GetSampledProfiles calls). https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:422: EXPECT_EQ(3U, stored_profiles.size()); nit: This should be an ASSERT_EQ, since otherwise calls like "stored_profiles[2]" might crash.
https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:37: void GetExamplePerfDataProto(PerfDataProto* proto) { On 2015/07/07 00:43:56, Ilya Sherman (Away July 8-13) wrote: > nit: Now that this function only produces a single result, please return that > result rather than passing in an out-param. Done. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:63: void GetExamplePerfStatProto(PerfStatProto* proto) { On 2015/07/07 00:43:56, Ilya Sherman (Away July 8-13) wrote: > nit: Now that this function only produces a single result, please return that > result rather than passing in an out-param. Done. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:93: } On 2015/07/07 00:43:56, Ilya Sherman (Away July 8-13) wrote: > Please either pass an enum value or name the method something more like > CreateWithIncognitoLaunched. A call site like "Create(false)" is not very clear > -- the reader has to check what the "false" means. Done. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:240: ASSERT_TRUE(stored_profiles.empty()); On 2015/07/07 00:43:56, Ilya Sherman (Away July 8-13) wrote: > nit: EXPECT_TRUE Done. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:421: stored_profiles3.end()); On 2015/07/07 00:43:56, Ilya Sherman (Away July 8-13) wrote: > Rather than accumulating the stored_profiles vector within the test, please > either change the test to call GetSampledProfiles just once, or test the > individual profiles as they're added (i.e. right next to the individual > GetSampledProfiles calls). Done. Went with the latter solution. https://codereview.chromium.org/1218583002/diff/190001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:422: EXPECT_EQ(3U, stored_profiles.size()); On 2015/07/07 00:43:56, Ilya Sherman (Away July 8-13) wrote: > nit: This should be an ASSERT_EQ, since otherwise calls like > "stored_profiles[2]" might crash. Done.
Thanks. LGTM % a final nit: https://codereview.chromium.org/1218583002/diff/210001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/210001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:105: static scoped_ptr<WindowedIncognitoObserver> CreateWithIncognitoLaunched() { nit: I was actually suggesting a single factory function: static scoped_ptr<WindowedIncognitoObserver> CreateWithIncognitoLaunched( bool incognito_launched) { scoped_ptr<TestIncognitoObserver> observer(new TestIncognitoObserver); observer->set_incognito_launched(incognito_launched); return observer; } Note that I'm also suggesting changing the TestIncognitoObserver constructor to take no parameters -- IMO that makes the code clearer, for the same reason as changing the naming of this function does. (I'm pretty sure that .Pass() isn't needed on the return line,but please add it if I'm mistaken.) https://codereview.chromium.org/1218583002/diff/210001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:107: new TestIncognitoObserver(true)).Pass(); FYI, you can write code like this more succinctly by using make_scoped_ptr, and omitting ".Pass()". (No action needed for this comment, since I'd prefer that you follow my suggestion above.)
PTAL to make sure I did it right. https://codereview.chromium.org/1218583002/diff/210001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/210001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:105: static scoped_ptr<WindowedIncognitoObserver> CreateWithIncognitoLaunched() { On 2015/07/07 01:15:17, Ilya Sherman (Away July 8-13) wrote: > nit: I was actually suggesting a single factory function: > > static scoped_ptr<WindowedIncognitoObserver> CreateWithIncognitoLaunched( > bool incognito_launched) { > scoped_ptr<TestIncognitoObserver> observer(new TestIncognitoObserver); > observer->set_incognito_launched(incognito_launched); > return observer; > } > > Note that I'm also suggesting changing the TestIncognitoObserver constructor to > take no parameters -- IMO that makes the code clearer, for the same reason as > changing the naming of this function does. > > (I'm pretty sure that .Pass() isn't needed on the return line,but please add it > if I'm mistaken.) Done.
https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:98: TestIncognitoObserver* observer = new TestIncognitoObserver; I tried to create a scoped_ptr<WindowedIncognitoObserver> and then set the flag, but got a compiler error saying set_incognito_launched(bool) is protected.
https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:98: TestIncognitoObserver* observer = new TestIncognitoObserver; Please declare this as a scoped_ptr. https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:100: return scoped_ptr<WindowedIncognitoObserver>(observer); And this line would just be "return observer;"
https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:98: TestIncognitoObserver* observer = new TestIncognitoObserver; On 2015/07/07 01:32:57, Simon Que wrote: > I tried to create a scoped_ptr<WindowedIncognitoObserver> and then set the flag, > but got a compiler error saying set_incognito_launched(bool) is protected. Ah, you want a scoped_ptr<TestIncognitoObserver>.
On 2015/07/07 01:33:53, Ilya Sherman (Away July 8-13) wrote: > https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... > File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): > > https://codereview.chromium.org/1218583002/diff/230001/chrome/browser/metrics... > chrome/browser/metrics/perf_provider_chromeos_unittest.cc:98: > TestIncognitoObserver* observer = new TestIncognitoObserver; > On 2015/07/07 01:32:57, Simon Que wrote: > > I tried to create a scoped_ptr<WindowedIncognitoObserver> and then set the > flag, > > but got a compiler error saying set_incognito_launched(bool) is protected. > > Ah, you want a scoped_ptr<TestIncognitoObserver>. Done. But I had to Pass() the pointer to the returned scoped_ptr<WindowedIncognitoObserver>.
sque@chromium.org changed reviewers: + hashimoto@chromium.org, stevenjb@chromium.org
Adding OWNERS of chromeos/dbus.
https://codereview.chromium.org/1218583002/diff/250001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/250001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:100: return scoped_ptr<WindowedIncognitoObserver>(observer.Pass()); Hmm, does the code not compile if you simply write "return observer.Pass()"?
https://codereview.chromium.org/1218583002/diff/250001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/250001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:100: return scoped_ptr<WindowedIncognitoObserver>(observer.Pass()); On 2015/07/07 01:41:33, Ilya Sherman (Away July 8-13) wrote: > Hmm, does the code not compile if you simply write "return observer.Pass()"? It does.
Extracting WindowedIncognitoObserver just to override it for a unit test seems a
bit heavy handed; every additional .cc file carries a certain amount of
maintenance overhead.
Have you considered adding a Delegate to PerfProvider instead? Something like:
class PerfProvider {
class Delegate {
virtual bool WasIncognitoBrowserLaunched() = 0;
...
};
...
set_delegate_for_test(scoped_ptr<Delegate> delegate);
...
scoped_ptr<Delegate> delegate_;
};
Then you could make the default delegate include WindowedIncognitoObserver
(defined locally in perf_provider_chromeos.cc as before), and set a test
delegate for testing.
https://codereview.chromium.org/1218583002/diff/250001/chromeos/dbus/debug_da...
File chromeos/dbus/debug_daemon_client.cc (right):
https://codereview.chromium.org/1218583002/diff/250001/chromeos/dbus/debug_da...
chromeos/dbus/debug_daemon_client.cc:481: }
nit: no {}
https://codereview.chromium.org/1218583002/diff/250001/chromeos/dbus/debug_da...
chromeos/dbus/debug_daemon_client.cc:489: const uint8* buffer = NULL;
nullptr
On 2015/07/07 02:03:51, stevenjb wrote:
> Extracting WindowedIncognitoObserver just to override it for a unit test seems
a
> bit heavy handed; every additional .cc file carries a certain amount of
> maintenance overhead.
>
> Have you considered adding a Delegate to PerfProvider instead? Something like:
>
> class PerfProvider {
> class Delegate {
> virtual bool WasIncognitoBrowserLaunched() = 0;
> ...
> };
> ...
> set_delegate_for_test(scoped_ptr<Delegate> delegate);
> ...
> scoped_ptr<Delegate> delegate_;
> };
>
>
> Then you could make the default delegate include WindowedIncognitoObserver
> (defined locally in perf_provider_chromeos.cc as before), and set a test
> delegate for testing.
FWIW, I don't particularly mind either solution, but I'm curious what the
additional maintenance that you're referring to is. Just gypi and BUILD.gn
files, or something more as well?
https://codereview.chromium.org/1218583002/diff/250001/chromeos/dbus/debug_da... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/1218583002/diff/250001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:481: } On 2015/07/07 02:03:51, stevenjb wrote: > nit: no {} Done. https://codereview.chromium.org/1218583002/diff/250001/chromeos/dbus/debug_da... chromeos/dbus/debug_daemon_client.cc:489: const uint8* buffer = NULL; On 2015/07/07 02:03:51, stevenjb wrote: > nullptr Done.
On 2015/07/07 02:03:51, stevenjb wrote:
> Extracting WindowedIncognitoObserver just to override it for a unit test seems
a
> bit heavy handed; every additional .cc file carries a certain amount of
> maintenance overhead.
>
> Have you considered adding a Delegate to PerfProvider instead? Something like:
>
> class PerfProvider {
> class Delegate {
> virtual bool WasIncognitoBrowserLaunched() = 0;
> ...
> };
> ...
> set_delegate_for_test(scoped_ptr<Delegate> delegate);
> ...
> scoped_ptr<Delegate> delegate_;
> };
>
>
> Then you could make the default delegate include WindowedIncognitoObserver
> (defined locally in perf_provider_chromeos.cc as before), and set a test
> delegate for testing.
This will not work because our current model is one observer per dbus call, not
one observer for the whole PerfProvider class.
The WindowedIncognitoObserver actually doesn't have a mechanism for setting from
true to false. The rule is, once it detects an incognito window, it will always
return true.
I had previously defined an interface class, IncognitoObserverInterface, that
was similar to what you describe. I changed away from it in Patch Set 6.
On 2015/07/07 04:42:32, Simon Que wrote:
> On 2015/07/07 02:03:51, stevenjb wrote:
> > Extracting WindowedIncognitoObserver just to override it for a unit test
seems
> a
> > bit heavy handed; every additional .cc file carries a certain amount of
> > maintenance overhead.
> >
> > Have you considered adding a Delegate to PerfProvider instead? Something
like:
> >
> > class PerfProvider {
> > class Delegate {
> > virtual bool WasIncognitoBrowserLaunched() = 0;
> > ...
> > };
> > ...
> > set_delegate_for_test(scoped_ptr<Delegate> delegate);
> > ...
> > scoped_ptr<Delegate> delegate_;
> > };
> >
> >
> > Then you could make the default delegate include WindowedIncognitoObserver
> > (defined locally in perf_provider_chromeos.cc as before), and set a test
> > delegate for testing.
>
> This will not work because our current model is one observer per dbus call,
not
> one observer for the whole PerfProvider class.
>
> The WindowedIncognitoObserver actually doesn't have a mechanism for setting
from
> true to false. The rule is, once it detects an incognito window, it will
always
> return true.
>
> I had previously defined an interface class, IncognitoObserverInterface, that
> was similar to what you describe. I changed away from it in Patch Set 6.
I don't think you understood my suggestion, but if Ilya is fine with this I
don't think it is important enough to spend time more on. As Ilya mentioned,
there isn't a whole lot of added overhead for extra files, it just adds a little
more effort when searching through code and trying to follow it.
owner rubberstamp lgtm for chromeos/dbus.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1218583002/#ps290001 (title: "Responded to stevenjb's comments: removed {}s, NULL -> nullptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218583002/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1218583002/#ps310001 (title: ""virtual" is redundant with "override"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218583002/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/07/07 19:57:32, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) On 2015/07/07 19:57:32, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) The problem was that OnUserLoggedIn() was not called. Having fixed that, my code failed the following CHECK: [5125:5125:0707/152305:992293096185:FATAL:thread_task_runner_handle.cc(23)] Check failed: current. #0 0x7fe38f608979 base::debug::StackTrace::StackTrace() #1 0x7fe38f640785 logging::LogMessage::~LogMessage() #2 0x7fe38f6d7032 base::ThreadTaskRunnerHandle::Get() #3 0x7fe38f6f68ad base::Timer::GetTaskRunner() #4 0x7fe38f6f6526 base::Timer::PostNewScheduledTask() #5 0x7fe38f6f625c base::Timer::Reset() #6 0x7fe38f6f6127 base::Timer::Start() #7 0x7fe38d9d3dd1 base::BaseTimerMethodPointer<>::Start() #8 0x7fe38d9d2c1a metrics::PerfProvider::ScheduleIntervalCollection() #9 0x7fe38d9d2a19 metrics::PerfProvider::OnUserLoggedIn() #10 0x7fe38d9d2611 metrics::PerfProvider::LoginObserver::LoggedInStateChanged() #11 0x7fe38ffbaf75 chromeos::LoginState::NotifyObservers() #12 0x7fe38ffbabf4 chromeos::LoginState::SetLoggedInState() #13 0x7fe38bb6d9a4 metrics::PerfProviderTest::SetUp() The problem is that there's a base::Timer in PerfProvider that can't run during a unit test. I tried to replace it with a MockTimer but got the following message: Received signal 11 <unknown> 000000000000 #0 0x7f68eedaaed9 base::debug::StackTrace::StackTrace() #1 0x7f68eedaa7a8 base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f68e9071340 <unknown> #3 0x7f68ebf6cc6b sync_file_system::RemoteServiceState testing::internal::InvokeHelper<sync_file_system::RemoteServiceState, std::tr1::tuple<> >::InvokeMethod<sync_file_system::MockRemoteFileSyncService, sync_file_system::RemoteServiceState (sync_file_system::MockRemoteFileSyncService::*)() const>(sync_file_system::MockRemoteFileSyncService*, sync_file_system::RemoteServiceState (sync_file_system::MockRemoteFileSyncService::*)() const, std::tr1::tuple<> const&) #4 0x7f68ee4d0a91 testing::internal::HandleExceptionsInMethodIfSupported<>() #5 0x7f68ee4c2fe5 testing::TestInfo::Run() #6 0x7f68ee4c358a testing::TestCase::Run() #7 0x7f68ee4c9306 testing::internal::UnitTestImpl::RunAllTests() #8 0x7f68ebf6cc81 sync_file_system::RemoteServiceState testing::internal::InvokeHelper<sync_file_system::RemoteServiceState, std::tr1::tuple<> >::InvokeMethod<sync_file_system::MockRemoteFileSyncService, sync_file_system::RemoteServiceState (sync_file_system::MockRemoteFileSyncService::*)() const>(sync_file_system::MockRemoteFileSyncService*, sync_file_system::RemoteServiceState (sync_file_system::MockRemoteFileSyncService::*)() const, std::tr1::tuple<> const&) #9 0x7f68ee4d0a91 testing::internal::HandleExceptionsInMethodIfSupported<>() #10 0x7f68ee4c80a4 testing::UnitTest::Run() #11 0x7f68f692b7e8 RUN_ALL_TESTS() Not sure what's going wrong.... I can't tell from the error message. As an alternative, I could make virtual all methods that use the timer and override them in TestPerfProvider.
I used a TestSimpleTaskRunner to handle the TaskRunner stuff... but I'm still
getting that stack trace.
Using gdb, I instead see:
Program received signal SIGSEGV, Segmentation fault.
0x000055555746296b in
testing::internal::InvokeHelper<sync_file_system::RemoteServiceState,
std::tr1::tuple<> >::InvokeMethod<sync_file_system::MockRemoteFileSyncService,
sync_file_system::RemoteServiceState
(sync_file_system::MockRemoteFileSyncService::*)()
const>(sync_file_system::MockRemoteFileSyncService*,
sync_file_system::RemoteServiceState
(sync_file_system::MockRemoteFileSyncService::*)() const, std::tr1::tuple<>
const&) (obj_ptr=0x329bf6849e20,
method_ptr=&virtual table offset 93825009482002) at
../../testing/gmock/include/gmock/gmock-generated-actions.h:65
65 return (obj_ptr->*method_ptr)();
And one level up in the stack trace:
#1 0x00005555599c6547 in
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>
(object=0x329bf6849e20,
method=&virtual table offset 93825009482002, location=0x5555646de117 "the
test fixture's destructor") at ../../testing/gtest/src/gtest.cc:2417
I suspect something's wrong with the way the task runner and handle are being
cleaned up.
https://chromiumcodereview.appspot.com/1218583002/diff/350001/chrome/browser/... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://chromiumcodereview.appspot.com/1218583002/diff/350001/chrome/browser/... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:144: chromeos::LoginState::LOGGED_IN_USER_REGULAR); By the way, this looks like it's affecting global state. Should this be undone somewhere in TearDown()?
https://codereview.chromium.org/1218583002/diff/350001/chrome/browser/metrics... File chrome/browser/metrics/perf_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/1218583002/diff/350001/chrome/browser/metrics... chrome/browser/metrics/perf_provider_chromeos_unittest.cc:144: chromeos::LoginState::LOGGED_IN_USER_REGULAR); On 2015/07/08 01:59:12, Ilya Sherman (Away July 8-13) wrote: > By the way, this looks like it's affecting global state. Should this be undone > somewhere in TearDown()? It shouldn't be, since TearDown() will call LoginState::Shutdown, destroying the singleton instance of LoginState.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1218583002/#ps350001 (title: "Create TestSimpleTaskRunner and handle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218583002/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1218583002/#ps370001 (title: "Check has_ms_after_login() instead of ms_after_login() value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218583002/370001
Message was sent while issue was closed.
Committed patchset #20 (id:370001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/e0260fd891172eb94a614751b1c77596e476bab2 Cr-Commit-Position: refs/heads/master@{#337910} |
