Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1885673003:
Create and integrate a metrics service client into Blimp engine. (Closed)
|
Created:
4 years, 8 months ago by Jess Modified:
4 years, 7 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate and integrate a metrics service client into Blimp engine.
This will allow Blimp to collect and upload metrics to aid in
development.
BUG=592757
Committed: https://crrev.com/0be9daea216b9144830da20bcb3e2a56591207e6
Cr-Commit-Position: refs/heads/master@{#391360}
Patch Set 1 : Initial work based on in review BlimpPrefStore. #Patch Set 2 : Updates based on PrefStore changes before commit. #
Total comments: 27
Patch Set 3 : Clean ups from initial reviews. #
Total comments: 66
Patch Set 4 : Comment follow ups. TODO: client unittest and additional refactoring. #
Total comments: 4
Patch Set 5 : Adding testing and rest of refactoring clean up. #Patch Set 6 : Handling recent BUILD checkin. #
Total comments: 29
Patch Set 7 : Resolve Analyze step failures in certain tests. #Patch Set 8 : More build deps not caught by blimp build target. #Patch Set 9 : More build dep cleanup. #Patch Set 10 : Adding //net to app_metrics. #
Total comments: 14
Patch Set 11 : Comment follow ups. #
Total comments: 12
Patch Set 12 : Assorted cleanups from review. #
Total comments: 6
Patch Set 13 : Move newline and ASSERT as requested. #
Messages
Total messages: 33 (10 generated)
Description was changed from ========== Create and integrate a metrics service client into Blimp. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ========== to ========== Create and integrate a metrics service client into Blimp. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ==========
jessicag@chromium.org changed reviewers: + asvitkine@chromium.org, battre@chromium.org, bauerb@chromium.org, wez@chromium.org
jessicag@chromium.org changed required reviewers: + asvitkine@chromium.org, wez@chromium.org
This is for the collection of metrics data in the Blimp engine. The structure is largely based on android_webview metrics integration as suggested by Alexei.
Description was changed from ========== Create and integrate a metrics service client into Blimp. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ========== to ========== Create and integrate a metrics service client into Blimp engine. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ==========
prefs/ usage LGTM; just some general nits: https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:25: namespace { Nit: I find it more readable to add empty lines after the opening and before the closing of a namespace (or a group thereof). https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:36: return client_info; You can just return nullptr here. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:84: if (is_reporting_enabled()) { Nit: Braces are optional for single-line bodies. (I don't know whether Blimp prefers one way or the other; I would look around the code to see). https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:151: return scoped_ptr<metrics::MetricsLogUploader>( You can use base::WrapUnique for this; it's a bit shorter. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:20: class PerfService; PrefService :) But the fact that this doesn't cause an error tells me the forward declaration isn't necessary anyway. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:276: } Nit: empty line after this one.
https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:57: base::Bind(&StoreClientInfo), base::Bind(&LoadClientInfo)); Hmm, I wonder if we should just change MetricsStateManager to take a MetricsServiceClient and remove these three callback params in favor of just the client param? Anyway, that should be done in a separate CL. I'm okay if you just add a TODO about it if you'd rather not do that. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:114: return metrics::ChromeUserMetricsExtension::CHROME; Change this to BLIMP. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:133: return ""; std::string() https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:153: // URLRequestContextGetter* that remains valid for class lifetime Sounds like this comment should be on the field in the class, rather than here. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:68: bool is_reporting_enabled(); Nit: IsReportingEnabled() Or if you want to keep it using hacker_style, inline its impl. Can you make it const? https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:73: scoped_ptr<metrics::MetricsStateManager> metrics_state_manager_; Nit: Use unique_ptr. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.cc:105: user_prefs::PrefRegistrySyncable* pref_registry = Nit: unique_ptr
Follow up items: How best to track the proposed MetricsStateManager work. Blimp engine style question. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:25: namespace { On 2016/04/25 08:21:36, Bernhard Bauer wrote: > Nit: I find it more readable to add empty lines after the opening and before the > closing of a namespace (or a group thereof). Done. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:36: return client_info; On 2016/04/25 08:21:36, Bernhard Bauer wrote: > You can just return nullptr here. Done. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:57: base::Bind(&StoreClientInfo), base::Bind(&LoadClientInfo)); On 2016/04/25 20:44:25, Alexei Svitkine wrote: > Hmm, I wonder if we should just change MetricsStateManager to take a > MetricsServiceClient and remove these three callback params in favor of just the > client param? Anyway, that should be done in a separate CL. I'm okay if you just > add a TODO about it if you'd rather not do that. A TODO or a crbug? I am thinking in terms of just Blimp or more broadly clients. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:84: if (is_reporting_enabled()) { On 2016/04/25 08:21:36, Bernhard Bauer wrote: > Nit: Braces are optional for single-line bodies. (I don't know whether Blimp > prefers one way or the other; I would look around the code to see). Acknowledged. Wez, please let me know if there is a preference on engine. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:114: return metrics::ChromeUserMetricsExtension::CHROME; On 2016/04/25 20:44:25, Alexei Svitkine wrote: > Change this to BLIMP. There is no such enum option. I believe we discussed using OS instead of product to separate Blimp from other Chrome products. Please feel free to reach out to me offline if I misunderstand the usage here. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:133: return ""; On 2016/04/25 20:44:25, Alexei Svitkine wrote: > std::string() Done. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:151: return scoped_ptr<metrics::MetricsLogUploader>( On 2016/04/25 08:21:36, Bernhard Bauer wrote: > You can use base::WrapUnique for this; it's a bit shorter. Done. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:153: // URLRequestContextGetter* that remains valid for class lifetime On 2016/04/25 20:44:25, Alexei Svitkine wrote: > Sounds like this comment should be on the field in the class, rather than here. Done. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:68: bool is_reporting_enabled(); On 2016/04/25 20:44:25, Alexei Svitkine wrote: > Nit: IsReportingEnabled() > > Or if you want to keep it using hacker_style, inline its impl. > > Can you make it const? Why not both? :) https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:73: scoped_ptr<metrics::MetricsStateManager> metrics_state_manager_; On 2016/04/25 20:44:25, Alexei Svitkine wrote: > Nit: Use unique_ptr. Done. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.cc:105: user_prefs::PrefRegistrySyncable* pref_registry = On 2016/04/25 20:44:25, Alexei Svitkine wrote: > Nit: unique_ptr Unique ptr is not usable in this case since user_prefs::PrefRegistrySyncable has a private destructor. Using scoped_refptr instead. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:20: class PerfService; On 2016/04/25 08:21:36, Bernhard Bauer wrote: > PrefService :) But the fact that this doesn't cause an error tells me the > forward declaration isn't necessary anyway. Cleaned up. https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:276: } On 2016/04/25 08:21:36, Bernhard Bauer wrote: > Nit: empty line after this one. Done.
Yay for metrics! Do we have a plan to unit-test this in some fashion? Is there some existing pattern for testing MetricsServiceClient implementations, for example? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:24: base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_; This needs to be in the anonymous namespace, surely? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:28: // Standard interval between log uploads, in minutes Can you reword this to explain what the "standard interval" is actually for; otherwise the comment is almost the variable name verbatim. nit: End comment with . nit: Constant is already named kFooMinutes, so no need to mention units. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:29: const int kStandardUploadIntervalMinutes = 30; // Thirty minutes. nit: Trailing comment adds nothing ;) https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:31: bool IsReportingEnabled() { nit: Suggest providing a one-line comment explaining that the metrics state manager expects an is-enabled callback to be provided. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:32: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Why this thread-check here but nowhere else? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:37: // allows Windows Chrome to back up ClientInfo. They're no-ops for Blimp. IsReportingEnabled also seems to be a callback for that purpose? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:62: base::Bind(&StoreClientInfo), base::Bind(&LoadClientInfo)); nit: Is this the format "git cl format" gives? Seems strange to wrap the preceding two lines but not this one. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:64: metrics_service_.reset(new ::metrics::MetricsService( nit: Do we need the leading ::? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:70: content::BrowserThread::GetBlockingPool()))); We should be very careful about using content::BrowserThread stuff; means that we presumably have some constraints about when this instance must be created, and torn, down, so that it's dependencies are met, and those should be documented in the comment on the class. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:74: new metrics::GPUMetricsProvider)); nit: Why are these two registrations grouped together, separate from the profiler and call-stack ones? Is there something special about these two? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:108: // This is not used on required by Blimp, so these are no-ops. typo: on -> or I think it's reasonable to just say "We don't implement SetMetricsClientId since we don't use Breakpad." - the MetricsServiceClient interface docs should explain what the API would normally do. It's not clear what OnRecordingDisabled has to do with providing an Id to anything - it doesn't seem to even have a parameter? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:120: // Indicates product family, not reported platform. Not sure what this comment means by "reported platform"? Is this comment really specific to Blimp, or should the GetProduct comment in MetricsServiceClient interface be sufficient to understand? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:129: // Blimp doesn't use brand codes. Not clear from this comment what a brand-code even is? Sadly the MetricsServiceClient comment for it is not very helpful either :-/ https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:135: return metrics::SystemProfileProto::CHANNEL_UNKNOWN; nit: Hmmm, tricky; channel is used elsewhere to decide which APIs to support, so we should be careful that returning UNKNOWN here doesn't lead to some weird mismatch. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:139: // TODO(jessicag): Add in a meaningful version string. Excitingly I'm working on a CL that will add such a thing and was just wondering if we even needed it. Yay! https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:147: done_callback.Run(); Add one-line comment to clarify why it's OK to call the callback immediately, here and below? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:163: // Member of g/e/common/blimp_browser_context. This comment is already present in the class declaration of the member. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:34: // Based on AwMetricsServiceClient. This comment is somewhat cryptic, IMO! - It's not clear what "depends on blimp" means; are we saying that this is a MetricsServiceClient tailored to the Blimp Engine? - Not sure what you mean by "This singleton...for an app". - Not clear why metrics being always-on is remarkable; might be clearer to instead override the GetDefaultOptIn() method to always return ON, with a suitable comment? Presumably we also need a TODO w/ bug filed to change the behaviour before this is released? - Comment talks about threads, but doesn't itself have threads, nor any immediate mention of threads; in general it's best to avoid referring to details of the surrounding system in comments describing "leaf" classes of this sort, and instead document the assumptions their interface makes. In this case the MetricsServiceClient _owns_ the MetricsService, and it seems that the MetricsService expects only to be called from a single thread, so the constraint is just that Initialize and Finalize must be called on the same thread - the MetricsService takes care of calling to the MetricsServiceClient APIs on that same thread itself? - That this is based on AwMetricsServiceClient is only worth noting if there is some particular pattern that that class used that you wanted to copy - if it's just an example of another implementation then remove the comment; reader can use code-search to find other examples if need be! https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:38: static BlimpMetricsServiceClient* GetInstance(); Document this API - e.g. does it return a process-global singleton? Does/should the caller take a reference to it? Does it ever get deleted? Also, since this is a singleton and the only APIs it really has are Initialize() and Finalize(), did you consider making those stand alone methods in the blimp namespace and moving the class declaration into the .cc file? You'd then be able to safely get rid of the private ctor/dtor and friending, and the is_initialized bool (since you'd only create the instance in Initialize, so it'd always be initialized). https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:44: // metrics::MetricsServiceClient implementation: nit: End comment with . not : https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:66: bool is_initialized_; Suggest initializing this and other POD members in-line. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:70: // g_browser_process->system_request_context() I'd suggest replacing this entire comment with just "Used by NetMetricsLogUploader to create log-upload requests." https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:71: // In engine use subclass BlimpURLRequestContextGetter. If this detail is important for the caller to know then comment on the Initialize() call, where they provide the getter. If not then remove the comment, since it's not relevant to the implementation. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:72: // Member of g/e/common/blimp_browser_context. nit:What is g/e/? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:73: net::URLRequestContextGetter* request_context_; This class is ref-counted - take a lovely safe reference to it, please :D nit: This is the getter, not a context. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.cc:25: // Shows notifications which correspond to PersistentPrefStore's reading errors. Looks like the body of this function is missing? Is it supposed to be declared in-line? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.cc:105: void BlimpBrowserContext::InitPrefService() { If I understand this code correctly (big "if") this is doing three things: 1. Creating a PrefRegistry & registering the metrics service preferences with it. 2. Creating an in-memory preferences store, to hold metrics logs 3. Attaching it to that registry. It would therefore be helpful to break this code up into logical blocks with blob-level comments, i.e. something like: // Create a PrefRegistry and populate it with MetricsService prefs. ... // Create an in-memory PrefStore. ... // Create a PrefService binding the metrics PrefRegistry to our in-memory store. ... nit: It's also really unfortunate that those interfaces use bare pointers when many of the objects being passed-around are ref-counted, but I don't think there's much we can do about that in this CL, except perhaps mentioning in that third comment that the PrefService ends up owning the PrefRegistry and store. ;-( https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:36: // These map to BrowserMainParts::Pre/PostMainMessageLoopRun. nit: Do you mean "Called by BrowserMainParts::Pre/PostMainMessageLoopRun, respectively."? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:74: void InitPrefService(); nit: Blank line above this so it doesn't look like the IO comment applies to it too. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:77: std::unique_ptr<PrefService> pref_service_; nit: Blank line after this, to separate it from the other members, otherwise it looks like the comment applies to them too. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:263: browser_context_->Initialize(); nit: Add a one-line comment to explain what this is for? Seems strange that this only became required when we started doing metrics... https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:275: browser_context_->Finalize(); Is it important that this happen before we enter the BlimpEngineSession dtor, or could we move it there and avoid the explicit Finalize() API? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:87: // Finalizes the metrics reporting client. nit: Blank line before this comment, plz, to match the style of other comments in this code.
https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/20001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:114: return metrics::ChromeUserMetricsExtension::CHROME; On 2016/04/26 00:01:24, Jess wrote: > On 2016/04/25 20:44:25, Alexei Svitkine wrote: > > Change this to BLIMP. > > There is no such enum option. > > I believe we discussed using OS instead of product to separate Blimp from other > Chrome products. Please feel free to reach out to me offline if I misunderstand > the usage here. Ah right. I suggest mentioning this in the comment above. i.e.: // Note: Blimp still reports Chrome as the product, but uses a different OS string to differentiate itself from normal Chrome reports. I would also see if we can have the Blimp os name string be provided through this client interface, rather than what we currently have in place as an ifdef in the component. Maybe something like GetOsNameOverride() API on the client interface?
Wez: None of the other implementations I found had tests. I'll be following up on getters and setters, let me know if there is anything else you'd like me to cover. Also feel free to grab me offline to discuss further refactoring. I suspect I am missing something relatively small here. Alexei: I think that will take logic threading and a bit of thought on how it will interact with the existing interfaces for OS. Something will need to happen in the space for the current code the upload the correct OS metadata. Should we break off the refactoring into its own crbug? Am I overestimating the amount of thought this will take? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:24: base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_; On 2016/04/26 01:38:17, Wez wrote: > This needs to be in the anonymous namespace, surely? Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:28: // Standard interval between log uploads, in minutes On 2016/04/26 01:38:18, Wez wrote: > Can you reword this to explain what the "standard interval" is actually for; > otherwise the comment is almost the variable name verbatim. > > nit: End comment with . > nit: Constant is already named kFooMinutes, so no need to mention units. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:29: const int kStandardUploadIntervalMinutes = 30; // Thirty minutes. On 2016/04/26 01:38:18, Wez wrote: > nit: Trailing comment adds nothing ;) Removed. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:31: bool IsReportingEnabled() { On 2016/04/26 01:38:18, Wez wrote: > nit: Suggest providing a one-line comment explaining that the metrics state > manager expects an is-enabled callback to be provided. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:32: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2016/04/26 01:38:17, Wez wrote: > Why this thread-check here but nowhere else? Usually this call makes use of features the require the Browser UI thread, but it isn't a requirement here. I kept it as a sanity check, but in hindsight here is not the place for such a check. Removed. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:37: // allows Windows Chrome to back up ClientInfo. They're no-ops for Blimp. On 2016/04/26 01:38:18, Wez wrote: > IsReportingEnabled also seems to be a callback for that purpose? See comment above. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:62: base::Bind(&StoreClientInfo), base::Bind(&LoadClientInfo)); On 2016/04/26 01:38:18, Wez wrote: > nit: Is this the format "git cl format" gives? Seems strange to wrap the > preceding two lines but not this one. Ran git cl format. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:64: metrics_service_.reset(new ::metrics::MetricsService( On 2016/04/26 01:38:18, Wez wrote: > nit: Do we need the leading ::? No. Removed. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:70: content::BrowserThread::GetBlockingPool()))); On 2016/04/26 01:38:18, Wez wrote: > We should be very careful about using content::BrowserThread stuff; means that > we presumably have some constraints about when this instance must be created, > and torn, down, so that it's dependencies are met, and those should be > documented in the comment on the class. Honestly I am not quite sure here. Perhaps removal is the best option. We can always register it in a later cr. Thoughts? https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:74: new metrics::GPUMetricsProvider)); On 2016/04/26 01:38:18, Wez wrote: > nit: Why are these two registrations grouped together, separate from the > profiler and call-stack ones? Is there something special about these two? Separated. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:108: // This is not used on required by Blimp, so these are no-ops. On 2016/04/26 01:38:18, Wez wrote: > typo: on -> or > > I think it's reasonable to just say "We don't implement SetMetricsClientId since > we don't use Breakpad." - the MetricsServiceClient interface docs should explain > what the API would normally do. > > It's not clear what OnRecordingDisabled has to do with providing an Id to > anything - it doesn't seem to even have a parameter? Based on Marcin's work I think we actually do use breakpad, however unlike other Chromium products we don't care about mapping between UMA and breakpad client ids. Separated out note on OnRecordingDisabled. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:120: // Indicates product family, not reported platform. On 2016/04/26 01:38:17, Wez wrote: > Not sure what this comment means by "reported platform"? > > Is this comment really specific to Blimp, or should the GetProduct comment in > MetricsServiceClient interface be sufficient to understand? Added exampled to help clarify. This Product is not the same Product you see in the metrics service UX. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:129: // Blimp doesn't use brand codes. On 2016/04/26 01:38:18, Wez wrote: > Not clear from this comment what a brand-code even is? Sadly the > MetricsServiceClient comment for it is not very helpful either :-/ Acknowledged. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:135: return metrics::SystemProfileProto::CHANNEL_UNKNOWN; On 2016/04/26 01:38:18, Wez wrote: > nit: Hmmm, tricky; channel is used elsewhere to decide which APIs to support, so > we should be careful that returning UNKNOWN here doesn't lead to some weird > mismatch. Do we have the concept of a channel within the engine? I am happy to switch to whatever is current practice. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:139: // TODO(jessicag): Add in a meaningful version string. On 2016/04/26 01:38:17, Wez wrote: > Excitingly I'm working on a CL that will add such a thing and was just wondering > if we even needed it. Yay! Acknowledged. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:147: done_callback.Run(); On 2016/04/26 01:38:18, Wez wrote: > Add one-line comment to clarify why it's OK to call the callback immediately, > here and below? Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:163: // Member of g/e/common/blimp_browser_context. On 2016/04/26 01:38:18, Wez wrote: > This comment is already present in the class declaration of the member. Cleaned up. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:34: // Based on AwMetricsServiceClient. Inlining since various sub points require own responses. On 2016/04/26 01:38:19, Wez wrote: > This comment is somewhat cryptic, IMO! > > - It's not clear what "depends on blimp" means; are we saying that this is a > MetricsServiceClient tailored to the Blimp Engine? > - Not sure what you mean by "This singleton...for an app". Rewritten PTAL. I was operating under the assumption that key points in the Aw comments reflected common comment patterns that would be generally relevant. > - Not clear why metrics being always-on is remarkable; might be clearer to > instead override the GetDefaultOptIn() method to always return ON, with a > suitable comment? Presumably we also need a TODO w/ bug filed to change the > behaviour before this is released? Are we already filing bugs for v0.6+? I know this is included in some of our documents of required work there, and was unsure if we had a preferred way of tracking such items yet. Also right now this is doing more than just setting the opt in but instead ignoring that setting entirely and always reporting. > - Comment talks about threads, but doesn't itself have threads, nor any > immediate mention of threads; in general it's best to avoid referring to details > of the surrounding system in comments describing "leaf" classes of this sort, > and instead document the assumptions their interface makes. In this case the > MetricsServiceClient _owns_ the MetricsService, and it seems that the > MetricsService expects only to be called from a single thread, so the constraint > is just that Initialize and Finalize must be called on the same thread - the > MetricsService takes care of calling to the MetricsServiceClient APIs on that > same thread itself? Sounds right to me. Alexei can you confirm? > - That this is based on AwMetricsServiceClient is only worth noting if there is > some particular pattern that that class used that you wanted to copy - if it's > just an example of another implementation then remove the comment; reader can > use code-search to find other examples if need be! Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:38: static BlimpMetricsServiceClient* GetInstance(); On 2016/04/26 01:38:18, Wez wrote: > Document this API - e.g. does it return a process-global singleton? Does/should > the caller take a reference to it? Does it ever get deleted? > > Also, since this is a singleton and the only APIs it really has are Initialize() > and Finalize(), did you consider making those stand alone methods in the blimp > namespace and moving the class declaration into the .cc file? You'd then be > able to safely get rid of the private ctor/dtor and friending, and the > is_initialized bool (since you'd only create the instance in Initialize, so it'd > always be initialized). I see what you mean about moving the class definition into the .cc and have done that. However I am having a harder time following how you are propose to update the interface. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:44: // metrics::MetricsServiceClient implementation: On 2016/04/26 01:38:18, Wez wrote: > nit: End comment with . not : Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:66: bool is_initialized_; On 2016/04/26 01:38:19, Wez wrote: > Suggest initializing this and other POD members in-line. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:70: // g_browser_process->system_request_context() On 2016/04/26 01:38:18, Wez wrote: > I'd suggest replacing this entire comment with just "Used by > NetMetricsLogUploader to create log-upload requests." Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:71: // In engine use subclass BlimpURLRequestContextGetter. On 2016/04/26 01:38:18, Wez wrote: > If this detail is important for the caller to know then comment on the > Initialize() call, where they provide the getter. If not then remove the > comment, since it's not relevant to the implementation. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:72: // Member of g/e/common/blimp_browser_context. On 2016/04/26 01:38:18, Wez wrote: > nit:What is g/e/? Comment cleaned up. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:73: net::URLRequestContextGetter* request_context_; On 2016/04/26 01:38:18, Wez wrote: > This class is ref-counted - take a lovely safe reference to it, please :D > > nit: This is the getter, not a context. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.cc:25: // Shows notifications which correspond to PersistentPrefStore's reading errors. On 2016/04/26 01:38:19, Wez wrote: > Looks like the body of this function is missing? Is it supposed to be declared > in-line? PTAL at the new comment. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.cc:105: void BlimpBrowserContext::InitPrefService() { On 2016/04/26 01:38:19, Wez wrote: > If I understand this code correctly (big "if") this is doing three things: > > 1. Creating a PrefRegistry & registering the metrics service preferences with > it. > > 2. Creating an in-memory preferences store, to hold metrics logs > > 3. Attaching it to that registry. > > It would therefore be helpful to break this code up into logical blocks with > blob-level comments, i.e. something like: > > // Create a PrefRegistry and populate it with MetricsService prefs. > ... > > // Create an in-memory PrefStore. > ... > > // Create a PrefService binding the metrics PrefRegistry to our in-memory > store. > ... > > nit: It's also really unfortunate that those interfaces use bare pointers when > many of the objects being passed-around are ref-counted, but I don't think > there's much we can do about that in this CL, except perhaps mentioning in that > third comment that the PrefService ends up owning the PrefRegistry and store. > ;-( Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:36: // These map to BrowserMainParts::Pre/PostMainMessageLoopRun. On 2016/04/26 01:38:19, Wez wrote: > nit: Do you mean "Called by BrowserMainParts::Pre/PostMainMessageLoopRun, > respectively."? Yes. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:74: void InitPrefService(); On 2016/04/26 01:38:19, Wez wrote: > nit: Blank line above this so it doesn't look like the IO comment applies to it > too. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... blimp/engine/common/blimp_browser_context.h:77: std::unique_ptr<PrefService> pref_service_; On 2016/04/26 01:38:19, Wez wrote: > nit: Blank line after this, to separate it from the other members, otherwise it > looks like the comment applies to them too. Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.cc (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:263: browser_context_->Initialize(); On 2016/04/26 01:38:19, Wez wrote: > nit: Add a one-line comment to explain what this is for? > > Seems strange that this only became required when we started doing metrics... Done. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.cc:275: browser_context_->Finalize(); On 2016/04/26 01:38:19, Wez wrote: > Is it important that this happen before we enter the BlimpEngineSession dtor, or > could we move it there and avoid the explicit Finalize() API? Done. Also moved call to browser_context_->Initialize() into ctor. https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... File blimp/engine/session/blimp_engine_session.h (right): https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... blimp/engine/session/blimp_engine_session.h:87: // Finalizes the metrics reporting client. On 2016/04/26 01:38:19, Wez wrote: > nit: Blank line before this comment, plz, to match the style of other comments > in this code. Done.
I'm ok with the refactoring of osname override being a separate CL / its own bug. (But I also don't think it's a lot of work either - but keeping it separate sgtm). On Apr 26, 2016 5:24 PM, <jessicag@chromium.org> wrote: > Wez: > None of the other implementations I found had tests. I'll be following up > on > getters and setters, let me know if there is anything else you'd like me to > cover. > > Also feel free to grab me offline to discuss further refactoring. I > suspect I am > missing something relatively small here. > > > Alexei: > I think that will take logic threading and a bit of thought on how it will > interact with the existing interfaces for OS. Something will need to > happen in > the space for the current code the upload the correct OS metadata. Should > we > break off the refactoring into its own crbug? Am I overestimating the > amount of > thought this will take? > > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > File blimp/engine/app/blimp_metrics_service_client.cc (right): > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:24: > base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_; > On 2016/04/26 01:38:17, Wez wrote: > > This needs to be in the anonymous namespace, surely? > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:28: // Standard > interval between log uploads, in minutes > On 2016/04/26 01:38:18, Wez wrote: > > Can you reword this to explain what the "standard interval" is > actually for; > > otherwise the comment is almost the variable name verbatim. > > > > nit: End comment with . > > nit: Constant is already named kFooMinutes, so no need to mention > units. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:29: const int > kStandardUploadIntervalMinutes = 30; // Thirty minutes. > On 2016/04/26 01:38:18, Wez wrote: > > nit: Trailing comment adds nothing ;) > > Removed. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:31: bool > IsReportingEnabled() { > On 2016/04/26 01:38:18, Wez wrote: > > nit: Suggest providing a one-line comment explaining that the metrics > state > > manager expects an is-enabled callback to be provided. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:32: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); > On 2016/04/26 01:38:17, Wez wrote: > > Why this thread-check here but nowhere else? > > Usually this call makes use of features the require the Browser UI > thread, but it isn't a requirement here. I kept it as a sanity check, > but in hindsight here is not the place for such a check. > > Removed. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:37: // allows Windows > Chrome to back up ClientInfo. They're no-ops for Blimp. > On 2016/04/26 01:38:18, Wez wrote: > > IsReportingEnabled also seems to be a callback for that purpose? > > See comment above. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:62: > base::Bind(&StoreClientInfo), base::Bind(&LoadClientInfo)); > On 2016/04/26 01:38:18, Wez wrote: > > nit: Is this the format "git cl format" gives? Seems strange to wrap > the > > preceding two lines but not this one. > > Ran git cl format. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:64: > metrics_service_.reset(new ::metrics::MetricsService( > On 2016/04/26 01:38:18, Wez wrote: > > nit: Do we need the leading ::? > > No. Removed. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:70: > content::BrowserThread::GetBlockingPool()))); > On 2016/04/26 01:38:18, Wez wrote: > > We should be very careful about using content::BrowserThread stuff; > means that > > we presumably have some constraints about when this instance must be > created, > > and torn, down, so that it's dependencies are met, and those should be > > documented in the comment on the class. > > Honestly I am not quite sure here. Perhaps removal is the best option. > We can always register it in a later cr. Thoughts? > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:74: new > metrics::GPUMetricsProvider)); > On 2016/04/26 01:38:18, Wez wrote: > > nit: Why are these two registrations grouped together, separate from > the > > profiler and call-stack ones? Is there something special about these > two? > > Separated. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:108: // This is not > used on required by Blimp, so these are no-ops. > On 2016/04/26 01:38:18, Wez wrote: > > typo: on -> or > > > > I think it's reasonable to just say "We don't implement > SetMetricsClientId since > > we don't use Breakpad." - the MetricsServiceClient interface docs > should explain > > what the API would normally do. > > > > It's not clear what OnRecordingDisabled has to do with providing an Id > to > > anything - it doesn't seem to even have a parameter? > > Based on Marcin's work I think we actually do use breakpad, however > unlike other Chromium products we don't care about mapping between UMA > and breakpad client ids. > > Separated out note on OnRecordingDisabled. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:120: // Indicates > product family, not reported platform. > On 2016/04/26 01:38:17, Wez wrote: > > Not sure what this comment means by "reported platform"? > > > > Is this comment really specific to Blimp, or should the GetProduct > comment in > > MetricsServiceClient interface be sufficient to understand? > > Added exampled to help clarify. This Product is not the same Product > you see in the metrics service UX. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:129: // Blimp doesn't > use brand codes. > On 2016/04/26 01:38:18, Wez wrote: > > Not clear from this comment what a brand-code even is? Sadly the > > MetricsServiceClient comment for it is not very helpful either :-/ > > Acknowledged. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:135: return > metrics::SystemProfileProto::CHANNEL_UNKNOWN; > On 2016/04/26 01:38:18, Wez wrote: > > nit: Hmmm, tricky; channel is used elsewhere to decide which APIs to > support, so > > we should be careful that returning UNKNOWN here doesn't lead to some > weird > > mismatch. > > Do we have the concept of a channel within the engine? I am happy to > switch to whatever is current practice. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:139: // TODO(jessicag): > Add in a meaningful version string. > On 2016/04/26 01:38:17, Wez wrote: > > Excitingly I'm working on a CL that will add such a thing and was just > wondering > > if we even needed it. Yay! > > Acknowledged. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:147: > done_callback.Run(); > On 2016/04/26 01:38:18, Wez wrote: > > Add one-line comment to clarify why it's OK to call the callback > immediately, > > here and below? > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.cc:163: // Member of > g/e/common/blimp_browser_context. > On 2016/04/26 01:38:18, Wez wrote: > > This comment is already present in the class declaration of the > member. > > Cleaned up. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > File blimp/engine/app/blimp_metrics_service_client.h (right): > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:34: // Based on > AwMetricsServiceClient. > Inlining since various sub points require own responses. > > On 2016/04/26 01:38:19, Wez wrote: > > This comment is somewhat cryptic, IMO! > > > > - It's not clear what "depends on blimp" means; are we saying that > this is a > > MetricsServiceClient tailored to the Blimp Engine? > > - Not sure what you mean by "This singleton...for an app". > Rewritten PTAL. I was operating under the assumption that key points in > the Aw comments reflected common comment patterns that would be > generally relevant. > > - Not clear why metrics being always-on is remarkable; might be > clearer to > > instead override the GetDefaultOptIn() method to always return ON, > with a > > suitable comment? Presumably we also need a TODO w/ bug filed to > change the > > behaviour before this is released? > Are we already filing bugs for v0.6+? I know this is included in some > of our documents of required work there, and was unsure if we had a > preferred way of tracking such items yet. > > Also right now this is doing more than just setting the opt in but > instead ignoring that setting entirely and always reporting. > > - Comment talks about threads, but doesn't itself have threads, nor > any > > immediate mention of threads; in general it's best to avoid referring > to details > > of the surrounding system in comments describing "leaf" classes of > this sort, > > and instead document the assumptions their interface makes. In this > case the > > MetricsServiceClient _owns_ the MetricsService, and it seems that the > > MetricsService expects only to be called from a single thread, so the > constraint > > is just that Initialize and Finalize must be called on the same thread > - the > > MetricsService takes care of calling to the MetricsServiceClient APIs > on that > > same thread itself? > Sounds right to me. > > Alexei can you confirm? > > - That this is based on AwMetricsServiceClient is only worth noting if > there is > > some particular pattern that that class used that you wanted to copy - > if it's > > just an example of another implementation then remove the comment; > reader can > > use code-search to find other examples if need be! > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:38: static > BlimpMetricsServiceClient* GetInstance(); > On 2016/04/26 01:38:18, Wez wrote: > > Document this API - e.g. does it return a process-global singleton? > Does/should > > the caller take a reference to it? Does it ever get deleted? > > > > Also, since this is a singleton and the only APIs it really has are > Initialize() > > and Finalize(), did you consider making those stand alone methods in > the blimp > > namespace and moving the class declaration into the .cc file? You'd > then be > > able to safely get rid of the private ctor/dtor and friending, and the > > is_initialized bool (since you'd only create the instance in > Initialize, so it'd > > always be initialized). > > I see what you mean about moving the class definition into the .cc and > have done that. However I am having a harder time following how you are > propose to update the interface. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:44: // > metrics::MetricsServiceClient implementation: > On 2016/04/26 01:38:18, Wez wrote: > > nit: End comment with . not : > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:66: bool > is_initialized_; > On 2016/04/26 01:38:19, Wez wrote: > > Suggest initializing this and other POD members in-line. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:70: // > g_browser_process->system_request_context() > On 2016/04/26 01:38:18, Wez wrote: > > I'd suggest replacing this entire comment with just "Used by > > NetMetricsLogUploader to create log-upload requests." > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:71: // In engine use > subclass BlimpURLRequestContextGetter. > On 2016/04/26 01:38:18, Wez wrote: > > If this detail is important for the caller to know then comment on the > > Initialize() call, where they provide the getter. If not then remove > the > > comment, since it's not relevant to the implementation. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:72: // Member of > g/e/common/blimp_browser_context. > On 2016/04/26 01:38:18, Wez wrote: > > nit:What is g/e/? > > Comment cleaned up. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/app/blimp_... > blimp/engine/app/blimp_metrics_service_client.h:73: > net::URLRequestContextGetter* request_context_; > On 2016/04/26 01:38:18, Wez wrote: > > This class is ref-counted - take a lovely safe reference to it, please > :D > > > > nit: This is the getter, not a context. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > File blimp/engine/common/blimp_browser_context.cc (right): > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > blimp/engine/common/blimp_browser_context.cc:25: // Shows notifications > which correspond to PersistentPrefStore's reading errors. > On 2016/04/26 01:38:19, Wez wrote: > > Looks like the body of this function is missing? Is it supposed to be > declared > > in-line? > > PTAL at the new comment. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > blimp/engine/common/blimp_browser_context.cc:105: void > BlimpBrowserContext::InitPrefService() { > On 2016/04/26 01:38:19, Wez wrote: > > If I understand this code correctly (big "if") this is doing three > things: > > > > 1. Creating a PrefRegistry & registering the metrics service > preferences with > > it. > > > > 2. Creating an in-memory preferences store, to hold metrics logs > > > > 3. Attaching it to that registry. > > > > It would therefore be helpful to break this code up into logical > blocks with > > blob-level comments, i.e. something like: > > > > // Create a PrefRegistry and populate it with MetricsService prefs. > > ... > > > > // Create an in-memory PrefStore. > > ... > > > > // Create a PrefService binding the metrics PrefRegistry to our > in-memory > > store. > > ... > > > > nit: It's also really unfortunate that those interfaces use bare > pointers when > > many of the objects being passed-around are ref-counted, but I don't > think > > there's much we can do about that in this CL, except perhaps > mentioning in that > > third comment that the PrefService ends up owning the PrefRegistry and > store. > > ;-( > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > File blimp/engine/common/blimp_browser_context.h (right): > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > blimp/engine/common/blimp_browser_context.h:36: // These map to > BrowserMainParts::Pre/PostMainMessageLoopRun. > On 2016/04/26 01:38:19, Wez wrote: > > nit: Do you mean "Called by > BrowserMainParts::Pre/PostMainMessageLoopRun, > > respectively."? > > Yes. Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > blimp/engine/common/blimp_browser_context.h:74: void InitPrefService(); > On 2016/04/26 01:38:19, Wez wrote: > > nit: Blank line above this so it doesn't look like the IO comment > applies to it > > too. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/common/bli... > blimp/engine/common/blimp_browser_context.h:77: > std::unique_ptr<PrefService> pref_service_; > On 2016/04/26 01:38:19, Wez wrote: > > nit: Blank line after this, to separate it from the other members, > otherwise it > > looks like the comment applies to them too. > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... > File blimp/engine/session/blimp_engine_session.cc (right): > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.cc:263: > browser_context_->Initialize(); > On 2016/04/26 01:38:19, Wez wrote: > > nit: Add a one-line comment to explain what this is for? > > > > Seems strange that this only became required when we started doing > metrics... > > Done. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.cc:275: > browser_context_->Finalize(); > On 2016/04/26 01:38:19, Wez wrote: > > Is it important that this happen before we enter the > BlimpEngineSession dtor, or > > could we move it there and avoid the explicit Finalize() API? > > Done. > > Also moved call to browser_context_->Initialize() into ctor. > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... > File blimp/engine/session/blimp_engine_session.h (right): > > > https://codereview.chromium.org/1885673003/diff/40001/blimp/engine/session/bl... > blimp/engine/session/blimp_engine_session.h:87: // Finalizes the metrics > reporting client. > On 2016/04/26 01:38:19, Wez wrote: > > nit: Blank line before this comment, plz, to match the style of other > comments > > in this code. > > Done. > > https://codereview.chromium.org/1885673003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:32: void InitializeBlimpMetricsServiceClient( Suggest InitializeBlimpMetrics - sufficiently unambiguous but this function also initializes the whole metrics stack. :) We can add an InitializeBlimpMetricsForTest() function that accepts additional test-only parameters if necessary, if mocking the PrefService and/or URLRequestContextGetter are insufficient.
lgtm https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:138: if (IsReportingEnabled()) { Nit: No {}'s
Added unittest. Unfortunately much of the interesting impacts of client initialization happen in private portions of the metrics service. Please let me know your thoughts on doing more. https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.cc:138: if (IsReportingEnabled()) { On 2016/04/28 16:58:26, Alexei Svitkine wrote: > Nit: No {}'s Done. https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/60001/blimp/engine/app/blimp_... blimp/engine/app/blimp_metrics_service_client.h:32: void InitializeBlimpMetricsServiceClient( On 2016/04/27 22:30:27, Wez wrote: > Suggest InitializeBlimpMetrics - sufficiently unambiguous but this function also > initializes the whole metrics stack. :) > > We can add an InitializeBlimpMetricsForTest() function that accepts additional > test-only parameters if necessary, if mocking the PrefService and/or > URLRequestContextGetter are insufficient. Done.
https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:31: class BlimpMetricsServiceClient : public metrics::MetricsServiceClient { This should be tucked away in an anonymous namespace now, since it's an internal implementation detail of the InitializeBlimpMetrics API. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:37: // valid for the client lifetime. nit: Since it's ref-counted is there any way it might _not_ remain valid for the client lifetime? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:74: // How often after initial logging metrics results should be uploaded to the nit: Not sure what this means; are you just saying that this is how often results should be uploaded, or is there some significance to "after initial logging"? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:78: base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_; Does this actually need to be a lazy instance? We know that Initialize and Finalize will be called from the same thread, by the caller, and nothing else tries to get at the instance, so there's no need for threaded-lazy-initialization magic either - can we just use a bare pointer initialized to nullptr (i.e. initialized via text-segment, which is OK) and then DCHECK(!g_instance_) in Initialize()? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:80: // Returns if the MetricsService should be recording metrics information for nit: if -> whether https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:82: // This callback required by MetricsStateManager::Create. nit: is required https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:147: // This is not required by Blimp, so these are no-ops. nit: I think it's sufficient to say this is not required by blimp; no need for "so this is a no-op." at the end. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:151: // Recording can not be disabled in Blimp, so this function is a no-op. nit: See above re :...so this is a no-op" https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:155: // Blimp does not have incognito mode. nit: Move this comment outside the function much as you have for the ones above. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:180: // TODO(jessicag): Add in a meaningful version string. I have a CL in-progress to add version stuff, so if you want you can switch this to a TODO(wez): and mention that bug #. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:222: BlimpMetricsServiceClient* GetBlimpMetricsServiceClientInstance() { This should be in an anonymous namespace; basically only the Initialize and Finalize functions should be in the blimp::engine namespace in this file. Once it's anonymous it could reasonably be called GetInstance(), or even omitted entirely since the implementation is almost shorter than the name! https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:234: GetBlimpMetricsServiceClientInstance()->Finalize(); Can you clear the lazy-instance here, in which case it need not be Leaky? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.h:10: #include <string> no need for blank between stdint.h and string includes? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.h:24: void InitializeBlimpMetrics( Please a add a brief comment explaining what the supplied parameters are for e.g. "|pref_service| is used to log metrics to the chosen PrefStore." or whatever. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.h:28: void FinalizeBlimpMetrics(); nit: Consider commenting on what guarantees there are on the state of the metrics service at the point at which this call returns - does the metrics system shut down asynchronously or synchronously in response to the call, for example. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right): https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:27: // Shows notifications which correspond to PersistentPrefStore's reading errors. This function just seems to be a no-op dummy used to Bind() into a read-error callback, so suggest describing what it's actually used for here (to make clear why we need a strange no-op function :) rather than what a real implementation should do. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:34: void Finalize(); Why not override Setup() and Teardown() from testing::Test here? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:36: void SetUpPrefRegistry(); micro-nit: I believe we normally capitalize "setup" as Setup, not SetUp. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:50: ASSERT_NE(nullptr, pref_service_.get()); Can you use ASSERT_TRUE(pref_service_) here? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:52: InitializeBlimpMetrics(std::move(pref_service_), request_context_getter_); Since you're passing the pref-service here, and creating it in the SetUpPrefService() sub-routine, why not have the sub-routine return a unique_ptr<> and avoid the need for the test-base-class member? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:81: // TODO(jessicag): Check request_context_getter_ set. Is this a reminder to just have ASSERT_TRUE(request_context_getter_), in effect? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:87: // TODO(jessicag): Confirm MetricsService::Start() is called. If the MetricsService is too much of a black-box for us to be able to do this fine-grained testing, can we at least do something like: 1. Verify that the pref-store is empty. 2. Log a metric. 3. Verify that the pref-store is no longer empty. or maybe even verify that the pref-store has correct-looking content? Remember that if we need additional hooks set by our MetricsServiceClient impl then we can add an InitializeBlimpMetricsForTest helper. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:28: void HandleReadError(PersistentPrefStore::PrefReadError error) {} Feels like this comment could be clearer e.g. "Callback used to handle PrefStore read errors; since we use an in-memory store we don't expect any errors." and then have the implementation body be NOTREACHED(). https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:85: InitializeBlimpMetrics(GetPrefService(), GetSystemRequestContextGetter()); Is there a reason that these Initialize and Finalize can't be folded into the constructor & destructor? e.g. do they need to be called at very specific point during startup & shutdown? If so then consider naming them more specifically e.g. OnBrowserMainPartsInitialize() or DoBrowserMainPartsInit() or even just InitializeMetrics(). https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:103: std::unique_ptr<PrefService> BlimpBrowserContext::GetPrefService() { nit: Put blank lines between each code-block and the comment starting the next code block, for readability. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:109: // Create an in memory preferences store to hold metrics logs. i.e. move this comment to precede declaration of |pref_service_factory| and put a blank line between it and the preceding RegisterPrefs() call. https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:113: // The PrefService ends up owning the PrefRegistry and the InMemoryPrefStore. What do we mean by "ends up owning"? Since PrefRegistry is ref-counted it looks like the PrefService holds a _reference_ to it. Similarly the InMemoryPrefStore is also ref-counted, so the PrefService presumably only takes a reference to it, rather than owning it (which makes sense since the InMemoryPrefStore is passed to the PrefServiceFactory and so presumably might be associated with several PrefService instances). https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.h:36: // These map to BrowserMainParts::Pre/PostMainMessageLoopRun respectively. Can you be more specific? Do you mean "These call through to..." or "These are called by..."? https://codereview.chromium.org/1885673003/diff/100001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.h:76: // temporarily. This comment is pretty confusing; why does metrics initialization need to store temporary logs?
jessicag@chromium.org changed reviewers: + kmarshall@chromium.org
jessicag@chromium.org changed required reviewers: + kmarshall@chromium.org - wez@chromium.org
Adding in kmarshall to continue for wez.
https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:82: // This callback required by MetricsStateManager::Create. is required https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:33: void Initialize(); ANy reason why these shouldn't be SetUp()/TearDown() overrides? https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:28: void HandleReadError(PersistentPrefStore::PrefReadError error) {} IgnoreReadError()? https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:84: void BlimpBrowserContext::Initialize() { Any reason why we can't do this in the constructor and destructor? https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:109: // Create an in memory preferences store to hold metrics logs. Add newlines before comments https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:112: // Create a PrefService binding the PrefRegistry to the InMemoryPrefStore. Newline beforehand https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.h:36: // These map to BrowserMainParts::Pre/PostMainMessageLoopRun respectively. This comment is kind of unclear. What does "map" mean in this context?
https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:82: // This callback required by MetricsStateManager::Create. On 2016/05/02 21:50:52, Kevin M wrote: > is required Done. https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:33: void Initialize(); On 2016/05/02 21:50:52, Kevin M wrote: > ANy reason why these shouldn't be SetUp()/TearDown() overrides? Not particularly. An empty TEST_F isn't a pattern I am particularly used to seeing, but otherwise seems like a totally reasonable change. Done. https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... File blimp/engine/common/blimp_browser_context.cc (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:28: void HandleReadError(PersistentPrefStore::PrefReadError error) {} On 2016/05/02 21:50:52, Kevin M wrote: > IgnoreReadError()? Done. https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:84: void BlimpBrowserContext::Initialize() { On 2016/05/02 21:50:52, Kevin M wrote: > Any reason why we can't do this in the constructor and destructor? None that's apparent to me. Done. https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:109: // Create an in memory preferences store to hold metrics logs. On 2016/05/02 21:50:52, Kevin M wrote: > Add newlines before comments Done. https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.cc:112: // Create a PrefService binding the PrefRegistry to the InMemoryPrefStore. On 2016/05/02 21:50:52, Kevin M wrote: > Newline beforehand Done. https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... File blimp/engine/common/blimp_browser_context.h (right): https://codereview.chromium.org/1885673003/diff/180001/blimp/engine/common/bl... blimp/engine/common/blimp_browser_context.h:36: // These map to BrowserMainParts::Pre/PostMainMessageLoopRun respectively. On 2016/05/02 21:50:52, Kevin M wrote: > This comment is kind of unclear. What does "map" mean in this context? Rewritten.
Getting close! https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:28: // BlimpMetricsServiceClient provides a singleton implementation of Nit: this doesn't vend a singleton, but it's used *as* a singleton? The comment should describe the purpose of this class before going into detail about how it's stored. (e.g. Defines the Blimp-specific policies for metric logging) https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:28: // BlimpMetricsServiceClient provides a singleton implementation of Move this class into the anonymous namespace. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:78: base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_; = LAZY_INSTANCE_INITIALIZER https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:114: Nit: remove the newlines between the Register calls for better semantic blocking. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:181: return std::string(); Can we use VERSION_INFO from components/version_info/version_info_values.h ? https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.h:24: void InitializeBlimpMetrics( Add comments for these functions.
PTAL https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:28: // BlimpMetricsServiceClient provides a singleton implementation of On 2016/05/03 18:13:19, Kevin M wrote: > Move this class into the anonymous namespace. Done. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:28: // BlimpMetricsServiceClient provides a singleton implementation of On 2016/05/03 18:13:19, Kevin M wrote: > Nit: this doesn't vend a singleton, but it's used *as* a singleton? > > The comment should describe the purpose of this class before going into detail > about how it's stored. (e.g. Defines the Blimp-specific policies for metric > logging) Updated. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:78: base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_; On 2016/05/03 18:13:19, Kevin M wrote: > = LAZY_INSTANCE_INITIALIZER Done. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:114: On 2016/05/03 18:13:19, Kevin M wrote: > Nit: remove the newlines between the Register calls for better semantic > blocking. Done. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:181: return std::string(); On 2016/05/03 18:13:19, Kevin M wrote: > Can we use VERSION_INFO from components/version_info/version_info_values.h ? Wez didn't seem to think version info was available to blimp yet. Glad to be able to add it in and clear up a TODO. https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.h (right): https://codereview.chromium.org/1885673003/diff/200001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.h:24: void InitializeBlimpMetrics( On 2016/05/03 18:13:20, Kevin M wrote: > Add comments for these functions. Done.
lgtm Just some nits :) https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:28: remove this newline https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:30: // BlimpMetricsServiceClient provides an implementation of MetricsServiceClient add a newline above the comment https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right): https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:51: ASSERT_NE(nullptr, pref_service_.get()); move this into the end of SetUpPrefService
Thanks everyone! https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client.cc (right): https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:28: On 2016/05/03 20:15:08, Kevin M wrote: > remove this newline Done. https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client.cc:30: // BlimpMetricsServiceClient provides an implementation of MetricsServiceClient On 2016/05/03 20:15:08, Kevin M wrote: > add a newline above the comment Done. https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... File blimp/engine/app/blimp_metrics_service_client_unittest.cc (right): https://codereview.chromium.org/1885673003/diff/220001/blimp/engine/app/blimp... blimp/engine/app/blimp_metrics_service_client_unittest.cc:51: ASSERT_NE(nullptr, pref_service_.get()); On 2016/05/03 20:15:08, Kevin M wrote: > move this into the end of SetUpPrefService Done.
The CQ bit was checked by jessicag@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, asvitkine@chromium.org, kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/1885673003/#ps240001 (title: "Move newline and ASSERT as requested.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1885673003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885673003/240001
Message was sent while issue was closed.
Description was changed from ========== Create and integrate a metrics service client into Blimp engine. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ========== to ========== Create and integrate a metrics service client into Blimp engine. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Create and integrate a metrics service client into Blimp engine. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 ========== to ========== Create and integrate a metrics service client into Blimp engine. This will allow Blimp to collect and upload metrics to aid in development. BUG=592757 Committed: https://crrev.com/0be9daea216b9144830da20bcb3e2a56591207e6 Cr-Commit-Position: refs/heads/master@{#391360} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/0be9daea216b9144830da20bcb3e2a56591207e6 Cr-Commit-Position: refs/heads/master@{#391360}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/1948733003/ by jessicag@chromium.org. The reason for reverting is: Detected engine crash. Looks like a PerfService lifetime issue.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
