Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458613004:
Record the initial state of the Physical Web at startup (Closed)
|
Created:
4 years, 1 month ago by mattreynolds Modified:
3 years, 11 months ago CC:
chromium-reviews, mac-reviews_chromium.org, asvitkine+watch_chromium.org, sdefresne+watch_chromium.org, jochen (gone - plz use gerrit) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord the initial state of the Physical Web at startup
Add a histogram PhysicalWeb.InitialState.IosChrome to record the startup
state of Bluetooth, Location, and the Physical Web preference.
BUG=630769
Review-Url: https://codereview.chromium.org/2458613004
Cr-Commit-Position: refs/heads/master@{#442000}
Committed: https://chromium.googlesource.com/chromium/src/+/0b12fdc62e87168506a3e9f0a52cc06b88efea1f
Patch Set 1 #
Total comments: 7
Patch Set 2 : remove pre-iOS10 API #Patch Set 3 : move reporting to IOSChromePhysicalWebDataSource #
Total comments: 10
Patch Set 4 : changes for olivierrobin@, rohitrao@, sdefresne@ #
Total comments: 1
Patch Set 5 : rebase, use local state pref #
Total comments: 6
Patch Set 6 : add startup delay #
Total comments: 4
Patch Set 7 : fix retain loop #Messages
Total messages: 37 (10 generated)
https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:47: locationAuthorized:location_authorized] autorelease]; Not sure if this is kosher. The idea is to let the PhysicalWebInitialStateRecorder take ownership of itself so it can self-destruct once the asynchronous Bluetooth check is complete and the histogram is recorded.
mattreynolds@chromium.org changed reviewers: + rohitrao@chromium.org
Hi Rohit, PTAL
https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:16: // permission. Bits 4:3 encode the Physical Web preference tristate. This feels very brittle. There is no better way to encode this information in a histogram? https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:95: // flux. Unwind this #ifdef once the APIs settle. Have the APIs settled now that iOS10 is out for real? https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:47: locationAuthorized:location_authorized] autorelease]; On 2016/10/27 23:56:04, mattreynolds wrote: > Not sure if this is kosher. The idea is to let the > PhysicalWebInitialStateRecorder take ownership of itself so it can self-destruct > once the asynchronous Bluetooth check is complete and the histogram is recorded. I think this works, but I don't really like it. This feels like it will be way too easy for someone to accidentally turn into a leak or a crash in the future.
https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:16: // permission. Bits 4:3 encode the Physical Web preference tristate. On 2016/10/31 19:41:08, rohitrao wrote: > This feels very brittle. There is no better way to encode this information in a > histogram? We could record the state separately, but combining it into one enum makes it easier for us to tell at a glance which settings are preventing our users from seeing Physical Web results. We have a similar enum for the Today extension. https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:95: // flux. Unwind this #ifdef once the APIs settle. On 2016/10/31 19:41:08, rohitrao wrote: > Have the APIs settled now that iOS10 is out for real? Yeah, I think we can remove the #ifdef now. https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/1/ios/chrome/browser/physical... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:47: locationAuthorized:location_authorized] autorelease]; On 2016/10/31 19:41:08, rohitrao wrote: > On 2016/10/27 23:56:04, mattreynolds wrote: > > Not sure if this is kosher. The idea is to let the > > PhysicalWebInitialStateRecorder take ownership of itself so it can > self-destruct > > once the asynchronous Bluetooth check is complete and the histogram is > recorded. > > I think this works, but I don't really like it. This feels like it will be way > too easy for someone to accidentally turn into a leak or a crash in the future. Agreed, I'd rather not leave it like this. The reason I designed it this way is because I couldn't figure out how else to get the preference state and the bluetooth enabled state in the same place. The bluetooth check is async, so we need someone to hold a reference to the bluetooth delegate until it gets its response. To get the preference, we need a valid PrefService. I think I could move this into the PhysicalWebDataSource if I could get the PrefService from a global context. Is that possible, or do I need a BrowserState?
https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:68: (IOSChromePhysicalWebDataSource*)data_source; This is safe since we're in iOS-specific code. The ugly cast is only necessary because I couldn't find a way to access the PrefService from a global context, any suggestions?
https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.h:6: #define IOS_CHROME_BROWSER_PHYSICAL_WEB_IOS_CHROME_PHYSICAL_WEB_DATA_SOURCE_H_ The data source implementation shouldn't be in ios/chrome/common since it's not used by the Today extension. If this approach seems reasonable I'll split the move into a separate CL.
A few questions, to make sure I understand what's happening here: 1) In an earlier patchset, the class that checked for the current bluetooth state owned and deleted itself. In the latest patchset, does this class now live forever? 2) What's the relationship between PhysicalWebDataSource and IOSChromePhysicalWebDataSource? Instead of casting, does it make sense to add RecordInitialState() as a virtual method in the base class instead? Or, alternatively, should GetApplicationContext() return an instance of the iOS subclass, not an instance of the base class? https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:67: prefService_ = prefService; Instead of caching the prefService, could you extract the values you need and save those instead? That would reduce the risk of the PrefService going away while the bluetooth check is in progress.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:68: (IOSChromePhysicalWebDataSource*)data_source; On 2016/11/04 00:23:07, mattreynolds wrote: > This is safe since we're in iOS-specific code. The ugly cast is only necessary > because I couldn't find a way to access the PrefService from a global context, > any suggestions? Well, the reason you could not find a place with access to the PrefService from a global context is that there are multiple PrefService in that application. So you need to have your preferences be global preferences (i.e. registered in the local_state_ in ApplicationContext) and not in the ChromeBrowserState. If you do, then ApplicationContext has access to both the PrefService and IOSChromePhysicalWebDataSource (since it creates both, see ApplicationContextImpl::CreateLocalState and ApplicationContextImpl::GetPhysicalWebDataSource). My suggestion: 1. move the preference to be store in local_state, 2. pass the pref service to the constructor of IOSChromePhysicalWebDataSource, 3. use the PrefChangeRegistrar to be notified of pref change if necessary otherwise just use the value in initialisation.
olivierrobin@chromium.org changed reviewers: + olivierrobin@chromium.org
https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:21: // permission. Bits 4:3 encode the Physical Web preference tristate. Can you add that this enum is used in histogram and should not be reorder or states removed? https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:70: ios_chrome_data_source->RecordInitialState(pref_service); This method is called on every BVC switch. Is it expected? This does not seem "initial"
Re: Rohit's questions: > 1) Does this class now live forever? Yes > 2) Instead of casting, does it make sense to add RecordInitialState() as a virtual method in the base class instead? No more casting required thanks to Sylvain's suggestion. https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:21: // permission. Bits 4:3 encode the Physical Web preference tristate. On 2016/11/10 18:08:08, Olivier Robin wrote: > Can you add that this enum is used in histogram and should not be reorder or > states removed? Done. https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:67: prefService_ = prefService; On 2016/11/10 13:01:44, rohitrao wrote: > Instead of caching the prefService, could you extract the values you need and > save those instead? That would reduce the risk of the PrefService going away > while the bluetooth check is in progress. Done. https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/start_physical_web_discovery.mm (right): https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:68: (IOSChromePhysicalWebDataSource*)data_source; On 2016/11/10 13:15:42, sdefresne wrote: > On 2016/11/04 00:23:07, mattreynolds wrote: > > This is safe since we're in iOS-specific code. The ugly cast is only necessary > > because I couldn't find a way to access the PrefService from a global context, > > any suggestions? > > Well, the reason you could not find a place with access to the PrefService from > a global context is that there are multiple PrefService in that application. So > you need to have your preferences be global preferences (i.e. registered in the > local_state_ in ApplicationContext) and not in the ChromeBrowserState. > > If you do, then ApplicationContext has access to both the PrefService and > IOSChromePhysicalWebDataSource (since it creates both, see > ApplicationContextImpl::CreateLocalState and > ApplicationContextImpl::GetPhysicalWebDataSource). > > My suggestion: > 1. move the preference to be store in local_state, > 2. pass the pref service to the constructor of IOSChromePhysicalWebDataSource, > 3. use the PrefChangeRegistrar to be notified of pref change if necessary > otherwise just use the value in initialisation. Thanks! This is much cleaner. https://codereview.chromium.org/2458613004/diff/40001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/start_physical_web_discovery.mm:70: ios_chrome_data_source->RecordInitialState(pref_service); On 2016/11/10 18:08:08, Olivier Robin wrote: > This method is called on every BVC switch. Is it expected? This does not seem > "initial" There's a check in PhysicalWebInitialStateRecorder to make sure we don't record more than once. But I agree, this was confusing. In the latest PS I'm recording the state in the IOSChromePhysicalWebDataSource constructor so it's clearer that this should only happen once.
This will also require some changes in the internal repo https://codereview.chromium.org/2458613004/diff/60001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/physical_web_preference_manager.h (right): https://codereview.chromium.org/2458613004/diff/60001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_preference_manager.h:5: #ifndef IOS_CHROME_BROWSER_PHYSICAL_WEB_PHYSICAL_WEB_PREFERENCE_MANAGER_H_ Moved from ios_internal/chrome/browser/physical_web/physical_web_preference_manager.h
I'm working through the changes necessary to safely move the pref into local_state and add the initial state logging. I think this will happen in 4 steps: 1. Move IOSChromePhysicalWebDataSource into ios/chrome/browser. If the data source is going to have browser-specific initialization logic then it shouldn't be shared with the Today extension. This isn't strictly necessary but it would be good to do this now. https://codereview.chromium.org/2494033003 2. Register the new Physical Web pref in Chrome's local state. https://codereview.chromium.org/2494193002 3. In the internal repo, remove the old browser state pref and switch to using the new local state pref. (internal CL) 4. Add PhysicalWebInitialStateRecorder to record state when the IOSChromePhysicalWebDataSource is constructed. (this CL)
Just to double-check, is it ok for this pref to be in LocalState? Is the pref something that is specific to the device, not specific to the user or profile? If we supported multiple users on iOS, would it make sense for two users to set the pref differently?
On 2016/11/11 20:05:05, rohitrao wrote: > Just to double-check, is it ok for this pref to be in LocalState? Is the pref > something that is specific to the device, not specific to the user or profile? > If we supported multiple users on iOS, would it make sense for two users to set > the pref differently? It doesn't make much difference to us, but IMO it makes more sense for this to be device-specific since the feature is tied to the device's location. Would a profile-level pref be synced across devices?
lgtm
Hi Rohit, please take another look
https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/appl... File ios/chrome/browser/application_context_impl.cc (right): https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/appl... ios/chrome/browser/application_context_impl.cc:289: ApplicationContextImpl::GetPhysicalWebDataSource() { When is this method called for the first time? Creating the data source automatically starts the metrics collection, and the bluetooth callback is scheduled on the main thread. Is this something that we could delay for N seconds after startup, so we're not running non-essential code in the critical path of startup? https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm (right): https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm:16: [initialStateRecorder_ collectAndRecordState]; Keeping this object alive forever probably isn't terrible, especially if we can destroy the CBCentralManager when we're done with it. Another option would be to add a PhysicalWebInitialStateRecorderDelegate method that's called when the recording is done, which would allow the owner to destroy the recorder. The savings might not be worth the overhead. https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:88: [centralManager_ setDelegate:nil]; Could we completely destroy the centralManager_ when we're done with it in this function? Does it need to stay alive for the lifetime of the app?
https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/appl... File ios/chrome/browser/application_context_impl.cc (right): https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/appl... ios/chrome/browser/application_context_impl.cc:289: ApplicationContextImpl::GetPhysicalWebDataSource() { On 2016/12/12 12:54:17, rohitrao wrote: > When is this method called for the first time? Creating the data source > automatically starts the metrics collection, and the bluetooth callback is > scheduled on the main thread. Is this something that we could delay for N > seconds after startup, so we're not running non-essential code in the critical > path of startup? It's first called during [MainController startUpBrowserForegroundInitialization] when the browser view is switched. I added a 10 second delay so we won't record the state until after initialization is complete. The delay could affect the recorded state (eg, user could grant the Location app permission before the delay expires) but it's unlikely to be significant. The preference state likely WILL change during the delay period since we have logic to auto-enable the pref when certain conditions are met. We intentionally capture the pref state before the delay to avoid grabbing it after the auto-enable logic has executed. https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm (right): https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/ios_chrome_physical_web_data_source.mm:16: [initialStateRecorder_ collectAndRecordState]; On 2016/12/12 12:54:17, rohitrao wrote: > Keeping this object alive forever probably isn't terrible, especially if we can > destroy the CBCentralManager when we're done with it. > > Another option would be to add a PhysicalWebInitialStateRecorderDelegate method > that's called when the recording is done, which would allow the owner to destroy > the recorder. The savings might not be worth the overhead. I modified PWISR to destroy the CBCentralManager after we get the bluetooth state callback. I think it's not worth the overhead to destroy the recorder. https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/80001/ios/chrome/browser/phys... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:88: [centralManager_ setDelegate:nil]; On 2016/12/12 12:54:17, rohitrao wrote: > Could we completely destroy the centralManager_ when we're done with it in this > function? Does it need to stay alive for the lifetime of the app? Done.
lgtm https://codereview.chromium.org/2458613004/diff/100001/ios/chrome/browser/phy... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/100001/ios/chrome/browser/phy... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:85: if (startupDelayTimer_.get()) { Note that this condition will never be true, because the timer retains self, preventing it from being destroyed. https://codereview.chromium.org/2458613004/diff/100001/ios/chrome/browser/phy... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:116: startupDelayTimer_.reset([NSTimer NSTimers are a little scary because they retain their targets, which will create a retain loop in this case. This will keep the PhysicalWebInitialStateRecorder alive until the timer fires, which means it might outlive its owner. Will that cause problems if it ever happens?
Thanks Rohit! https://codereview.chromium.org/2458613004/diff/100001/ios/chrome/browser/phy... File ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm (right): https://codereview.chromium.org/2458613004/diff/100001/ios/chrome/browser/phy... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:85: if (startupDelayTimer_.get()) { On 2017/01/04 19:19:22, rohitrao wrote: > Note that this condition will never be true, because the timer retains self, > preventing it from being destroyed. I've moved the check to a public method outside dealloc so the owner of the PhysicalWebInitialStateRecorder can ensure the timer is invalidated. https://codereview.chromium.org/2458613004/diff/100001/ios/chrome/browser/phy... ios/chrome/browser/physical_web/physical_web_initial_state_recorder.mm:116: startupDelayTimer_.reset([NSTimer On 2017/01/04 19:19:22, rohitrao wrote: > NSTimers are a little scary because they retain their targets, which will create > a retain loop in this case. This will keep the PhysicalWebInitialStateRecorder > alive until the timer fires, which means it might outlive its owner. Will that > cause problems if it ever happens? Wow that is terrible. Now the IOSChromePhysicalWebDataSource is responsible for invalidating the initial state recorder when it is destroyed. I discovered that we were invalidating the timer inappropriately in the normal case. With repeats:NO, an NSTimer invalidates itself when it fires. I removed the invalidate call in startupDelayElapsed. Also, NSTimer's scheduledTimerWithTimeInterval doesn't retain itself. We'd still like to hold a reference in a scoped_nspointer so we can invalidate it before it's fired, so I added retain.
mattreynolds@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in tools/metrics/histograms
On 2017/01/04 at 23:13:14, mattreynolds wrote: > jochen@chromium.org: Please review changes in tools/metrics/histograms
I can only approve UseCounter additions, please ask a member of the metrics team for review (tools/metrics/OWNERS)
Description was changed from ========== Record the initial state of the Physical Web at startup Add a histogram PhysicalWeb.InitialState.IosChrome to record the startup state of Bluetooth, Location, and the Physical Web preference. BUG=630769 ========== to ========== Record the initial state of the Physical Web at startup Add a histogram PhysicalWeb.InitialState.IosChrome to record the startup state of Bluetooth, Location, and the Physical Web preference. BUG=630769 ==========
mattreynolds@chromium.org changed reviewers: + rkaplow@chromium.org - jochen@chromium.org
Hi rkaplow, PTAL at tools/metrics/histograms
lgtm
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2458613004/#ps120001 (title: "fix retain loop")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483725659652650,
"parent_rev": "dc2a9e048fd191a43a35c8f6ee1e4cecb7fc919d", "commit_rev":
"0b12fdc62e87168506a3e9f0a52cc06b88efea1f"}
Message was sent while issue was closed.
Description was changed from ========== Record the initial state of the Physical Web at startup Add a histogram PhysicalWeb.InitialState.IosChrome to record the startup state of Bluetooth, Location, and the Physical Web preference. BUG=630769 ========== to ========== Record the initial state of the Physical Web at startup Add a histogram PhysicalWeb.InitialState.IosChrome to record the startup state of Bluetooth, Location, and the Physical Web preference. BUG=630769 Review-Url: https://codereview.chromium.org/2458613004 Cr-Commit-Position: refs/heads/master@{#442000} Committed: https://chromium.googlesource.com/chromium/src/+/0b12fdc62e87168506a3e9f0a52c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0b12fdc62e87168506a3e9f0a52c... |
