Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
563313004:
[Password Manager] Unfriend PasswordAutofillAgentTest from PasswordAutofillAgent (clean-up). (Closed)
|
Created:
6 years, 3 months ago by Pritam Nikam Modified:
6 years, 3 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, jam, darin-cc_chromium.org, Dane Wallinga, dyu1, rouslan+autofillwatch_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Password Manager] Unfriend PasswordAutofillAgentTest from PasswordAutofillAgent (clean-up).
BUG=368160
TBR=phajdan.jr@chromium.org
Committed: https://crrev.com/e5f069d57b9de5da6d710b3313b071b92e811fcb
Cr-Commit-Position: refs/heads/master@{#296071}
Patch Set 1 : Unfriend PasswordAutofillAgentTest from PasswordAutofillAgent. #Patch Set 2 : Unfriend PasswordAutofillAgentTest and RequestAutocompleteRendererTest from AutofillAgent. #
Total comments: 16
Patch Set 3 : Incorporated review comments. #
Total comments: 19
Patch Set 4 : Incorporated review inputs. #Patch Set 5 : Incorporated review comments. #
Messages
Total messages: 26 (6 generated)
pritam.nikam@samsung.com changed reviewers: + vabr@chromium.org
On 2014/09/14 11:03:45, Pritam Nikam wrote: > mailto:pritam.nikam@samsung.com changed reviewers: > + mailto:vabr@chromium.org Hi Vaclav, PTAL, Thanks!
pritam.nikam@samsung.com changed reviewers: + estade@chromium.org, isherman@chromium.org
PTAL Thanks!
Thanks, Pritam Nikam. Please have a look at the comments. Cheers, Vaclav https://codereview.chromium.org/563313004/diff/20001/chrome/renderer/autofill... File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right): https://codereview.chromium.org/563313004/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/password_generation_agent_browsertest.cc:203: AutofillAgent* autofill_agent = (AutofillAgent*)autofill_agent_; Don't use C-style cast, use one of C++ casts. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting) Here the appropriate one seems to be static_cast, except I don't understand why do you need the change to 2-step cast at all -- does the old code not work? Also below. https://codereview.chromium.org/563313004/diff/20001/chrome/test/base/chrome_... File chrome/test/base/chrome_render_view_test.cc (right): https://codereview.chromium.org/563313004/diff/20001/chrome/test/base/chrome_... chrome/test/base/chrome_render_view_test.cc:15: #include "components/autofill/content/renderer/autofill_agent.h" Do you still need this #include, when you add the test version below? https://codereview.chromium.org/563313004/diff/20001/chrome/test/base/chrome_... chrome/test/base/chrome_render_view_test.cc:42: using autofill::TestAutofillAgent; nit: alphabetise https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:60: virtual void FormControlElementClicked( No need to expose this, it is already public through PageClickListener. https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:65: virtual void textFieldDidEndEditing(const blink::WebInputElement& element); Also the WebAutofillClient methods are already public through WebAutofillClient, so no need to expose those. https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:80: virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; Why do you need to expose these two? They are already public through RenderViewObserver. https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/test_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/test_autofill_agent.h:40: virtual void OnFillPasswordSuggestion( Instead of re-defining the methods and calling the parent, use 'using'. Here it would be just: using AutofillAgent::OnFillPasswordSuggestion; The same holds for other methods declared here and in test_password_autofill_agent.h.
https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/test_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/test_autofill_agent.h:40: virtual void OnFillPasswordSuggestion( On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > Instead of re-defining the methods and calling the parent, use 'using'. Here it > would be just: > > using AutofillAgent::OnFillPasswordSuggestion; > are you suggesting to use "using ..." in the header file? If yes, please, don't.
On 2014/09/15 16:23:36, tfarina wrote: > https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... > File components/autofill/content/renderer/test_autofill_agent.h (right): > > https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... > components/autofill/content/renderer/test_autofill_agent.h:40: virtual void > OnFillPasswordSuggestion( > On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > > Instead of re-defining the methods and calling the parent, use 'using'. Here > it > > would be just: > > > > using AutofillAgent::OnFillPasswordSuggestion; > > > are you suggesting to use "using ..." in the header file? If yes, please, don't. Thiago, this is a different form of using than what you're thinking of. It is not a namespace alias that pulls namespaced symbols into the global namespace. Rather, it is a clearer and more concise way of increasing the visibility of a protected method in a (test-only) subclass. This use of "using" is appropriate even in a header file.
Thanks Vaclav, Ilya and Thiago for review. I've incorporated review comments in patch set #3. Please have a look. Thanks! https://codereview.chromium.org/563313004/diff/20001/chrome/renderer/autofill... File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right): https://codereview.chromium.org/563313004/diff/20001/chrome/renderer/autofill... chrome/renderer/autofill/password_generation_agent_browsertest.cc:203: AutofillAgent* autofill_agent = (AutofillAgent*)autofill_agent_; On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > Don't use C-style cast, use one of C++ casts. > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting) > Here the appropriate one seems to be static_cast, except I don't understand why > do you need the change to 2-step cast at all -- does the old code not work? > > Also below. Done. I've missed including header earlier. https://codereview.chromium.org/563313004/diff/20001/chrome/test/base/chrome_... File chrome/test/base/chrome_render_view_test.cc (right): https://codereview.chromium.org/563313004/diff/20001/chrome/test/base/chrome_... chrome/test/base/chrome_render_view_test.cc:15: #include "components/autofill/content/renderer/autofill_agent.h" On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > Do you still need this #include, when you add the test version below? Done. https://codereview.chromium.org/563313004/diff/20001/chrome/test/base/chrome_... chrome/test/base/chrome_render_view_test.cc:42: using autofill::TestAutofillAgent; On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > nit: alphabetise Done. https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:60: virtual void FormControlElementClicked( On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > No need to expose this, it is already public through PageClickListener. AutofillAgent overrides this function. As per current implementation it's kept under private: accessor. I've made it protected: so that only derived class - TestAutofillAgent can have it's accessibility. Hope I didn't misinterpret your comment. https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:65: virtual void textFieldDidEndEditing(const blink::WebInputElement& element); On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > Also the WebAutofillClient methods are already public through WebAutofillClient, > so no need to expose those. Ditto. https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:80: virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > Why do you need to expose these two? They are already public through > RenderViewObserver. Ditto. (From previous comment) https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/test_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/test_autofill_agent.h:40: virtual void OnFillPasswordSuggestion( On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > Instead of re-defining the methods and calling the parent, use 'using'. Here it > would be just: > > using AutofillAgent::OnFillPasswordSuggestion; > > The same holds for other methods declared here and in > test_password_autofill_agent.h. Done.
Pritam Nikam: LGTM, but please address the comment about accessing some methods through upcasting instead of by including them in the TestAutofillAgent. Thiago: Ilya's response hopefully cleared your concerns about "using". What I proposed can be seen on a couple of places in Chromium, e.g., in https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/.... If you have further concerns, please let me know. Cheers, Vaclav https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/20001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:60: virtual void FormControlElementClicked( On 2014/09/16 07:54:20, Pritam Nikam wrote: > On 2014/09/15 14:26:59, vabr (OOO until 16 Sep) wrote: > > No need to expose this, it is already public through PageClickListener. > > AutofillAgent overrides this function. As per current implementation it's kept > under private: accessor. > I've made it protected: so that only derived class - TestAutofillAgent can have > it's accessibility. Hope I didn't misinterpret your comment. It should not matter how AutofillAgent changes the visibility -- you can always upcast the pointer to (in this case) PageClickListener, and call the method. Because the method is virtual, it will be the AutofillAgent's method, but with PageClickListener's visibility. The reason for doing so is to keep the amount of changes, and the size of TestAutofillAgent, and the need for future updates to the test class at minimum. If it is still unclear, please let me know.
https://codereview.chromium.org/563313004/diff/40001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/563313004/diff/40001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:878: '../components/autofill/content/renderer/test_autofill_agent.cc', nit: Please alphabetize. https://codereview.chromium.org/563313004/diff/40001/chrome/test/base/chrome_... File chrome/test/base/chrome_render_view_test.h (right): https://codereview.chromium.org/563313004/diff/40001/chrome/test/base/chrome_... chrome/test/base/chrome_render_view_test.h:40: autofill::TestPasswordAutofillAgent* password_autofill_; While you're here, please rename this to "password_autofill_agent_". https://codereview.chromium.org/563313004/diff/40001/chrome/test/data/passwor... File chrome/test/data/password/form_with_only_password_field.html (right): https://codereview.chromium.org/563313004/diff/40001/chrome/test/data/passwor... chrome/test/data/password/form_with_only_password_field.html:11: </html> Where is this file used? https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:76: const base::string16& password); Please modify the test to simulate sending an IPC message, rather than exposing this private method. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:83: const FormData& form_data); Ditto. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:100: // blink::WebAutofillClient: If you move some of the methods in this group to have protected visibility, please move all of them. Ditto below. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:77: virtual void SendPasswordForms(blink::WebFrame* frame, bool only_visible); This is correctly a private method. Please instead fix the tests to call methods that are part of the public API for this class. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:143: // RenderViewObserver: Here, too, please increase the visibility of all of the RenderViewObserver methods, or none of them.
Thanks Vaclav & Ilya for review inputs. I've incorporated inputs and submitted patch set #4 for review, PTAL. Thanks! https://codereview.chromium.org/563313004/diff/40001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/563313004/diff/40001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:878: '../components/autofill/content/renderer/test_autofill_agent.cc', On 2014/09/18 18:45:52, Ilya Sherman wrote: > nit: Please alphabetize. Done. Removed test_autofill_agent.h/cc as not required. https://codereview.chromium.org/563313004/diff/40001/chrome/test/base/chrome_... File chrome/test/base/chrome_render_view_test.h (right): https://codereview.chromium.org/563313004/diff/40001/chrome/test/base/chrome_... chrome/test/base/chrome_render_view_test.h:40: autofill::TestPasswordAutofillAgent* password_autofill_; On 2014/09/18 18:45:52, Ilya Sherman wrote: > While you're here, please rename this to "password_autofill_agent_". Done. https://codereview.chromium.org/563313004/diff/40001/chrome/test/data/passwor... File chrome/test/data/password/form_with_only_password_field.html (right): https://codereview.chromium.org/563313004/diff/40001/chrome/test/data/passwor... chrome/test/data/password/form_with_only_password_field.html:11: </html> On 2014/09/18 18:45:52, Ilya Sherman wrote: > Where is this file used? Done. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:76: const base::string16& password); On 2014/09/18 18:45:53, Ilya Sherman wrote: > Please modify the test to simulate sending an IPC message, rather than exposing > this private method. Done. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:83: const FormData& form_data); On 2014/09/18 18:45:52, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.h:100: // blink::WebAutofillClient: On 2014/09/18 18:45:52, Ilya Sherman wrote: > If you move some of the methods in this group to have protected visibility, > please move all of them. Ditto below. Done. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:77: virtual void SendPasswordForms(blink::WebFrame* frame, bool only_visible); On 2014/09/18 18:45:53, Ilya Sherman wrote: > This is correctly a private method. Please instead fix the tests to call > methods that are part of the public API for this class. Done. https://codereview.chromium.org/563313004/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:143: // RenderViewObserver: On 2014/09/18 18:45:53, Ilya Sherman wrote: > Here, too, please increase the visibility of all of the RenderViewObserver > methods, or none of them. Done.
vabr@chromium.org changed reviewers: + phajdan.jr@chromium.org
LGTM, Thanks, Pritam Nikam, for this clean-up. phajdan.jr: could you please rubber-stamp changes in chrome/test/base/chrome_render_view_test.* ? Cheers, Vaclav
LGTM as well. Thanks! https://codereview.chromium.org/563313004/diff/40001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/563313004/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:306: autofill_agent_->FormControlElementClicked(username_input, false); Hmm, are you sure that this code is no longer needed? I guess we'll see whether the tests pass without it.
TBR Paweł for trivial change to //chrome/test/base (renaming a variable).
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/563313004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/563313004/diff/40001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/563313004/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:306: autofill_agent_->FormControlElementClicked(username_input, false); On 2014/09/19 17:34:18, Ilya Sherman wrote: > Hmm, are you sure that this code is no longer needed? I guess we'll see whether > the tests pass without it. Indeed, it looks like test fail without this code. Please restore it.
Thanks Ilya and Vaclav for review. I've incorporated missing API calls, and related browser-tests are getting pass in my local build. PTAL. Thanks! https://codereview.chromium.org/563313004/diff/40001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/563313004/diff/40001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:306: autofill_agent_->FormControlElementClicked(username_input, false); On 2014/09/19 19:08:59, Ilya Sherman wrote: > On 2014/09/19 17:34:18, Ilya Sherman wrote: > > Hmm, are you sure that this code is no longer needed? I guess we'll see > whether > > the tests pass without it. > > Indeed, it looks like test fail without this code. Please restore it. Done. Sorry! Looks like I missed executing ClickAndSelect test-case, I remember PasswordAutofillTriggersOnChangeEventsWaitForUsername getting pass. With Patch-set #5, these are getting pass in local testing. Apologies to Ilya and Vaclav.
Still LGTM, thanks for fixing the test issue. Cheers, Vaclav
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/563313004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as add36e10c37867f6d731ccee23a82184578fb683
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e5f069d57b9de5da6d710b3313b071b92e811fcb Cr-Commit-Position: refs/heads/master@{#296071} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
