Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517363002:
Decouple Metrics Initialization from IO Thread Initialization (Closed)
DescriptionDecouple Metrics Initialization from IO Thread Initialization
In preparation of moving Task Scheduler initialization to the content
CreateThreads phase, metrics has to be initialized during
PreMainMessageLoopRun.
The only reference to metrics during PreCreateThreads was in the
IO Thread state initialization to set up a IO thread to UI thread
forwarder that reported network connection state (e.g. cellular or not).
This change moves that initialization to be called on demand as the
callback is not needed until the first network URL request is satisfied.
This happens after CreateThreads and generally after PreMainMessageLoopRun.
BUG=662052
Committed: https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512
Cr-Commit-Position: refs/heads/master@{#435657}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove #if's, add "Can be a null callback" comment. #Patch Set 3 : Add One Level of Indirection #
Total comments: 8
Patch Set 4 : Shift UI Call Marshalling Responsibility to Caller #Patch Set 5 : Fix Unit Test #
Total comments: 6
Patch Set 6 : CR Feedback #
Total comments: 2
Patch Set 7 : CR Feedback #
Dependent Patchsets: Messages
Total messages: 57 (36 generated)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: Please review this CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % comment https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1089: #if DCHECK_IS_ON() Nit: I'd remove these ifdefs. I think the readability gains from not having them (in 3 places) outweigh the extra 4 byte overhead in release builds.
robliao@chromium.org changed reviewers: + brettw@chromium.org, mmenke@chromium.org
mmenke@chromium.org: Please review changes in c/b/io_* brettw@chromium.org: Please review changes in c/b/[everything else] Thanks! https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1089: #if DCHECK_IS_ON() On 2016/11/22 16:29:46, Alexei Svitkine (slow) wrote: > Nit: I'd remove these ifdefs. > > I think the readability gains from not having them (in 3 places) outweigh the > extra 4 byte overhead in release builds. Let's keep them for now. 4 bytes saved here and there adds up. If the owner has strong feelings about this, than I'll remove them.
https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1089: #if DCHECK_IS_ON() On 2016/11/22 18:02:37, robliao wrote: > On 2016/11/22 16:29:46, Alexei Svitkine (slow) wrote: > > Nit: I'd remove these ifdefs. > > > > I think the readability gains from not having them (in 3 places) outweigh the > > extra 4 byte overhead in release builds. > > Let's keep them for now. 4 bytes saved here and there adds up. If the owner has > strong feelings about this, than I'll remove them. Why is this even needed? Can't we just do DCHECK(!metrics_data_use_forwarder_.foo()), and get rid of the bool, where foo is one of is_null/is_empty/empty (Not sure which it is)?
https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1089: #if DCHECK_IS_ON() On 2016/11/22 18:09:06, mmenke wrote: > On 2016/11/22 18:02:37, robliao wrote: > > On 2016/11/22 16:29:46, Alexei Svitkine (slow) wrote: > > > Nit: I'd remove these ifdefs. > > > > > > I think the readability gains from not having them (in 3 places) outweigh > the > > > extra 4 byte overhead in release builds. > > > > Let's keep them for now. 4 bytes saved here and there adds up. If the owner > has > > strong feelings about this, than I'll remove them. > > Why is this even needed? Can't we just do > DCHECK(!metrics_data_use_forwarder_.foo()), and get rid of the bool, where foo > is one of is_null/is_empty/empty (Not sure which it is)? metrics_data_use_forwarder_ will be null/empty on non-Android builds. We can't distinguish between an empty callback, a real callback, and a callback that was never set.
What guarantee do we have that nothing does anything with IOThread between when it's created and the InitMetricsDataUseForwarder call? https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1089: #if DCHECK_IS_ON() On 2016/11/22 18:10:27, robliao wrote: > On 2016/11/22 18:09:06, mmenke wrote: > > On 2016/11/22 18:02:37, robliao wrote: > > > On 2016/11/22 16:29:46, Alexei Svitkine (slow) wrote: > > > > Nit: I'd remove these ifdefs. > > > > > > > > I think the readability gains from not having them (in 3 places) outweigh > > the > > > > extra 4 byte overhead in release builds. > > > > > > Let's keep them for now. 4 bytes saved here and there adds up. If the owner > > has > > > strong feelings about this, than I'll remove them. > > > > Why is this even needed? Can't we just do > > DCHECK(!metrics_data_use_forwarder_.foo()), and get rid of the bool, where foo > > is one of is_null/is_empty/empty (Not sure which it is)? > > metrics_data_use_forwarder_ will be null/empty on non-Android builds. We can't > distinguish between an empty callback, a real callback, and a callback that was > never set. I don't think that's document in either io_thread.h or metrics_service.h. Document it in both places? And yea, I'd rather just get rid of the ifdefs, too, even if it means keeping the bool.
> What guarantee do we have that nothing does anything with IOThread between when it's created and the InitMetricsDataUseForwarder call? As with anything in the land of startup, there are no hard guarantees. Folks need to be mindful to the state of initialization before using services. This callback is only needed during ProfileImplIOData initialization. That doesn't happen until the profile is initialized during PreMainMessageLoopRun. As long as this callback is initialized before the profile, you're good to go. https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:1089: #if DCHECK_IS_ON() On 2016/11/22 18:20:58, mmenke wrote: > On 2016/11/22 18:10:27, robliao wrote: > > On 2016/11/22 18:09:06, mmenke wrote: > > > On 2016/11/22 18:02:37, robliao wrote: > > > > On 2016/11/22 16:29:46, Alexei Svitkine (slow) wrote: > > > > > Nit: I'd remove these ifdefs. > > > > > > > > > > I think the readability gains from not having them (in 3 places) > outweigh > > > the > > > > > extra 4 byte overhead in release builds. > > > > > > > > Let's keep them for now. 4 bytes saved here and there adds up. If the > owner > > > has > > > > strong feelings about this, than I'll remove them. > > > > > > Why is this even needed? Can't we just do > > > DCHECK(!metrics_data_use_forwarder_.foo()), and get rid of the bool, where > foo > > > is one of is_null/is_empty/empty (Not sure which it is)? > > > > metrics_data_use_forwarder_ will be null/empty on non-Android builds. We can't > > distinguish between an empty callback, a real callback, and a callback that > was > > never set. > > I don't think that's document in either io_thread.h or metrics_service.h. > Document it in both places? > > And yea, I'd rather just get rid of the ifdefs, too, even if it means keeping > the bool. Done and done.
This just seems like a really bad idea to me. There's only one consumer of GetMetricsDataUseForwarder(). Can we just have the ProfileImplIOData grab it on the UI thread, and move this logic out of the IOThread, avoiding a race for this one random thing hooked up to IOThread.
On 2016/11/23 18:47:22, mmenke wrote: > This just seems like a really bad idea to me. There's only one consumer of > GetMetricsDataUseForwarder(). Can we just have the ProfileImplIOData grab it on > the UI thread, and move this logic out of the IOThread, avoiding a race for this > one random thing hooked up to IOThread. Part of this change hinged on moving metrics wholesale to PreMainMessageLoopRun. There are some issues with that, so I'll round back on this once I've solved that story. Agreed that using the IO thread as a conduit is strange, but my guess is it was the most convenient way to do this before.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Decouple Metrics Initialization from IO Thread Initialization in PreCreateThreads In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to PreMainMessageLoopRun as the callback is not needed until profile initialization, which happens after this point in PreMainMessageLoopRun. BUG=662052 ========== to ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 ==========
Patchset #3 (id:40001) has been deleted
Alright. How about this approach? Instead of immediately requiring the callback which we won't need until later, we get it on demand on the UI thread by doing a postback to it? I went ahead and decoupled ProfileImplIOData from the IOThread while I was here but it turns out we still need the callback for the System Network Delegate.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:266: metrics_service->GetDataUseForwardingCallback(); If you're doing like this, then maybe you don't need the intermediate callback? i.e. you could just call the thing from this point directly instead of get the callback and call that? Because I definitely don't love this double callback stuff. https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:500: std::move(chrome_network_delegate), metrics_data_use_forwarder_), Why can't it get it from IO thread like before? It can still be the wrapper callback that's implemented by that file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:266: metrics_service->GetDataUseForwardingCallback(); On 2016/11/28 23:04:18, Alexei Svitkine (slow) wrote: > If you're doing like this, then maybe you don't need the intermediate callback? > > i.e. you could just call the thing from this point directly instead of get the > callback and call that? > > Because I definitely don't love this double callback stuff. Because we're doing a IO->UI hop, if we wanted to save off the callback we would need to do another UI->IO hop to safely save the value (along with dealing with any inflight calls that could be in progress). Given that these calls don't block the critical path of anything (they're async), this seemed preferable. https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:500: std::move(chrome_network_delegate), metrics_data_use_forwarder_), On 2016/11/28 23:04:18, Alexei Svitkine (slow) wrote: > Why can't it get it from IO thread like before? > > It can still be the wrapper callback that's implemented by that file. This changes the relationship from 1. MetricsService <- IOThread <- ProfileImpl[IOData] to 1. MetricsService <- IOThread 2. MetricsService <- ProfileImpl[IOData] The IO Thread only holds this callback as a convenience for ProfileImplIOData. Having this as a direct relationship simplifies IOThread and allows MetricsService to be the source of truth. IOThread can do the dance it needs because it calls back from the IO thread.
https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:266: metrics_service->GetDataUseForwardingCallback(); On 2016/11/29 18:17:43, robliao wrote: > On 2016/11/28 23:04:18, Alexei Svitkine (slow) wrote: > > If you're doing like this, then maybe you don't need the intermediate > callback? > > > > i.e. you could just call the thing from this point directly instead of get the > > callback and call that? > > > > Because I definitely don't love this double callback stuff. > > Because we're doing a IO->UI hop, if we wanted to save off the callback we would > need to do another UI->IO hop to safely save the value (along with dealing with > any inflight calls that could be in progress). Given that these calls don't > block the critical path of anything (they're async), this seemed preferable. Sorry, let me clarify - I'm not suggesting to save off the callback. I'm suggesting instead of calling "metrics_service->GetDataUseForwardingCallback();", to just call: metrics_service->OnDataUse(service_name, message_size, is_cellular); And then not to have a need for an API on metrics service to get the callback. (Where you would replace the metrics service GetDataUseForwardingCallback API with OnDataUse function.) Basically the current complexity of the callback stuff comes from the fact that it needs to deal with both being called from non-UI thread and the relevant object in metrics service not existing (on some platforms). If you're already solving the threading stuff by ensuring we're executing on the UI thread (via the code you wrote here), then we can clean up a lot of this complexity and not have any functions that return callbacks in the metrics service and related code. https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:500: std::move(chrome_network_delegate), metrics_data_use_forwarder_), On 2016/11/29 18:17:43, robliao wrote: > On 2016/11/28 23:04:18, Alexei Svitkine (slow) wrote: > > Why can't it get it from IO thread like before? > > > > It can still be the wrapper callback that's implemented by that file. > > This changes the relationship from > 1. MetricsService <- IOThread <- ProfileImpl[IOData] to > > 1. MetricsService <- IOThread > 2. MetricsService <- ProfileImpl[IOData] > > The IO Thread only holds this callback as a convenience for ProfileImplIOData. > Having this as a direct relationship simplifies IOThread and allows > MetricsService to be the source of truth. > > IOThread can do the dance it needs because it calls back from the IO thread. So my concern with this is now it's duplicating some logic between the two. Specifically, you now need to have "if metrics service is not null" check in both places. I think if you go with my other suggestion of not having MetricsService expose a callback anymore and for the callback to exist only to the new IOThread function you added - then it would make sense to use this same callback here that you get from IOThread.
Description was changed from ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 ========== to ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 ==========
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
asvitkine: Please review the new metrics service changes. Thanks! https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:266: metrics_service->GetDataUseForwardingCallback(); On 2016/11/29 18:34:33, Alexei Svitkine (slow) wrote: > On 2016/11/29 18:17:43, robliao wrote: > > On 2016/11/28 23:04:18, Alexei Svitkine (slow) wrote: > > > If you're doing like this, then maybe you don't need the intermediate > > callback? > > > > > > i.e. you could just call the thing from this point directly instead of get > the > > > callback and call that? > > > > > > Because I definitely don't love this double callback stuff. > > > > Because we're doing a IO->UI hop, if we wanted to save off the callback we > would > > need to do another UI->IO hop to safely save the value (along with dealing > with > > any inflight calls that could be in progress). Given that these calls don't > > block the critical path of anything (they're async), this seemed preferable. > > Sorry, let me clarify - I'm not suggesting to save off the callback. > > I'm suggesting instead of calling > "metrics_service->GetDataUseForwardingCallback();", to just call: > > metrics_service->OnDataUse(service_name, message_size, is_cellular); > > And then not to have a need for an API on metrics service to get the callback. > (Where you would replace the metrics service GetDataUseForwardingCallback API > with OnDataUse function.) > > Basically the current complexity of the callback stuff comes from the fact that > it needs to deal with both being called from non-UI thread and the relevant > object in metrics service not existing (on some platforms). > > If you're already solving the threading stuff by ensuring we're executing on the > UI thread (via the code you wrote here), then we can clean up a lot of this > complexity and not have any functions that return callbacks in the metrics > service and related code. Gotcha. That works for me. I didn't know if you folks were planning on using this callback elsewhere. Changed. https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2517363002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:500: std::move(chrome_network_delegate), metrics_data_use_forwarder_), On 2016/11/29 18:34:34, Alexei Svitkine (slow) wrote: > On 2016/11/29 18:17:43, robliao wrote: > > On 2016/11/28 23:04:18, Alexei Svitkine (slow) wrote: > > > Why can't it get it from IO thread like before? > > > > > > It can still be the wrapper callback that's implemented by that file. > > > > This changes the relationship from > > 1. MetricsService <- IOThread <- ProfileImpl[IOData] to > > > > 1. MetricsService <- IOThread > > 2. MetricsService <- ProfileImpl[IOData] > > > > The IO Thread only holds this callback as a convenience for ProfileImplIOData. > > Having this as a direct relationship simplifies IOThread and allows > > MetricsService to be the source of truth. > > > > IOThread can do the dance it needs because it calls back from the IO thread. > > So my concern with this is now it's duplicating some logic between the two. > Specifically, you now need to have "if metrics service is not null" check in > both places. > > I think if you go with my other suggestion of not having MetricsService expose a > callback anymore and for the callback to exist only to the new IOThread function > you added - then it would make sense to use this same callback here that you get > from IOThread. Acknowledged. This is no longer an issue as I've pursued a minimal change given I'm now changing the metrics service.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Awesome cleanup - thanks! LGTM % comments below https://codereview.chromium.org/2517363002/diff/140001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/2517363002/diff/140001/components/metrics/dat... components/metrics/data_use_tracker.h:38: // Updates data usage prefs according to what the PrefService expects. "according to what the PrefService expects" doesn't make very much sense to me. How about: // Updates data usage tracking prefs with the specified values. Also change the header comment on the MetricsService function likewise. https://codereview.chromium.org/2517363002/diff/140001/components/metrics/dat... components/metrics/data_use_tracker.h:52: Nit: Remove extra line. https://codereview.chromium.org/2517363002/diff/140001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2517363002/diff/140001/components/metrics/met... components/metrics/metrics_service.cc:540: const std::string& service_name, int message_size, bool is_cellular) { Nit: Each param on its own line if the first param is. Or just run git cl format which I believe will do that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:160001) has been deleted
mmenke: Please review the io_thread files in this CL. Thanks! https://codereview.chromium.org/2517363002/diff/140001/components/metrics/dat... File components/metrics/data_use_tracker.h (right): https://codereview.chromium.org/2517363002/diff/140001/components/metrics/dat... components/metrics/data_use_tracker.h:38: // Updates data usage prefs according to what the PrefService expects. On 2016/11/30 19:16:27, Alexei Svitkine (slow) wrote: > "according to what the PrefService expects" doesn't make very much sense to me. > > How about: > > // Updates data usage tracking prefs with the specified values. > > Also change the header comment on the MetricsService function likewise. Done. https://codereview.chromium.org/2517363002/diff/140001/components/metrics/dat... components/metrics/data_use_tracker.h:52: On 2016/11/30 19:16:27, Alexei Svitkine (slow) wrote: > Nit: Remove extra line. Done. https://codereview.chromium.org/2517363002/diff/140001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/2517363002/diff/140001/components/metrics/met... components/metrics/metrics_service.cc:540: const std::string& service_name, int message_size, bool is_cellular) { On 2016/11/30 19:16:27, Alexei Svitkine (slow) wrote: > Nit: Each param on its own line if the first param is. Or just run git cl format > which I believe will do that. Done. https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D... https://google.github.io/styleguide/cppguide.html#Function_Calls The style ruling for declarations and invocations is ... Wrap parameter lists which do not fit on a single line as you would wrap arguments in a function call. Either write the call all on a single line, wrap the arguments at the parenthesis, or start the arguments on a new line indented by four spaces and continue at that 4 space indent. In the absence of other considerations, use the minimum number of lines, including placing multiple arguments on each line where appropriate. One argument per line is technically a violation of the minimum space rule but this rule is violated pretty much across Chromium. However, at the same time, the rule need not always be followed as there is discretion allowed by the style guide: If there is still a case where one argument is significantly more readable on its own line, then put it on its own line. So basically, you're free to do what you want as long as it looks good :-)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2517363002/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:553: base::Bind(&UpdateMetricsUsagePrefsOnUIThread)); Maybe just use GetMetricsDataUseForwarder() here? Doesn't get us much, but makes all the hookups go through a single point.
https://codereview.chromium.org/2517363002/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/2517363002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:553: base::Bind(&UpdateMetricsUsagePrefsOnUIThread)); On 2016/12/01 16:10:39, mmenke wrote: > Maybe just use GetMetricsDataUseForwarder() here? Doesn't get us much, but > makes all the hookups go through a single point. Done.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2517363002/#ps200001 (title: "CR Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@chromium.org changed reviewers: - brettw@chromium.org
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1480615018432730,
"parent_rev": "26dd78d9d792dfea14e2333f9a81f98ff739ab87", "commit_rev":
"d8fab536cd7e5d5ddcb2d3fa1eadee4421ce1ae9"}
Message was sent while issue was closed.
Description was changed from ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 ========== to ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 ========== to ========== Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG=662052 Committed: https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512 Cr-Commit-Position: refs/heads/master@{#435657} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512 Cr-Commit-Position: refs/heads/master@{#435657} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
