Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1018)

Issue 2801603008: [Cronet iOS] Add base::StatisticsRecorder::IsActive() check (Closed)

Created:
3 years, 8 months ago by xunjieli
Modified:
3 years, 7 months ago
Reviewers:
mef
CC:
chromium-reviews, cbentzel+watch_chromium.org, ios-reviews_chromium.org, net-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Cronet iOS] Add base::StatisticsRecorder::IsActive() check This CL adds a base::StatisticsRecorder::IsActive() check when user calls GetHistogramDeltas(). BUG=707836 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M components/cronet/ios/cronet_environment.mm View 1 chunk +1 line, -1 line 4 comments Download

Messages

Total messages: 8 (3 generated)
xunjieli
Misha: PTAL.
3 years, 8 months ago (2017-04-06 21:36:43 UTC) #3
xunjieli
https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/cronet_environment.mm File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/cronet_environment.mm#newcode128 components/cronet/ios/cronet_environment.mm:128: base::StatisticsRecorder::Initialize(); We are calling base::StatisticsRecorder::Initialize() here
3 years, 8 months ago (2017-04-06 21:37:30 UTC) #4
mef
https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/cronet_environment.mm File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/cronet_environment.mm#newcode128 components/cronet/ios/cronet_environment.mm:128: base::StatisticsRecorder::Initialize(); On 2017/04/06 21:37:29, xunjieli wrote: > We are ...
3 years, 8 months ago (2017-04-06 22:08:11 UTC) #5
xunjieli
https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/cronet_environment.mm File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/cronet_environment.mm#newcode352 components/cronet/ios/cronet_environment.mm:352: DCHECK(base::StatisticsRecorder::IsActive()); On 2017/04/06 22:08:11, mef wrote: > What would ...
3 years, 8 months ago (2017-04-06 23:58:01 UTC) #6
mef
3 years, 8 months ago (2017-04-07 20:49:33 UTC) #7
On 2017/04/06 23:58:01, xunjieli wrote:
>
https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/crone...
> File components/cronet/ios/cronet_environment.mm (right):
> 
>
https://codereview.chromium.org/2801603008/diff/1/components/cronet/ios/crone...
> components/cronet/ios/cronet_environment.mm:352:
> DCHECK(base::StatisticsRecorder::IsActive());
> On 2017/04/06 22:08:11, mef wrote:
> > What would happen if they call this on background thread before init
> completes?
> 
> Is there an entry point  (if not CronetEnvironment::Initialize()) which
> guarantees to complete before GetHistogramDeltas() is called?
I don't think so, and I do think that it is a bug.

> On Android, we are doing StatisticsRecorder::Initialize() in JNIOnload() which
> guarantees to be called before the user could do
> CronetEngine.GetHistogramDeltas().

There are similar static constructors and other global initializers that could
be used on iOS, but currently are not.

Powered by Google App Engine
This is Rietveld 408576698