Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15942004:
Stop offering Autocheckout bubble on the forms of no interest. (Closed)
|
Created:
7 years, 7 months ago by Raman Kakilate Modified:
7 years, 6 months ago CC:
chromium-reviews, Raman Kakilate, benquan, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionStop offering Autocheckout bubble on the forms of no interest to Autocheckout.
BUG=243522
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203923
Patch Set 1 #Patch Set 2 : Unittest #
Total comments: 8
Patch Set 3 : Mores tests and nits. #
Total comments: 6
Patch Set 4 : move HasServerSpecifiedFieldTypes to autofill_manager #
Total comments: 12
Patch Set 5 : fix more comments. #
Total comments: 8
Patch Set 6 : Even more fixes. #
Total comments: 4
Patch Set 7 : . #
Messages
Total messages: 19 (0 generated)
Alex/Ben - PTAL. Please let me know if you see issues with the approach. Will try to add more tests before complete review.
Evan/Ilya - PTAL. I tried to write unittest for autofill_manager changes, but didn't know a quick good way to write. Any pointers will be useful.
A few nits but otherwise lgtm. Could you also explain the changes in autofill_agent in the cl description? Thanks. https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/autofill_manager.cc:862: // For Autocheckout, if form structure doesn't have field types specified then "the form structure doesn't have field types specified by the Autofill server" https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/autofill_manager.cc:864: // because there is no crowd-sourced data for Autocheckout experiement. "the Autocheckout experiment." https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... File components/autofill/browser/form_structure_unittest.cc (right): https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/form_structure_unittest.cc:2405: ASSERT_FALSE(form_structure->HasServerSpecifiedFieldTypes()); EXPECT https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/form_structure_unittest.cc:2407: ASSERT_TRUE(form_structure->HasServerSpecifiedFieldTypes()); ditto
TestAutofillManager (in autofill_manager_unittest.cc) has some functions AddSeenForm and ClearFormStructures which seem like they'd be useful in allowing you to test the autofill manager changes.
Added tests for autofill manager changes as well. PTAL. https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/autofill_manager.cc:862: // For Autocheckout, if form structure doesn't have field types specified then On 2013/05/24 17:38:04, ahutter wrote: > "the form structure doesn't have field types specified by the Autofill server" Simplified comment. https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/autofill_manager.cc:864: // because there is no crowd-sourced data for Autocheckout experiement. On 2013/05/24 17:38:04, ahutter wrote: > "the Autocheckout experiment." Done. https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... File components/autofill/browser/form_structure_unittest.cc (right): https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/form_structure_unittest.cc:2405: ASSERT_FALSE(form_structure->HasServerSpecifiedFieldTypes()); On 2013/05/24 17:38:04, ahutter wrote: > EXPECT Done. https://codereview.chromium.org/15942004/diff/2001/components/autofill/browse... components/autofill/browser/form_structure_unittest.cc:2407: ASSERT_TRUE(form_structure->HasServerSpecifiedFieldTypes()); On 2013/05/24 17:38:04, ahutter wrote: > ditto Done.
https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... File components/autofill/browser/form_structure.h (right): https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... components/autofill/browser/form_structure.h:144: bool HasServerSpecifiedFieldTypes() const; This doesn't seem like it needs to be part of the FormStructure class; it can be implemented by the one and only client that's interested in answering this question. https://codereview.chromium.org/15942004/diff/11001/components/autofill/rende... File components/autofill/renderer/autofill_agent.cc (left): https://codereview.chromium.org/15942004/diff/11001/components/autofill/rende... components/autofill/renderer/autofill_agent.cc:328: try_to_show_autocheckout_bubble_ = false; Why is this no longer needed?
https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... File components/autofill/browser/form_structure.h (right): https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... components/autofill/browser/form_structure.h:144: bool HasServerSpecifiedFieldTypes() const; On 2013/05/29 22:45:45, Ilya Sherman wrote: > This doesn't seem like it needs to be part of the FormStructure class; it can be > implemented by the one and only client that's interested in answering this > question. I would argue otherwise, its a representation of server specified state of the form structure and doesn't depend on any external objects/variables. I will move it to the client if that makes it easy for you to maintain this code. Please let me know. https://codereview.chromium.org/15942004/diff/11001/components/autofill/rende... File components/autofill/renderer/autofill_agent.cc (left): https://codereview.chromium.org/15942004/diff/11001/components/autofill/rende... components/autofill/renderer/autofill_agent.cc:328: try_to_show_autocheckout_bubble_ = false; On 2013/05/29 22:45:45, Ilya Sherman wrote: > Why is this no longer needed? This was redundant as browser's AutocheckoutManager already manages it. Having this state in renderer is causing a issue where in user clicks on a wrong form (no bubble offered) and renderer's "try_to_show_autocheckout_bubble_" turns to false.
https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... File components/autofill/browser/form_structure.h (right): https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... components/autofill/browser/form_structure.h:144: bool HasServerSpecifiedFieldTypes() const; On 2013/05/30 01:09:51, Raman Kakilate wrote: > On 2013/05/29 22:45:45, Ilya Sherman wrote: > > This doesn't seem like it needs to be part of the FormStructure class; it can > be > > implemented by the one and only client that's interested in answering this > > question. > > I would argue otherwise, its a representation of server specified state of the > form structure and doesn't depend on any external objects/variables. > > I will move it to the client if that makes it easy for you to maintain this > code. Please let me know. In general, you should always prefer to minimize the visibility of code, both to reduce API bloat and to speed up compilation. That applies here as well: There's only one client of this code, and that client can implement the logic with the pre-existing public API, so there's no need to include it as part of the public API for the class.
https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... File components/autofill/browser/form_structure.h (right): https://codereview.chromium.org/15942004/diff/11001/components/autofill/brows... components/autofill/browser/form_structure.h:144: bool HasServerSpecifiedFieldTypes() const; On 2013/05/31 06:51:11, Ilya Sherman wrote: > On 2013/05/30 01:09:51, Raman Kakilate wrote: > > On 2013/05/29 22:45:45, Ilya Sherman wrote: > > > This doesn't seem like it needs to be part of the FormStructure class; it > can > > be > > > implemented by the one and only client that's interested in answering this > > > question. > > > > I would argue otherwise, its a representation of server specified state of the > > form structure and doesn't depend on any external objects/variables. > > > > I will move it to the client if that makes it easy for you to maintain this > > code. Please let me know. > > In general, you should always prefer to minimize the visibility of code, both to > reduce API bloat and to speed up compilation. That applies here as well: > There's only one client of this code, and that client can implement the logic > with the pre-existing public API, so there's no need to include it as part of > the public API for the class. Done.
https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:876: return; Does Autocheckout handle *every* fillable form on the pages we're supporting? It seems like you should really be checking for Autocheckout-related data for the form, not just for server-provided field types. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager.h (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager.h:140: const gfx::RectF& bounding_box); Why did you increase the visibility of this method to be public? Seems like it should be at most protected, if you want to be able to access it from test code. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:610: void MarkAsFirstPageInAutocheckoutFlow() { nit: Docs https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:3199: explicit MockAutofillManagerDelegate() {} nit: No need for explicit -- that's only needed for one-arg constructors. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:3256: content::RunAllPendingInMessageLoop(BrowserThread::IO); Why is this needed? Ditto below.
https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:876: return; On 2013/05/31 20:58:04, Ilya Sherman wrote: > Does Autocheckout handle *every* fillable form on the pages we're supporting? > It seems like you should really be checking for Autocheckout-related data for > the form, not just for server-provided field types. Expanded the comment. As we will be asking field mappings for the form with "experiment" set to "autocheckout", returned server-provided field types are autocheckout related field types. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager.h (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager.h:140: const gfx::RectF& bounding_box); On 2013/05/31 20:58:04, Ilya Sherman wrote: > Why did you increase the visibility of this method to be public? Seems like it > should be at most protected, if you want to be able to access it from test code. Done. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:610: void MarkAsFirstPageInAutocheckoutFlow() { On 2013/05/31 20:58:04, Ilya Sherman wrote: > nit: Docs Done. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:3199: explicit MockAutofillManagerDelegate() {} On 2013/05/31 20:58:04, Ilya Sherman wrote: > nit: No need for explicit -- that's only needed for one-arg constructors. Done. https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:3256: content::RunAllPendingInMessageLoop(BrowserThread::IO); On 2013/05/31 20:58:04, Ilya Sherman wrote: > Why is this needed? Ditto below. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:876: return; On 2013/05/31 21:36:39, Raman Kakilate wrote: > On 2013/05/31 20:58:04, Ilya Sherman wrote: > > Does Autocheckout handle *every* fillable form on the pages we're supporting? > > It seems like you should really be checking for Autocheckout-related data for > > the form, not just for server-provided field types. > > Expanded the comment. > > As we will be asking field mappings for the form with "experiment" set to > "autocheckout", returned server-provided field types are autocheckout related > field types. Suppose I have a page with two forms: A checkout form, and a registration form. The server has Autocheckout data for the checkout form, and regular ol' crowdsourced data for the registration form. Will the server simply not return data for the second form when Autocheckout is enabled? If so, that seems like an error, since the form is unrelated to Autocheckout. If not, what's guaranteeing that we don't show the Autocheckout popup for the registration form? https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:882: } nit: Please update the relative location of this method to match its relative location in the header file. https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... File components/autofill/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:558: } nit: You can shorten this to "using AutofillManager::OnMaybeShowAutocheckoutBubble;" I think that means that you don't need to make the method virtual either, though I'm not 100% certain. Note that this comment might be moot after you address the comment lower down in this file. https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:560: nit: Spurious newline. https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:3265: content::RunAllPendingInMessageLoop(BrowserThread::IO); In that case, any reason not to just move this into the (test) implementation of OnMaybeShowAutocheckoutBubble()?
https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/18001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:876: return; On 2013/05/31 21:46:55, Ilya Sherman wrote: > On 2013/05/31 21:36:39, Raman Kakilate wrote: > > On 2013/05/31 20:58:04, Ilya Sherman wrote: > > > Does Autocheckout handle *every* fillable form on the pages we're > supporting? > > > It seems like you should really be checking for Autocheckout-related data > for > > > the form, not just for server-provided field types. > > > > Expanded the comment. > > > > As we will be asking field mappings for the form with "experiment" set to > > "autocheckout", returned server-provided field types are autocheckout related > > field types. > > Suppose I have a page with two forms: A checkout form, and a registration form. > The server has Autocheckout data for the checkout form, and regular ol' > crowdsourced data for the registration form. Will the server simply not return > data for the second form when Autocheckout is enabled? If so, that seems like > an error, since the form is unrelated to Autocheckout. If not, what's > guaranteeing that we don't show the Autocheckout popup for the registration > form? Yes, Server simply doesn't lookup for crowdsourced data. Autofill server request is marked with experiment "autocheckout", and server uses the experiment name to create keys to lookup. Which means, we will not be using crowdsourced data for forms not known to Autocheckout, local heuristics will come into play though (which is suboptimal). Agree with you that we need more finer grained control on Autofill server to merge from two result sets and annotate the response accordingly, but that is going to be tricky as Autofill response doesn't have a concept of "Form", instead it is just a sequence of "Fields". https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:882: } On 2013/05/31 21:46:55, Ilya Sherman wrote: > nit: Please update the relative location of this method to match its relative > location in the header file. Done for few, but not all. https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... File components/autofill/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:558: } On 2013/05/31 21:46:55, Ilya Sherman wrote: > nit: You can shorten this to "using > AutofillManager::OnMaybeShowAutocheckoutBubble;" I think that means that you > don't need to make the method virtual either, though I'm not 100% certain. > > Note that this comment might be moot after you address the comment lower down in > this file. thanks for the tip. but moved RunAllPendingInMessageLoop to here. https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:560: On 2013/05/31 21:46:55, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/15942004/diff/24001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:3265: content::RunAllPendingInMessageLoop(BrowserThread::IO); On 2013/05/31 21:46:55, Ilya Sherman wrote: > In that case, any reason not to just move this into the (test) implementation of > OnMaybeShowAutocheckoutBubble()? Nope. moved.
LGTM with nits, thanks. https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:943: } nit: Please don't move these methods around, even though they are in the wrong place, as they are unrelated to this CL. If you want to split this off to a separate CL, though, I'd be happy to rubber stamp that :) https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... File components/autofill/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:558: content::RunAllPendingInMessageLoop(BrowserThread::IO); nit: Please add a comment documenting why this is needed.
+jschuh@ for message changes. https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... components/autofill/browser/autofill_manager.cc:943: } On 2013/05/31 22:22:59, Ilya Sherman wrote: > nit: Please don't move these methods around, even though they are in the wrong > place, as they are unrelated to this CL. If you want to split this off to a > separate CL, though, I'd be happy to rubber stamp that :) Done. https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... File components/autofill/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/15942004/diff/29001/components/autofill/brows... components/autofill/browser/autofill_manager_unittest.cc:558: content::RunAllPendingInMessageLoop(BrowserThread::IO); On 2013/05/31 22:22:59, Ilya Sherman wrote: > nit: Please add a comment documenting why this is needed. Done.
+cdn as well for messages
IPC messages LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ramankk@chromium.org/15942004/36001
Message was sent while issue was closed.
Change committed as 203923 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
