Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130773004:
mac: Add histograms to measure impact of Address Book integration. (Closed)
|
Created:
5 years, 7 months ago by erikchen Modified:
5 years, 7 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, estade+watch_chromium.org, browser-components-watch_chromium.org, asvitkine+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmac: Add histograms to measure impact of Address Book integration.
BUG=488146
Committed: https://crrev.com/ef2d25eba7d872c749d89589403f0bc01b02e30b
Cr-Commit-Position: refs/heads/master@{#330651}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 12
Patch Set 4 : Comments from isherman, round two. #Patch Set 5 : Fix some unit tests. #
Total comments: 3
Patch Set 6 : Comments from isherman. Rebase against top of tree. #Patch Set 7 : Comments from isherman, round four. #
Total comments: 2
Patch Set 8 : Comments from isherman, round five. #
Messages
Total messages: 33 (9 generated)
erikchen@chromium.org changed reviewers: + isherman@chromium.org
isherman: Please review.
What's the plan for these histograms? Are we going to postpone the feature removal for a milestone while we gather data? Will these give us the data that we actually need to make the decision? I'd really prefer not to get stuck in a cycle of waiting for more and more data before we make a decision. https://codereview.chromium.org/1130773004/diff/20001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/1130773004/diff/20001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:182: UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBook.ContainedUsefulInformation", For parallelism with existing histograms, please use MacAddressBook rather than AddressBook (applies throughout) https://codereview.chromium.org/1130773004/diff/20001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:198: UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBook.ContainedUsefulInformation", I'd prefer not to repeat the string constant, to avoid the chance of typos. Please either define a wrapper function or at least a string constant. https://codereview.chromium.org/1130773004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1130773004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1818: + </summary> Do we really need the raw times, or can we just collapse this to a heuristic boolean histogram? https://codereview.chromium.org/1130773004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1826: + information was found. I think "useful" is a pretty strong term here. This histogram is really just measuring whether the "Me" card had anything in it. That's a very low bar...
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
isherman: PTAL https://codereview.chromium.org/1130773004/diff/20001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/1130773004/diff/20001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:182: UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBook.ContainedUsefulInformation", On 2015/05/14 21:46:28, Ilya Sherman wrote: > For parallelism with existing histograms, please use MacAddressBook rather than > AddressBook (applies throughout) Done. https://codereview.chromium.org/1130773004/diff/20001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:198: UMA_HISTOGRAM_BOOLEAN("Autofill.AddressBook.ContainedUsefulInformation", On 2015/05/14 21:46:28, Ilya Sherman wrote: > I'd prefer not to repeat the string constant, to avoid the chance of typos. > Please either define a wrapper function or at least a string constant. Done. https://codereview.chromium.org/1130773004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1130773004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1818: + </summary> On 2015/05/14 21:46:28, Ilya Sherman wrote: > Do we really need the raw times, or can we just collapse this to a heuristic > boolean histogram? Since I haven't seen the raw times, I don't know what heuristic to use yet. Hence the ~100ms. It might be 5ms it might be 500ms. https://codereview.chromium.org/1130773004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1826: + information was found. On 2015/05/14 21:46:28, Ilya Sherman wrote: > I think "useful" is a pretty strong term here. This histogram is really just > measuring whether the "Me" card had anything in it. That's a very low bar... This histogram got replaced by 5 separate histograms.
Thanks, Erik. https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:20: #include "base/metrics/histogram.h" nit: Please include histogram_macros.h instead. https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:714: UMA_HISTOGRAM_BOOLEAN("Autofill.MacAddressBook.AcceptedResultFromAddressBook", nit: I'd name this "AcceptedResultIsFromAddressBook", so it's clearer that this is comparing AB vs. non-AB, as opposed to accepted vs. didn't accept. Also, I'd probably replace "Result" with "Suggestion" or "Profile". https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:301: hasValidCompanyName = [companyName length] > 0; I don't think the company name has much to do with the metric we're adding. For example, my name is "Ilya Sherman", and my company name is "Google, Inc.". https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:367: return hasAnyAddressInformation; I think this is perhaps too lax to be useful. For example, I'd expect that it's fairly common to have a country set, and maybe even a state; that's still not too helpful for Autofill. I'd be most interested in knowing whether there is a "complete" address. As a short heuristic for that, I'd check to see whether there is a street address + city or zip. https://codereview.chromium.org/1130773004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1130773004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1997: + enum="Boolean"> Please use more customized labels, so that the dashboard viewer doesn't have to figure out what "True" vs. "False" means. https://codereview.chromium.org/1130773004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2014: +<histogram name="Autofill.MacAddressBook.ContainedMeCard" enum="Boolean"> (Applies here and below, too.)
Patchset #4 (id:100001) has been deleted
isherman: PTAL https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:20: #include "base/metrics/histogram.h" On 2015/05/15 21:14:58, Ilya Sherman wrote: > nit: Please include histogram_macros.h instead. Done. https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:714: UMA_HISTOGRAM_BOOLEAN("Autofill.MacAddressBook.AcceptedResultFromAddressBook", On 2015/05/15 21:14:58, Ilya Sherman wrote: > nit: I'd name this "AcceptedResultIsFromAddressBook", so it's clearer that this > is comparing AB vs. non-AB, as opposed to accepted vs. didn't accept. Also, I'd > probably replace "Result" with "Suggestion" or "Profile". Done. https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager_mac.mm (right): https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:301: hasValidCompanyName = [companyName length] > 0; On 2015/05/15 21:14:58, Ilya Sherman wrote: > I don't think the company name has much to do with the metric we're adding. For > example, my name is "Ilya Sherman", and my company name is "Google, Inc.". Done. https://codereview.chromium.org/1130773004/diff/80001/components/autofill/cor... components/autofill/core/browser/personal_data_manager_mac.mm:367: return hasAnyAddressInformation; On 2015/05/15 21:14:58, Ilya Sherman wrote: > I think this is perhaps too lax to be useful. For example, I'd expect that it's > fairly common to have a country set, and maybe even a state; that's still not > too helpful for Autofill. I'd be most interested in knowing whether there is a > "complete" address. As a short heuristic for that, I'd check to see whether > there is a street address + city or zip. Done. https://codereview.chromium.org/1130773004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1130773004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1997: + enum="Boolean"> On 2015/05/15 21:14:58, Ilya Sherman wrote: > Please use more customized labels, so that the dashboard viewer doesn't have to > figure out what "True" vs. "False" means. I think that that causes unnecessary duplication of information between the name of the histogram and the label of the boolean but *shrug*, you're the owner. Done. https://codereview.chromium.org/1130773004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2014: +<histogram name="Autofill.MacAddressBook.ContainedMeCard" enum="Boolean"> On 2015/05/15 21:14:58, Ilya Sherman wrote: > (Applies here and below, too.) Done.
LGTM. Thanks, Erik.
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130773004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
isherman: PTAL
https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:708: bool result = GetProfile(unique_id, &profile, &variant); Rather than changing the test code, I think the production code should actually bail if the unique_id is negative. Does that resolve the test failures?
https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:708: bool result = GetProfile(unique_id, &profile, &variant); On 2015/05/18 21:36:22, Ilya Sherman wrote: > Rather than changing the test code, I think the production code should actually > bail if the unique_id is negative. Does that resolve the test failures? It does not resolve all of the test failures. I also don't think that it's the best approach - unique_id is effectively an opaque object, and there are methods that operate on it to return valid data. Comparing it against 0 makes assumptions about the internal structure of unique_id.
https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:708: bool result = GetProfile(unique_id, &profile, &variant); On 2015/05/18 23:29:40, erikchen wrote: > On 2015/05/18 21:36:22, Ilya Sherman wrote: > > Rather than changing the test code, I think the production code should > actually > > bail if the unique_id is negative. Does that resolve the test failures? > > It does not resolve all of the test failures. > > I also don't think that it's the best approach - unique_id is effectively an > opaque object, and there are methods that operate on it to return valid data. > Comparing it against 0 makes assumptions about the internal structure of > unique_id. It's not an opaque object. What does your code do in production if an id of -1 is passed to this method? What test failures remain if you compare against 0?
On 2015/05/18 23:31:24, Ilya Sherman wrote: > https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... > components/autofill/core/browser/autofill_manager.cc:708: bool result = > GetProfile(unique_id, &profile, &variant); > On 2015/05/18 23:29:40, erikchen wrote: > > On 2015/05/18 21:36:22, Ilya Sherman wrote: > > > Rather than changing the test code, I think the production code should > > actually > > > bail if the unique_id is negative. Does that resolve the test failures? > > > > It does not resolve all of the test failures. > > > > I also don't think that it's the best approach - unique_id is effectively an > > opaque object, and there are methods that operate on it to return valid data. > > Comparing it against 0 makes assumptions about the internal structure of > > unique_id. > > It's not an opaque object. What does your code do in production if an id of -1 > is passed to this method? > > What test failures remain if you compare against 0? unique_id is an opaque object. It consists of two shorts that have been packed into a single int. (it's type should be uint32_t, but oh well). Checking to see if it's negative doesn't make any sense. Is it 2's complement? 1's complement? There is a bug in patch set 5 of the CL, which is that autofill_external_delegate.cc will pass a value between 0 to -9 to autofill_manager.cc. It looks like 'identifier' is either a value from 0 to -9 (enum PopupItemId), or it's two shorts packed into a single 32-bit integer. But how does it distinguish between -3 and two packed numbers? Looking at the packing logic, it looks like one of the two shorts will always be 0. So assuming the machine is using 2's complement representation of signed integers, an 'identifier' of -1 through -9 is guaranteed to not be a "packed" number. But that's not very clear at all. :( Here's my suggestion: The implementation of DidAcceptSuggestion calls 'manager_->OnUserDidAcceptAutofillSuggestion(identifier)' if and only if 'identifier' is not an identifier defined in enum PopupItemId. Does that sound reasonable to you? -------------------- The test error: GMOCK WARNING: Uninteresting mock function call - returning default value. Function call: ShouldShowScanCreditCard(@0x7fc8f2a03880 0 1 Fields:, @0x7fc8f2a03968 0 false false false true true 1 0) Returns: false NOTE: You can safely ignore the above warning unless this call should not happen. Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call. See http://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect for details. Stack trace: [63761:1287:0518/163914:1549516405423703:FATAL:autofill_manager.cc(1475)] Check failed: false. 0 libbase.dylib 0x000000010f78d6af base::debug::StackTrace::StackTrace() + 47 1 libbase.dylib 0x000000010f78d703 base::debug::StackTrace::StackTrace() + 35 2 libbase.dylib 0x000000010f7ef313 logging::LogMessage::~LogMessage() + 67 3 libbase.dylib 0x000000010f7ee3a3 logging::LogMessage::~LogMessage() + 35 4 components_unittests 0x0000000105fbf3d1 autofill::AutofillManager::IntToBackendID(int) const + 289 5 components_unittests 0x0000000105fbe8b8 autofill::AutofillManager::SplitFrontendID(int, autofill::SuggestionBackendID*, autofill::SuggestionBackendID*) const + 200 6 components_unittests 0x0000000105fbc2f6 autofill::AutofillManager::GetProfile(int, autofill::AutofillProfile const**, unsigned long*) + 86 7 components_unittests 0x0000000105fbc7c1 autofill::AutofillManager::OnUserDidAcceptAutofillSuggestion(int) + 65 8 components_unittests 0x0000000105fb0ab5 autofill::AutofillExternalDelegate::DidAcceptSuggestion(std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> > const&, int) + 101 9 components_unittests 0x0000000104808006 autofill::AutofillExternalDelegateUnitTest_TestExternalDelegateVirtualCalls_Test::TestBody() + 870 10 components_unittests 0x0000000105d7b638 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 136 11 components_unittests 0x0000000105d6a72c void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 124 12 components_unittests 0x0000000105d5d7c3 testing::Test::Run() + 211 13 components_unittests 0x0000000105d5e136 testing::TestInfo::Run() + 230 14 components_unittests 0x0000000105d5e925 testing::TestCase::Run() + 245 15 components_unittests 0x0000000105d6494d testing::internal::UnitTestImpl::RunAllTests() + 733 16 components_unittests 0x0000000105d7f2a8 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 136 17 components_unittests 0x0000000105d6cd2f bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) + 127 18 components_unittests 0x0000000105d6461d testing::UnitTest::Run() + 205 19 components_unittests 0x0000000105c5cee3 RUN_ALL_TESTS() + 35 20 components_unittests 0x0000000105c5b998 base::TestSuite::Run() + 200 21 components_unittests 0x000000010471b2f7 base::internal::RunnableAdapter<int (base::TestSuite::*)()>::Run(base::TestSuite*) + 119 22 components_unittests 0x0000000104719dcd base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int (base::TestSuite::*)()>, base::internal::TypeList<(anonymous namespace)::ComponentsTestSuite*> >::MakeItSo(base::internal::RunnableAdapter<int (base::TestSuite::*)()>, (anonymous namespace)::ComponentsTestSuite*) + 61 23 components_unittests 0x0000000104719d66 base::internal::Invoker<IndexSequence<0ul>, base::internal::BindState<base::internal::RunnableAdapter<int (base::TestSuite::*)()>, int (base::TestSuite*), base::internal::TypeList<base::internal::UnretainedWrapper<(anonymous namespace)::ComponentsTestSuite> > >, base::internal::TypeList<base::internal::UnwrapTraits<base::internal::UnretainedWrapper<(anonymous namespace)::ComponentsTestSuite> > >, base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int (base::TestSuite::*)()>, base::internal::TypeList<(anonymous namespace)::ComponentsTestSuite*> >, int ()>::Run(base::internal::BindStateBase*) + 102 24 components_unittests 0x0000000105c4929f base::Callback<int ()>::Run() const + 63 25 components_unittests 0x0000000105c44e55 base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int ()> const&, int, bool, base::Callback<void ()> const&) + 341 26 components_unittests 0x0000000105c44cc5 base::LaunchUnitTests(int, char**, base::Callback<int ()> const&) + 117 27 components_unittests 0x0000000104718f69 main + 281 28 components_unittests 0x0000000104718e44 start + 52
On 2015/05/18 23:56:56, erikchen wrote: > On 2015/05/18 23:31:24, Ilya Sherman wrote: > > > https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... > > File components/autofill/core/browser/autofill_manager.cc (right): > > > > > https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... > > components/autofill/core/browser/autofill_manager.cc:708: bool result = > > GetProfile(unique_id, &profile, &variant); > > On 2015/05/18 23:29:40, erikchen wrote: > > > On 2015/05/18 21:36:22, Ilya Sherman wrote: > > > > Rather than changing the test code, I think the production code should > > > actually > > > > bail if the unique_id is negative. Does that resolve the test failures? > > > > > > It does not resolve all of the test failures. > > > > > > I also don't think that it's the best approach - unique_id is effectively an > > > opaque object, and there are methods that operate on it to return valid > data. > > > Comparing it against 0 makes assumptions about the internal structure of > > > unique_id. > > > > It's not an opaque object. What does your code do in production if an id of > -1 > > is passed to this method? > > > > What test failures remain if you compare against 0? > > unique_id is an opaque object. It consists of two shorts that have been packed > into a single int. (it's type should be uint32_t, but oh well). Checking to see > if it's negative doesn't make any sense. Is it 2's complement? 1's complement? > > There is a bug in patch set 5 of the CL, which is that > autofill_external_delegate.cc will pass a value between 0 to -9 to > autofill_manager.cc. It looks like 'identifier' is either a value from 0 to -9 > (enum PopupItemId), or it's two shorts packed into a single 32-bit integer. But > how does it distinguish between -3 and two packed numbers? Looking at the > packing logic, it looks like one of the two shorts will always be 0. So assuming > the machine is using 2's complement representation of signed integers, an > 'identifier' of -1 through -9 is guaranteed to not be a "packed" number. But > that's not very clear at all. :( Yeah, the identifier is essentially a union of an enum and this opaque type that you're describing. The enum covers the range 0 through int_min. The packed int pair covers all the positive range. I agree that it's kind of a funky setup and not very intuitive, but it works out really well in practice. I think that, technically, the code has a bug in it, because it doesn't actually assert that the packed int pair fits in the positive range; however, in practice, the user would need to have an inordinate number of credit cards stored in order for this to matter. In any case, probably worth fixing, but not practically relevant, I think. > Here's my suggestion: The implementation of DidAcceptSuggestion calls > 'manager_->OnUserDidAcceptAutofillSuggestion(identifier)' if and only if > 'identifier' is not an identifier defined in enum PopupItemId. Does that sound > reasonable to you? Gating in either the delegate or the manager sounds fine. Please test "unique_id > 0" rather than trying to enumerate all of the enum members, though.
On 2015/05/19 00:28:29, Ilya Sherman wrote: > On 2015/05/18 23:56:56, erikchen wrote: > > On 2015/05/18 23:31:24, Ilya Sherman wrote: > > > > > > https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... > > > File components/autofill/core/browser/autofill_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/1130773004/diff/140001/components/autofill/co... > > > components/autofill/core/browser/autofill_manager.cc:708: bool result = > > > GetProfile(unique_id, &profile, &variant); > > > On 2015/05/18 23:29:40, erikchen wrote: > > > > On 2015/05/18 21:36:22, Ilya Sherman wrote: > > > > > Rather than changing the test code, I think the production code should > > > > actually > > > > > bail if the unique_id is negative. Does that resolve the test failures? > > > > > > > > It does not resolve all of the test failures. > > > > > > > > I also don't think that it's the best approach - unique_id is effectively > an > > > > opaque object, and there are methods that operate on it to return valid > > data. > > > > Comparing it against 0 makes assumptions about the internal structure of > > > > unique_id. > > > > > > It's not an opaque object. What does your code do in production if an id of > > -1 > > > is passed to this method? > > > > > > What test failures remain if you compare against 0? > > > > unique_id is an opaque object. It consists of two shorts that have been packed > > into a single int. (it's type should be uint32_t, but oh well). Checking to > see > > if it's negative doesn't make any sense. Is it 2's complement? 1's complement? > > > > > There is a bug in patch set 5 of the CL, which is that > > autofill_external_delegate.cc will pass a value between 0 to -9 to > > autofill_manager.cc. It looks like 'identifier' is either a value from 0 to -9 > > (enum PopupItemId), or it's two shorts packed into a single 32-bit integer. > But > > how does it distinguish between -3 and two packed numbers? Looking at the > > packing logic, it looks like one of the two shorts will always be 0. So > assuming > > the machine is using 2's complement representation of signed integers, an > > 'identifier' of -1 through -9 is guaranteed to not be a "packed" number. But > > that's not very clear at all. :( > > Yeah, the identifier is essentially a union of an enum and this opaque type that > you're describing. The enum covers the range 0 through int_min. The packed int > pair covers all the positive range. I agree that it's kind of a funky setup and > not very intuitive, but it works out really well in practice. I think that, > technically, the code has a bug in it, because it doesn't actually assert that > the packed int pair fits in the positive range; however, in practice, the user > would need to have an inordinate number of credit cards stored in order for this > to matter. In any case, probably worth fixing, but not practically relevant, I > think. > > > Here's my suggestion: The implementation of DidAcceptSuggestion calls > > 'manager_->OnUserDidAcceptAutofillSuggestion(identifier)' if and only if > > 'identifier' is not an identifier defined in enum PopupItemId. Does that sound > > reasonable to you? > > Gating in either the delegate or the manager sounds fine. Please test > "unique_id > 0" rather than trying to enumerate all of the enum members, though. isherman: PTAL (Given your lack of response on the subject of the unit test, I'm guessing you're okay with the override of the metric to do nothing?)
On 2015/05/19 00:43:51, erikchen wrote: > (Given your lack of response on the subject of the unit test, I'm guessing > you're okay with the override of the metric to do nothing?) Sorry, it wasn't clear to me what test(s) fail(s) with the guard applied. Could you please clarify that? Is it still the same set of tests?
On 2015/05/19 00:58:19, Ilya Sherman wrote: > On 2015/05/19 00:43:51, erikchen wrote: > > (Given your lack of response on the subject of the unit test, I'm guessing > > you're okay with the override of the metric to do nothing?) > > Sorry, it wasn't clear to me what test(s) fail(s) with the guard applied. Could > you please clarify that? Is it still the same set of tests? There is exactly 1 test that fails: TestExternalDelegateVirtualCalls. It's not surprising that it fails, since it calls DidAcceptSuggestion with an identifier of '1', which the autofill_manager knows nothing about.
On 2015/05/19 01:04:19, erikchen wrote: > On 2015/05/19 00:58:19, Ilya Sherman wrote: > > On 2015/05/19 00:43:51, erikchen wrote: > > > (Given your lack of response on the subject of the unit test, I'm guessing > > > you're okay with the override of the metric to do nothing?) > > > > Sorry, it wasn't clear to me what test(s) fail(s) with the guard applied. > Could > > you please clarify that? Is it still the same set of tests? > > There is exactly 1 test that fails: TestExternalDelegateVirtualCalls. It's not > surprising that it fails, since it calls DidAcceptSuggestion with an identifier > of '1', which the autofill_manager knows nothing about. Okay, fair enough. Looking at this more closely, I'd prefer that you log the new metrics from AutofillManager::FillOrPreviewForm(), rather than adding a new method to the AutofillManager API. This will also allow you to avoid worrying about changing the tests -- that method call is already mocked for this test.
Patchset #7 (id:180001) has been deleted
On 2015/05/19 02:18:53, Ilya Sherman wrote: > On 2015/05/19 01:04:19, erikchen wrote: > > On 2015/05/19 00:58:19, Ilya Sherman wrote: > > > On 2015/05/19 00:43:51, erikchen wrote: > > > > (Given your lack of response on the subject of the unit test, I'm guessing > > > > you're okay with the override of the metric to do nothing?) > > > > > > Sorry, it wasn't clear to me what test(s) fail(s) with the guard applied. > > Could > > > you please clarify that? Is it still the same set of tests? > > > > There is exactly 1 test that fails: TestExternalDelegateVirtualCalls. It's not > > surprising that it fails, since it calls DidAcceptSuggestion with an > identifier > > of '1', which the autofill_manager knows nothing about. > > Okay, fair enough. Looking at this more closely, I'd prefer that you log the > new metrics from AutofillManager::FillOrPreviewForm(), rather than adding a new > method to the AutofillManager API. This will also allow you to avoid worrying > about changing the tests -- that method call is already mocked for this test. sure, done. isherman: PTAL
LGTM, thanks. https://codereview.chromium.org/1130773004/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/1130773004/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_manager.h:360: // Emits a UMA metric indicating whether the accepting Autofill suggestion is nit: "accepting" -> "accepted"
https://codereview.chromium.org/1130773004/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/1130773004/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_manager.h:360: // Emits a UMA metric indicating whether the accepting Autofill suggestion is On 2015/05/19 21:53:43, Ilya Sherman wrote: > nit: "accepting" -> "accepted" Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1130773004/#ps220001 (title: "Comments from isherman, round five.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130773004/220001
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ef2d25eba7d872c749d89589403f0bc01b02e30b Cr-Commit-Position: refs/heads/master@{#330651} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
