Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334653006:
mac: Prevent Address Book permissions dialog from erroneously appearing. (Closed)
|
Created:
6 years, 6 months ago by erikchen Modified:
6 years, 6 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionmac: Prevent Address Book permissions dialog from erroneously appearing.
This CL fixes a bug where auto-updating Chrome can cause the Address Book
permissions dialog to appear, even though the user has already granted/denied
Chrome access to the user's Address Book.
When the Chrome binary is updated, chrome_autofill_client now passes the
information to personal_data_manager, which will refrain from accessing the
address book, if doing so would cause a blocking dialog to appear.
BUG=381763
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278254
Patch Set 1 : First. #
Total comments: 42
Patch Set 2 : Comments from isherman. #Patch Set 3 : Add metrics. Note the addition of histograms.xml #
Total comments: 10
Patch Set 4 : Respond to isherman comments. #
Total comments: 17
Patch Set 5 : Respond to isherman comments. #Patch Set 6 : Fix test. #
Total comments: 3
Messages
Total messages: 22 (0 generated)
isherman: Please review. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:677: DCHECK([NSThread isMainThread]); I'm not confident that this is true, but the existing code would be pretty unsafe/broken if this condition wasn't true. If you prefer, I could redispatch to the main thread if it isn't already there via gcd instead of having this DCHECK. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:16: #if defined(OS_MACOSX) && !defined(OS_IOS) This was a lot of #ifdef-ing. Let me know if you prefer a different solution.
isherman: Ping?
Hmm. I'm a little worried that this will lead to an inconsistent user experience for users who actually want to use AddressBook data for Autofill. To the user, it will just feel like sometimes Autofill works, and sometimes it doesn't, quite mysteriously. How sure are we that this is the best we can do? If it *is* the best we can do, I'd definitely advocate for adding metrics to measure how disruptive this is. Given how frequently Chrome is released, and that many users leave Chrome running for long periods of time, I could imagine this reducing the usage of AddressBook integration by an order of magnitude. If it does, and we can't provide a good user experience without this hack, then the value proposition of the integration really goes way down :( https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.h:19: AutofillKeystoneObserverMac(AutofillKeystoneObserverMacDelegate* delegate); Please document lifetime expectations. (It seems that you are expecting that the |delegate| is guaranteed to outlive the observer?) https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.h:24: }; I'm noticing that you're adding four new files for something that seems pretty simple. Do we really need such a refined class hierarchy? Could the ChromeAutofillClient class just own a new member variable, and that new class implement all of the relevant functionality? That would be a simpler design, IMO. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac.mm (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.mm:16: AutofillKeystoneObserverMacDelegate* delegate_; Please document lifetime expectations here as well. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac_delegate.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac_delegate.h:16: }; nit: Should this be in the autofill namespace? https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:15: #endif // __OBJC__ Why is this header ever included on non-Mac platforms? https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:17: namespace keystone_glue { nit: Please leave a blank line after this one... https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:17: namespace keystone_glue { Why is this namespace needed? https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:40: } // namespace keystone_glue nit: ... and before this one. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:16: #if defined(OS_MACOSX) && !defined(OS_IOS) On 2014/06/12 23:54:41, erikchen1 wrote: > This was a lot of #ifdef-ing. Let me know if you prefer a different solution. Would it make sense for the client to own a class that is a delegate, rather than being one directly? Then you would only need two #ifdefs: One for the member variable, and one for the initialization of that member variable. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:31: #endif // defined(OS_MACOSX) && !defined(OS_IOS) It's fine to forward-declare the class on all platforms, even if it's only needed on one platform. If you prefer to keep the #ifdefs, though, please move this to be below the other forward-declarations. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:82: // AutofillKeystoneObserverMacDelegate nit: Please add a trailing colon. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:110: base::WeakPtr<AutofillPopupControllerImpl> popup_controller_; nit: Please leave a blank line below this one. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:114: // pointer cannot be wrapped in a scoped_ptr. I'm pretty sure that scoped_ptrs work fine with forward-declared classes. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:29: if (manager) Why might the manager be null? https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager.h:214: // The Chrome binary is in the process of being changed, or has been changed. Please indicate why this method is relevant/important. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:36: // are only intended to be accessed from the main thread. There's generally no need to document that something is not thread safe, unless readers of the code might have some reason to guess that it is. Chrome code is assumed to be non-thread safe by default. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:42: // since it presents a blocking dialog. I found it kind of hard to understand what this comment meant. Could you please rephrase it to be a little clearer? It might help to explicitly describe the sequence of events that leads to a bad state in a more overt way, perhaps as a numbered list, or a simple ASCII art diagram. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:44: static bool kBinaryChanged = false; These are not constants, so they should not be named as such. You can either name them as g_accessed_address_book, etc.; or, better yet, make them static private members of the class. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:139: // changed. Same comment applies here. If you find that it's hard to succinctly describe what this is doing, it might make sense to have one verbose comment, and then from the other comment just say "See this verbose comment here, it'll explain everything."
isherman: PTAL I've added metrics for the frequency that the logic in this CL gets invoked. I suspect that it's relatively rare, since Chrome must be updated before it ever attempts to access the Address Book, and focusing almost any form field will cause an Address Book access. While it is bad to inconsistently show Address Book results, it is /definitely/ bad to show a blocking modal dialog when a user selects almost any form field. Attempts to access the Address Book are only skipped when it's guaranteed that doing so would cause an incorrect appearance of the permissions dialog. This is the best solution I have short of reworking the entire mechanism of auto-update on mac. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.h:19: AutofillKeystoneObserverMac(AutofillKeystoneObserverMacDelegate* delegate); On 2014/06/14 01:18:50, Ilya Sherman wrote: > Please document lifetime expectations. (It seems that you are expecting that > the |delegate| is guaranteed to outlive the observer?) I've added the lifetime expectation to the comment at the top of the class. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.h:24: }; On 2014/06/14 01:18:50, Ilya Sherman wrote: > I'm noticing that you're adding four new files for something that seems pretty > simple. Do we really need such a refined class hierarchy? Could the > ChromeAutofillClient class just own a new member variable, and that new class > implement all of the relevant functionality? That would be a simpler design, > IMO. Your suggestion is implementable, but it does not significantly reduce the number of lines added to chrome_autofill_client.{h,cc}, and also does not significantly reduce the number of new classes. The organization of the new code in this CL is convoluted because I need to call Obj-C code from/into chrome_autofill_client, which is compiled in c++. Let me know if you want a more detailed explanation. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac.mm (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.mm:16: AutofillKeystoneObserverMacDelegate* delegate_; On 2014/06/14 01:18:50, Ilya Sherman wrote: > Please document lifetime expectations here as well. Done. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac_delegate.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac_delegate.h:16: }; On 2014/06/14 01:18:51, Ilya Sherman wrote: > nit: Should this be in the autofill namespace? Yes, done. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:15: #endif // __OBJC__ On 2014/06/14 01:18:51, Ilya Sherman wrote: > Why is this header ever included on non-Mac platforms? I don't know? https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:17: namespace keystone_glue { On 2014/06/14 01:18:51, Ilya Sherman wrote: > Why is this namespace needed? I was under the impression that there is a style guideline that enums should never be released into the global namespace. Looking through Google/Chromium style guides, I could not find this statement, but I still think its good practice. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:17: namespace keystone_glue { On 2014/06/14 01:18:51, Ilya Sherman wrote: > nit: Please leave a blank line after this one... Done (adding vertical whitespace). I could not find a style guideline that explicitly states this, and the Chrome codebase is certainly not consistent about this. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.h:40: } // namespace keystone_glue On 2014/06/14 01:18:51, Ilya Sherman wrote: > nit: ... and before this one. Done. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:16: #if defined(OS_MACOSX) && !defined(OS_IOS) On 2014/06/14 01:18:51, Ilya Sherman wrote: > On 2014/06/12 23:54:41, erikchen1 wrote: > > This was a lot of #ifdef-ing. Let me know if you prefer a different solution. > > Would it make sense for the client to own a class that is a delegate, rather > than being one directly? Then you would only need two #ifdefs: One for the > member variable, and one for the initialization of that member variable. That adds another layer of indirection to reduce the number of ifdefs in this file by 3, which I do not think is a worthwhile trade.. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:31: #endif // defined(OS_MACOSX) && !defined(OS_IOS) On 2014/06/14 01:18:51, Ilya Sherman wrote: > It's fine to forward-declare the class on all platforms, even if it's only > needed on one platform. If you prefer to keep the #ifdefs, though, please move > this to be below the other forward-declarations. I've removed the ifdefs here. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:82: // AutofillKeystoneObserverMacDelegate On 2014/06/14 01:18:51, Ilya Sherman wrote: > nit: Please add a trailing colon. Done. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:110: base::WeakPtr<AutofillPopupControllerImpl> popup_controller_; On 2014/06/14 01:18:51, Ilya Sherman wrote: > nit: Please leave a blank line below this one. Done. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client.h:114: // pointer cannot be wrapped in a scoped_ptr. On 2014/06/14 01:18:51, Ilya Sherman wrote: > I'm pretty sure that scoped_ptrs work fine with forward-declared classes. I've updated the comment to further indicate why this must remain a not-smart pointer. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:29: if (manager) On 2014/06/14 01:18:51, Ilya Sherman wrote: > Why might the manager be null? There is no comment indicating that the method is guaranteed to return a non-NULL pointer. Is this the case? If so, I will update the method and this logic. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager.h:214: // The Chrome binary is in the process of being changed, or has been changed. On 2014/06/14 01:18:51, Ilya Sherman wrote: > Please indicate why this method is relevant/important. Done. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:36: // are only intended to be accessed from the main thread. On 2014/06/14 01:18:51, Ilya Sherman wrote: > There's generally no need to document that something is not thread safe, unless > readers of the code might have some reason to guess that it is. Chrome code is > assumed to be non-thread safe by default. okay. I've removed comments about thread safety. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:42: // since it presents a blocking dialog. On 2014/06/14 01:18:51, Ilya Sherman wrote: > I found it kind of hard to understand what this comment meant. Could you please > rephrase it to be a little clearer? It might help to explicitly describe the > sequence of events that leads to a bad state in a more overt way, perhaps as a > numbered list, or a simple ASCII art diagram. I've completely rewritten the comments. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:44: static bool kBinaryChanged = false; On 2014/06/14 01:18:51, Ilya Sherman wrote: > These are not constants, so they should not be named as such. You can either > name them as g_accessed_address_book, etc.; or, better yet, make them static > private members of the class. I renamed them as global variables. If you strongly prefer, I can make them static private members of personal_data_manager, but that adds complexity to the logic of this class. The meaning and intended use of the variables is tied to accessing the address book, rather than the personal_data_manager. https://codereview.chromium.org/334653006/diff/20001/components/autofill/core... components/autofill/core/browser/personal_data_manager_mac.mm:139: // changed. On 2014/06/14 01:18:51, Ilya Sherman wrote: > Same comment applies here. If you find that it's hard to succinctly describe > what this is doing, it might make sense to have one verbose comment, and then > from the other comment just say "See this verbose comment here, it'll explain > everything." I've used your suggestion and changed the comment to redirect the user to the previous comment for more explanation.
On 2014/06/16 20:30:47, erikchen1 wrote: > I've added metrics for the frequency that the logic in this CL gets invoked. I > suspect that it's relatively rare, since Chrome must be updated before it ever > attempts to access the Address Book, and focusing almost any form field will > cause an Address Book access. The empirical evidence I've observed based on my own usage is that this sequence of events is not actually all that rare. That said, empirical evidence is clearly not the best data to go on. I fully support landing this CL and looking at the metrics to judge whether my empirical evidence is representative or not. > While it is bad to inconsistently show Address Book results, it is /definitely/ > bad to show a blocking modal dialog when a user selects almost any form field. > Attempts to access the Address Book are only skipped when it's guaranteed that > doing so would cause an incorrect appearance of the permissions dialog. This is > the best solution I have short of reworking the entire mechanism of auto-update > on mac. I agree that we shouldn't show the blocking modal dialog unnecessarily. However, the more caveats we add to our Address Book integration, the more tempted I am to just drop the feature. I'd rather have zero integration than something that feels half-baked, even if half-baked is the best we can do given the hurdles the OS puts in our way. This is not in any way a criticism of your work -- you've written great code, done some seriously impressive debugging, and have come up with nice, clever solutions to subtle issues. I really appreciate your efforts on this, and would very much prefer to keep the feature as long as we can provide a good user experience. Given that we have lots of good code implemented for M37, I think we should definitely ship it and see what the metrics are. But I think it'll be really important to look carefully at those metrics, and decide whether we're providing a quality user experience at the end of the day. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.h:24: }; On 2014/06/16 20:30:45, erikchen1 wrote: > On 2014/06/14 01:18:50, Ilya Sherman wrote: > > I'm noticing that you're adding four new files for something that seems pretty > > simple. Do we really need such a refined class hierarchy? Could the > > ChromeAutofillClient class just own a new member variable, and that new class > > implement all of the relevant functionality? That would be a simpler design, > > IMO. > > Your suggestion is implementable, but it does not significantly reduce the > number of lines added to chrome_autofill_client.{h,cc}, and also does not > significantly reduce the number of new classes. The organization of the new code > in this CL is convoluted because I need to call Obj-C code from/into > chrome_autofill_client, which is compiled in c++. Let me know if you want a more > detailed explanation. Here's a more complete implementation along the lines of what I was thinking: https://codereview.chromium.org/339053003 https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:42: // (3). For more details, see crbug.com/381763. nit: Please prepend "http://" to the URL, so that it's more likely to be automagically linkified. https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:49: // This variable is set to true after the Address Book is accessed for the nit: "This variable is set" -> "Set" https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:51: static bool g_accessed_address_book = false; nit: Please leave a blank line between consecutive commented variables, unless the comment above the first applies to the rest as well. https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:55: static bool g_recorded_skip_metrics = false; Hmm, only recording the metrics once per launch seems like it would introduce considerable bias, since the users who are potentially affected by this issue are the very same users who don't relaunch Chrome as frequently. What we really want to measure is: Of the times that we would have shown Address Book suggestions in the Autofill popup UI, how many times are we prevented from doing so by this logic? Of course, we can't directly measure that, because we don't know whether an Address Book that we can't access actually has useful data in it or not. But I think we can do better than measuring just once per launch. Perhaps we should measure once per time the popup is shown? https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:72: // Book was skipped beccause doing so would cause the Address Book permissions nit: "beccause" -> "because"
isherman: PTAL You bring up very reasonable points about the "half-baked" feel of Autofill Address Book integration. +1 to landing this CL and metrics, and using them to determine whether we actually want any Address Book integration at all. https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... File chrome/browser/autofill/autofill_keystone_observer_mac.h (right): https://codereview.chromium.org/334653006/diff/20001/chrome/browser/autofill/... chrome/browser/autofill/autofill_keystone_observer_mac.h:24: }; On 2014/06/17 03:29:16, Ilya Sherman wrote: > On 2014/06/16 20:30:45, erikchen1 wrote: > > On 2014/06/14 01:18:50, Ilya Sherman wrote: > > > I'm noticing that you're adding four new files for something that seems > pretty > > > simple. Do we really need such a refined class hierarchy? Could the > > > ChromeAutofillClient class just own a new member variable, and that new > class > > > implement all of the relevant functionality? That would be a simpler > design, > > > IMO. > > > > Your suggestion is implementable, but it does not significantly reduce the > > number of lines added to chrome_autofill_client.{h,cc}, and also does not > > significantly reduce the number of new classes. The organization of the new > code > > in this CL is convoluted because I need to call Obj-C code from/into > > chrome_autofill_client, which is compiled in c++. Let me know if you want a > more > > detailed explanation. > > Here's a more complete implementation along the lines of what I was thinking: > https://codereview.chromium.org/339053003 :O Your implementation is much cleaner and shorter. I've applied it with minimal modifications to my CL. Thanks! https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:42: // (3). For more details, see crbug.com/381763. On 2014/06/17 03:29:17, Ilya Sherman wrote: > nit: Please prepend "http://" to the URL, so that it's more likely to be > automagically linkified. Done. https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:49: // This variable is set to true after the Address Book is accessed for the On 2014/06/17 03:29:17, Ilya Sherman wrote: > nit: "This variable is set" -> "Set" Done. https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:51: static bool g_accessed_address_book = false; On 2014/06/17 03:29:17, Ilya Sherman wrote: > nit: Please leave a blank line between consecutive commented variables, unless > the comment above the first applies to the rest as well. Done. https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:55: static bool g_recorded_skip_metrics = false; On 2014/06/17 03:29:17, Ilya Sherman wrote: > Hmm, only recording the metrics once per launch seems like it would introduce > considerable bias, since the users who are potentially affected by this issue > are the very same users who don't relaunch Chrome as frequently. > > What we really want to measure is: Of the times that we would have shown Address > Book suggestions in the Autofill popup UI, how many times are we prevented from > doing so by this logic? Of course, we can't directly measure that, because we > don't know whether an Address Book that we can't access actually has useful data > in it or not. But I think we can do better than measuring just once per launch. > Perhaps we should measure once per time the popup is shown? I've removed the logic to record metrics once per launch - the new logic records metrics once per time the popup is shown. https://codereview.chromium.org/334653006/diff/140001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:72: // Book was skipped beccause doing so would cause the Address Book permissions On 2014/06/17 03:29:17, Ilya Sherman wrote: > nit: "beccause" -> "because" Done.
https://codereview.chromium.org/334653006/diff/210001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/334653006/diff/210001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:42: } // namespace keystone_glue I would prefer not to add this namespace as part of this CL. If you're really keen on adding this, please split that change off to a separate CL, and ask an owner of //chrome/browser/mac to review it. In general, though, it's uncommon to use namespaces around enums in //chrome. Instead, enums are typically named in a wacky verbose style. For this enum, the Chrome convention would be something like this: enum KeystoneAutoupdateStatus { KEYSTONE_AUTOUPDATE_STATUS_NONE = 0, KEYSTONE_AUTOUPDATE_STATUS_REGISTERED, // etc. } Specifically, the style is to pick a verbose name for the enum, so that the enum name is clear and does not conflict with anything, and then to have each enumerated constant's name contain the entire enum name as a prefix. I assume that this is preferred over namespaces because most other code isn't namespaced, and its a little awkward to use namespaces just for enums -- especially given that Objective-C doesn't support namespaces. https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:86: // Creates bridge_wrapper_, which is responsible for dealing with Keystone nit: "bridge_wrapper_" -> "|bridge_wrapper_|" https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:108: // scoped_ptr. This explanation isn't quite right anymore. The issue is that the class is defined in a Mac-only file, and hence the definition isn't visible from the ChromeAutofillClient destructor. Which is similar to what you've written, but there's no header file that requires ObjC++ to compile. https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:97: bridge_wrapper_ = new AutofillKeystoneBridgeWrapper(GetPersonalDataManager()); I'm realizing that I forgot to answer your question from earlier: GetPersonalDataManager() is indeed guaranteed to return a non-null value in production code. (It's been a longstanding pet peeve of mine that it can be NULL in test code, rather than being properly mocked or faked or stubbed out.) https://codereview.chromium.org/334653006/diff/210001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/210001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:159: RecordSkipReprompt(false); This isn't actually guaranteed to be called only when a popup is shown, right? I wonder whether we should jump through some extra hoops to get the metric to be recorded only when it's really relevant. https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1565: +<histogram name="Autofill.AddressBookReprompt" enum="BooleanAvailable"> nit: Please use a more custom-tailored enum, possibly defining a new one if there isn't an appropriate one already defined. https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1565: +<histogram name="Autofill.AddressBookReprompt" enum="BooleanAvailable"> Let's name the histogram something more like "Autofill.AddressBook.AccessSuppressed" or "Autofill.AddressBook.QuerySkipped" or something like that. https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1569: + so would incorrectly cause the appearance of the permissions dialog. Please go into a bit more detail as to why this might happen, i.e. that the binary on disk has changed via autoupdate prior to the first AddressBook access.
isherman: Thanks! PTAL. https://codereview.chromium.org/334653006/diff/210001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/334653006/diff/210001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:42: } // namespace keystone_glue On 2014/06/17 19:55:01, Ilya Sherman wrote: > I would prefer not to add this namespace as part of this CL. If you're really > keen on adding this, please split that change off to a separate CL, and ask an > owner of //chrome/browser/mac to review it. In general, though, it's uncommon > to use namespaces around enums in //chrome. Instead, enums are typically named > in a wacky verbose style. For this enum, the Chrome convention would be > something like this: > > enum KeystoneAutoupdateStatus { > KEYSTONE_AUTOUPDATE_STATUS_NONE = 0, > KEYSTONE_AUTOUPDATE_STATUS_REGISTERED, > // etc. > } > > Specifically, the style is to pick a verbose name for the enum, so that the enum > name is clear and does not conflict with anything, and then to have each > enumerated constant's name contain the entire enum name as a prefix. I assume > that this is preferred over namespaces because most other code isn't namespaced, > and its a little awkward to use namespaces just for enums -- especially given > that Objective-C doesn't support namespaces. I have reverted the namespace change, and well as the DCHECK in keystone_glue.mm, and the additional comment. I was trying to do the "right" thing, but I'm happy to write less code and have fewer reviewers. https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:86: // Creates bridge_wrapper_, which is responsible for dealing with Keystone On 2014/06/17 19:55:01, Ilya Sherman wrote: > nit: "bridge_wrapper_" -> "|bridge_wrapper_|" Done. https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.h:108: // scoped_ptr. On 2014/06/17 19:55:01, Ilya Sherman wrote: > This explanation isn't quite right anymore. The issue is that the class is > defined in a Mac-only file, and hence the definition isn't visible from the > ChromeAutofillClient destructor. Which is similar to what you've written, but > there's no header file that requires ObjC++ to compile. You're right. I've fixed the explanation. https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/210001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:97: bridge_wrapper_ = new AutofillKeystoneBridgeWrapper(GetPersonalDataManager()); On 2014/06/17 19:55:01, Ilya Sherman wrote: > I'm realizing that I forgot to answer your question from earlier: > GetPersonalDataManager() is indeed guaranteed to return a non-null value in > production code. (It's been a longstanding pet peeve of mine that it can be > NULL in test code, rather than being properly mocked or faked or stubbed out.) ah, thanks. https://codereview.chromium.org/334653006/diff/210001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/210001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:159: RecordSkipReprompt(false); On 2014/06/17 19:55:01, Ilya Sherman wrote: > This isn't actually guaranteed to be called only when a popup is shown, right? > I wonder whether we should jump through some extra hoops to get the metric to be > recorded only when it's really relevant. I think that the question you are trying to answer with metrics is: "Should we have Address Book integration at all?" The relevant metrics are: 1a. How often (or for how many people) does Address Book provide useful autofill results? 1b. How often is one of those results actually used? 2. Of instances where Chrome would provide results (1a) or useful results (1b), how often does the logic in this CL prevent those useful results from appearing? The answer lies in the ratio of (2) and (1a), and the ratio of (1a) to (1b). Neither (1a) nor (1b) is currently measured. I think that the metric that you are asking for in your comment is (2). The metric I am recording does not tell you (2), but it provides sufficient information to answer the question. Let A = # of instances that line 156 is called. Let B = # of instances that line 159 is called. Then A / B approximates the ratio of (2) : (1a). https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1565: +<histogram name="Autofill.AddressBookReprompt" enum="BooleanAvailable"> On 2014/06/17 19:55:01, Ilya Sherman wrote: > Let's name the histogram something more like > "Autofill.AddressBook.AccessSuppressed" or "Autofill.AddressBook.QuerySkipped" > or something like that. I used "Autofill.AddressBook.AccessSkipped" https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1565: +<histogram name="Autofill.AddressBookReprompt" enum="BooleanAvailable"> On 2014/06/17 19:55:01, Ilya Sherman wrote: > nit: Please use a more custom-tailored enum, possibly defining a new one if > there isn't an appropriate one already defined. I used a new enum BooleanSkipped https://codereview.chromium.org/334653006/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1569: + so would incorrectly cause the appearance of the permissions dialog. On 2014/06/17 19:55:01, Ilya Sherman wrote: > Please go into a bit more detail as to why this might happen, i.e. that the > binary on disk has changed via autoupdate prior to the first AddressBook access. I've expanded the comment.
LGTM assuming you want to keep the current metric, but please see my comment below: https://codereview.chromium.org/334653006/diff/210001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/334653006/diff/210001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:159: RecordSkipReprompt(false); On 2014/06/17 22:36:06, erikchen1 wrote: > On 2014/06/17 19:55:01, Ilya Sherman wrote: > > This isn't actually guaranteed to be called only when a popup is shown, right? > > > I wonder whether we should jump through some extra hoops to get the metric to > be > > recorded only when it's really relevant. > > I think that the question you are trying to answer with metrics is: > > "Should we have Address Book integration at all?" > > The relevant metrics are: > 1a. How often (or for how many people) does Address Book provide useful autofill > results? > 1b. How often is one of those results actually used? > 2. Of instances where Chrome would provide results (1a) or useful results (1b), > how often does the logic in this CL prevent those useful results from appearing? > The answer lies in the ratio of (2) and (1a), and the ratio of (1a) to (1b). > > Neither (1a) nor (1b) is currently measured. I think that the metric that you > are asking for in your comment is (2). The metric I am recording does not tell > you (2), but it provides sufficient information to answer the question. Let A = > # of instances that line 156 is called. Let B = # of instances that line 159 is > called. Then A / B approximates the ratio of (2) : (1a). The trouble with this approximation is that we currently request AddressBook data before the popup is shown. So, hits on line 159 would proxy for surfacing useful AddressBook data, even for users who have no AddressBook data stored. Moreover, we query for AddressBook data even on forms that aren't autofillable (right?). Therefore, much (most?) of the data would come from forms that aren't relevant to Autofill. It sounds like you're asserting that these caveats don't matter. I'm not sure -- it's entirely possible that users who frequently use AddressBook integration also fill out autofillable forms more frequently, whereas other users don't... but still encounter other forms frequently. If you think that measuring what we actually show in the popup vs. what we would show is too hard, then we can proceed with this current metric. But if we do so, I think we should look carefully at the absolute numbers, as well as the ratio. If line 156 is hit by a large number of users in a day, then I'd be worried about that result, even if line 159 is hit by a much larger number of users.
On Tue, Jun 17, 2014 at 4:50 PM, <isherman@chromium.org> wrote: > LGTM assuming you want to keep the current metric, but please see my > comment > below: > > > > https://codereview.chromium.org/334653006/diff/210001/ > components/autofill/core/browser/personal_data_manager_mac.mm > File components/autofill/core/browser/personal_data_manager_mac.mm > (right): > > https://codereview.chromium.org/334653006/diff/210001/ > components/autofill/core/browser/personal_data_manager_mac.mm#newcode159 > components/autofill/core/browser/personal_data_manager_mac.mm:159: > RecordSkipReprompt(false); > On 2014/06/17 22:36:06, erikchen1 wrote: > >> On 2014/06/17 19:55:01, Ilya Sherman wrote: >> > This isn't actually guaranteed to be called only when a popup is >> > shown, right? > > > I wonder whether we should jump through some extra hoops to get the >> > metric to > >> be >> > recorded only when it's really relevant. >> > > I think that the question you are trying to answer with metrics is: >> > > "Should we have Address Book integration at all?" >> > > The relevant metrics are: >> 1a. How often (or for how many people) does Address Book provide >> > useful autofill > >> results? >> 1b. How often is one of those results actually used? >> 2. Of instances where Chrome would provide results (1a) or useful >> > results (1b), > >> how often does the logic in this CL prevent those useful results from >> > appearing? > >> The answer lies in the ratio of (2) and (1a), and the ratio of (1a) to >> > (1b). > > Neither (1a) nor (1b) is currently measured. I think that the metric >> > that you > >> are asking for in your comment is (2). The metric I am recording does >> > not tell > >> you (2), but it provides sufficient information to answer the >> > question. Let A = > >> # of instances that line 156 is called. Let B = # of instances that >> > line 159 is > >> called. Then A / B approximates the ratio of (2) : (1a). >> > > The trouble with this approximation is that we currently request > AddressBook data before the popup is shown. So, hits on line 159 would > proxy for surfacing useful AddressBook data, even for users who have no > AddressBook data stored. Moreover, we query for AddressBook data even > Both 'A' and 'B' are independent of whether the returned results are "useful", so I expect the ratio to be independent as well. on forms that aren't autofillable (right?). Therefore, much (most?) of > the data would come from forms that aren't relevant to Autofill. > O? I was not under the impression that we queried the AddressBook on non-autofillable forms. That sounds like a bug. If your statement is accurate, then you're right, my metrics will not be accurate. > > It sounds like you're asserting that these caveats don't matter. I'm > not sure -- it's entirely possible that users who frequently use > AddressBook integration also fill out autofillable forms more > frequently, whereas other users don't... but still encounter other forms > frequently. > > If you think that measuring what we actually show in the popup vs. what > we would show is too hard, then we can proceed with this current metric. > But if we do so, I think we should look carefully at the absolute > numbers, as well as the ratio. If line 156 is hit by a large number of > users in a day, then I'd be worried about that result, even if line 159 > is hit by a much larger number of users. > I agree. If line 156 is hit a lot, we're significantly decreasing the utility of AddressBook integration. That being said, the more its being hit, then more I want to get this CL landed. I have created a crbug, assigned to myself, to get accurate metrics in for AddressBook integration. https://code.google.com/p/chromium/issues/detail?id=385954. With that done, I am landing this CL, hopefully in time to get it in the beta. > > https://codereview.chromium.org/334653006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/334653006/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
isherman: Purely informative. No response expected. As we should have guessed, there is a test (AutofillCCInfobarDelegateTest.Metrics) that breaks the assumption that PersonalDataManager() != NULL. I've removed the DCHECK and added appropriate conditionals.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/334653006/290001
https://codereview.chromium.org/334653006/diff/290001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/290001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:23: // to outlive this object. The PersonalDataManager may be NULL in tests. Please add a TODO, either in this CL or in a follow-up, to fix the test code rather than bending production code to the whims of erroneous tests. ("LGTM" so as to not prevent the CQ from proceeding.)
https://codereview.chromium.org/334653006/diff/290001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/290001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:23: // to outlive this object. The PersonalDataManager may be NULL in tests. On 2014/06/18 23:10:04, Ilya Sherman wrote: > Please add a TODO, either in this CL or in a follow-up, to fix the test code > rather than bending production code to the whims of erroneous tests. ("LGTM" so > as to not prevent the CQ from proceeding.) File a bug against myself to fix this problem: https://code.google.com/p/chromium/issues/detail?id=386388&thanks=386388&ts=1...
On 2014/06/18 01:07:16, erikchen1 wrote: > O? I was not under the impression that we queried the AddressBook on > non-autofillable forms. That sounds like a bug. If your statement is > accurate, then you're right, my metrics will not be accurate. I think this is why we (used to) show the AddressBook prompt on the first form interaction, even when that form interaction seems completely unrelated to Autofill. I agree that this is probably a bug... though if we fix it, we actually increase the likelihood that the binary changes out from under us due to auto-update prior to the first attempt to access the AddressBook. It's kind of an awkward situation :/
https://codereview.chromium.org/334653006/diff/290001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client_mac.mm (right): https://codereview.chromium.org/334653006/diff/290001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client_mac.mm:23: // to outlive this object. The PersonalDataManager may be NULL in tests. On 2014/06/18 23:13:51, erikchen1 wrote: > On 2014/06/18 23:10:04, Ilya Sherman wrote: > > Please add a TODO, either in this CL or in a follow-up, to fix the test code > > rather than bending production code to the whims of erroneous tests. ("LGTM" > so > > as to not prevent the CQ from proceeding.) > > File a bug against myself to fix this problem: > https://code.google.com/p/chromium/issues/detail?id=386388&thanks=386388&ts=1... Thanks :)
Message was sent while issue was closed.
Change committed as 278254 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
