Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17450010:
Create AutofillDriverImpl unit test. (Closed)
|
Created:
7 years, 6 months ago by blundell Modified:
7 years, 6 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, Raman Kakilate, benquan, jam, ahutter, browser-components-watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, selim, benm (inactive) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreate AutofillDriverImpl unit test.
The unit test is currently skeletal; future CLs will flesh it out.
BUG=247015
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208039
Patch Set 1 #Patch Set 2 : Nits #
Total comments: 20
Patch Set 3 : Response to review #
Total comments: 18
Patch Set 4 : Response to review #Patch Set 5 : Nit #Patch Set 6 : Rebase after conflict #
Messages
Total messages: 10 (0 generated)
https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl_unittest.cc (right): https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:9: #include "chrome/browser/ui/autofill/tab_autofill_manager_delegate.h" Is there a way around creating the real AutofillManagerDelegate here? https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:10: #include "chrome/test/base/chrome_render_view_host_test_harness.h" My understanding is that I can't change this to be at the content level until this test is no longer in unit_tests (in fact, I tried, and got a crash). https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:63: AutofillDriverImpl* driver_; I had made this a scoped ptr but got complaints about the destructor being private.
https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl.h (right): https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:76: autofill_manager_.reset(manager.release()); nit: Can you use .Pass() instead of .release() here? I think that's preferred, when possible. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:87: friend class AutofillDriverImplTest; Please don't add a friend class; instead, move set_autofill_manager to have protected visibility, and create a subclass for testing that ups the visibility to public. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl_unittest.cc (right): https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:9: #include "chrome/browser/ui/autofill/tab_autofill_manager_delegate.h" On 2013/06/19 17:49:37, blundell wrote: > Is there a way around creating the real AutofillManagerDelegate here? You can probably use components/autofill/browser/test_autofill_manager_delegate.h here. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:10: #include "chrome/test/base/chrome_render_view_host_test_harness.h" On 2013/06/19 17:49:37, blundell wrote: > My understanding is that I can't change this to be at the content level until > this test is no longer in unit_tests (in fact, I tried, and got a crash). Yeah, that seems to be what Jói concluded as well. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:20: class AutofillManagerMock : public AutofillManager { nit: "MockAutofillManager" https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:23: autofill::AutofillManagerDelegate* delegate, nit: Since this code is in the autofill namespace already, no need for "autofill::" https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:53: scoped_ptr<AutofillManager>(autofill_manager_)); note: If you take my advice about creating a subclass of AutofillDriverImpl for testing, you can wire this up in the constructor. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:63: AutofillDriverImpl* driver_; On 2013/06/19 17:49:37, blundell wrote: > I had made this a scoped ptr but got complaints about the destructor being > private. Please create a "class TestAutofillDriverImpl : public AutofillDriverImpl { ... }" that has a public destructor and use a scoped_ptr here. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:70: driver_->SetAutofillExternalDelegate(scoped_ptr<AutofillExternalDelegate>()); Hmm, this is testing deprecated functionality, i.e. the non-native UI, which is going away soon. I'd prefer not to add a test that we expect to fail quite soon ;)
I will defer to Ilya, he knows this code better. Removing myself as a reviewer on this change. Cheers, Jói
Thanks! https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl.h (right): https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:76: autofill_manager_.reset(manager.release()); On 2013/06/20 00:07:16, Ilya Sherman wrote: > nit: Can you use .Pass() instead of .release() here? I think that's preferred, > when possible. Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:87: friend class AutofillDriverImplTest; On 2013/06/20 00:07:16, Ilya Sherman wrote: > Please don't add a friend class; instead, move set_autofill_manager to have > protected visibility, and create a subclass for testing that ups the visibility > to public. Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl_unittest.cc (right): https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:9: #include "chrome/browser/ui/autofill/tab_autofill_manager_delegate.h" On 2013/06/20 00:07:16, Ilya Sherman wrote: > On 2013/06/19 17:49:37, blundell wrote: > > Is there a way around creating the real AutofillManagerDelegate here? > > You can probably use > components/autofill/browser/test_autofill_manager_delegate.h here. Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:20: class AutofillManagerMock : public AutofillManager { On 2013/06/20 00:07:16, Ilya Sherman wrote: > nit: "MockAutofillManager" Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:23: autofill::AutofillManagerDelegate* delegate, On 2013/06/20 00:07:16, Ilya Sherman wrote: > nit: Since this code is in the autofill namespace already, no need for > "autofill::" Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:53: scoped_ptr<AutofillManager>(autofill_manager_)); On 2013/06/20 00:07:16, Ilya Sherman wrote: > note: If you take my advice about creating a subclass of AutofillDriverImpl for > testing, you can wire this up in the constructor. Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:63: AutofillDriverImpl* driver_; On 2013/06/20 00:07:16, Ilya Sherman wrote: > On 2013/06/19 17:49:37, blundell wrote: > > I had made this a scoped ptr but got complaints about the destructor being > > private. > > Please create a "class TestAutofillDriverImpl : public AutofillDriverImpl { ... > }" that has a public destructor and use a scoped_ptr here. Done. https://codereview.chromium.org/17450010/diff/1001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:70: driver_->SetAutofillExternalDelegate(scoped_ptr<AutofillExternalDelegate>()); Replaced this test with tests to test response to frame navigation. On 2013/06/20 00:07:16, Ilya Sherman wrote: > Hmm, this is testing deprecated functionality, i.e. the non-native UI, which is > going away soon. I'd prefer not to add a test that we expect to fail quite soon > ;)
Thanks! LGTM % nits: https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl.h (right): https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:74: // Sets the manager to |manager|. Takes ownership of |manager|. nit: Please document that the manager's external delegate is updated as well. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:78: nit: Spurious newline. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl_unittest.cc (right): https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:40: AutofillManager::AutofillDownloadManagerState enable_download_manager) nit: This should be indented to be aligned with all of the other params, which might mean that you need to wrap all of the other params. Alternately, you could simply get rid of this parameter, and pass a fixed value into the AutofillManager constructor, either here or from MockAutofillManager. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:41: : AutofillDriverImpl( nit: This line should be indented two more spaces. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:43: MockAutofillManager* autofill_manager = new MockAutofillManager( nit: Please declare this as a scoped_ptr. Basically, all allocations should be protected, unless something rather unusual is going on. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:80: MockAutofillManager* autofill_manager_; nit: Why store this as a variable, rather than just always access it as driver_->autofill_manager()? https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:88: DCHECK(details.is_navigation_to_different_page()); nit: Please use ASSERT_TRUE rather than DCHECK, as DCHECK can bring down the entire unit_tests suite. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:97: DCHECK(!details.is_navigation_to_different_page()); nit: Please use ASSERT_TRUE rather than DCHECK, as DCHECK can bring down the entire unit_tests suite. https://codereview.chromium.org/17450010/diff/8001/components/autofill/core/b... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/17450010/diff/8001/components/autofill/core/b... components/autofill/core/browser/autofill_manager.h:88: virtual void SetExternalDelegate(AutofillExternalDelegate* delegate); nit: Does this still need to be virtual?
Thanks! https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl.h (right): https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:74: // Sets the manager to |manager|. Takes ownership of |manager|. On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Please document that the manager's external delegate is updated as well. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl.h:78: On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Spurious newline. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... File components/autofill/content/browser/autofill_driver_impl_unittest.cc (right): https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:40: AutofillManager::AutofillDownloadManagerState enable_download_manager) On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: This should be indented to be aligned with all of the other params, which > might mean that you need to wrap all of the other params. Alternately, you > could simply get rid of this parameter, and pass a fixed value into the > AutofillManager constructor, either here or from MockAutofillManager. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:41: : AutofillDriverImpl( On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: This line should be indented two more spaces. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:43: MockAutofillManager* autofill_manager = new MockAutofillManager( On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Please declare this as a scoped_ptr. Basically, all allocations should be > protected, unless something rather unusual is going on. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:80: MockAutofillManager* autofill_manager_; Done. Had to create a TestAutofillDriverImpl method to return the manager as a MockAutofillManager*. On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Why store this as a variable, rather than just always access it as > driver_->autofill_manager()? https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:88: DCHECK(details.is_navigation_to_different_page()); On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Please use ASSERT_TRUE rather than DCHECK, as DCHECK can bring down the > entire unit_tests suite. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/conten... components/autofill/content/browser/autofill_driver_impl_unittest.cc:97: DCHECK(!details.is_navigation_to_different_page()); On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Please use ASSERT_TRUE rather than DCHECK, as DCHECK can bring down the > entire unit_tests suite. Done. https://codereview.chromium.org/17450010/diff/8001/components/autofill/core/b... File components/autofill/core/browser/autofill_manager.h (right): https://codereview.chromium.org/17450010/diff/8001/components/autofill/core/b... components/autofill/core/browser/autofill_manager.h:88: virtual void SetExternalDelegate(AutofillExternalDelegate* delegate); On 2013/06/21 20:19:24, Ilya Sherman wrote: > nit: Does this still need to be virtual? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/17450010/25001
Failed to apply patch for
components/autofill/content/browser/autofill_driver_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file components/autofill/content/browser/autofill_driver_impl.cc
Hunk #1 FAILED at 59.
Hunk #2 succeeded at 70 (offset -5 lines).
Hunk #3 succeeded at 128 (offset -5 lines).
1 out of 3 hunks FAILED -- saving rejects to file
components/autofill/content/browser/autofill_driver_impl.cc.rej
Patch: components/autofill/content/browser/autofill_driver_impl.cc
Index: components/autofill/content/browser/autofill_driver_impl.cc
diff --git a/components/autofill/content/browser/autofill_driver_impl.cc
b/components/autofill/content/browser/autofill_driver_impl.cc
index
937a8033ab92edc34519fb0ab6590c9df4760be5..09b940e9a8d0d4e9bd4d864b38eaa0a50c325d57
100644
--- a/components/autofill/content/browser/autofill_driver_impl.cc
+++ b/components/autofill/content/browser/autofill_driver_impl.cc
@@ -59,10 +59,11 @@ AutofillDriverImpl::AutofillDriverImpl(
AutofillManager::AutofillDownloadManagerState enable_download_manager,
bool enable_native_ui)
: content::WebContentsObserver(web_contents),
- autofill_manager_(this, delegate, app_locale, enable_download_manager) {
+ autofill_manager_(new AutofillManager(
+ this, delegate, app_locale, enable_download_manager)) {
if (enable_native_ui) {
SetAutofillExternalDelegate(scoped_ptr<AutofillExternalDelegate>(
- new AutofillExternalDelegate(web_contents, &autofill_manager_)));
+ new AutofillExternalDelegate(web_contents, autofill_manager_.get())));
}
}
@@ -75,52 +76,54 @@ content::WebContents* AutofillDriverImpl::GetWebContents() {
bool AutofillDriverImpl::OnMessageReceived(const IPC::Message& message) {
bool handled = true;
IPC_BEGIN_MESSAGE_MAP(AutofillDriverImpl, message)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_FormsSeen, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_FormsSeen, autofill_manager_.get(),
AutofillManager::OnFormsSeen)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_FormSubmitted, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_FormSubmitted, autofill_manager_.get(),
AutofillManager::OnFormSubmitted)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_TextFieldDidChange, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_TextFieldDidChange,
+ autofill_manager_.get(),
AutofillManager::OnTextFieldDidChange)
IPC_MESSAGE_FORWARD(AutofillHostMsg_QueryFormFieldAutofill,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnQueryFormFieldAutofill)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_ShowAutofillDialog, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_ShowAutofillDialog,
+ autofill_manager_.get(),
AutofillManager::OnShowAutofillDialog)
IPC_MESSAGE_FORWARD(AutofillHostMsg_FillAutofillFormData,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnFillAutofillFormData)
IPC_MESSAGE_FORWARD(AutofillHostMsg_DidPreviewAutofillFormData,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnDidPreviewAutofillFormData)
IPC_MESSAGE_FORWARD(AutofillHostMsg_DidFillAutofillFormData,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnDidFillAutofillFormData)
IPC_MESSAGE_FORWARD(AutofillHostMsg_DidShowAutofillSuggestions,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnDidShowAutofillSuggestions)
IPC_MESSAGE_FORWARD(AutofillHostMsg_DidEndTextFieldEditing,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnDidEndTextFieldEditing)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_HideAutofillUi, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_HideAutofillUi,
autofill_manager_.get(),
AutofillManager::OnHideAutofillUi)
IPC_MESSAGE_FORWARD(AutofillHostMsg_AddPasswordFormMapping,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnAddPasswordFormMapping)
IPC_MESSAGE_FORWARD(AutofillHostMsg_ShowPasswordSuggestions,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnShowPasswordSuggestions)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_SetDataList, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_SetDataList, autofill_manager_.get(),
AutofillManager::OnSetDataList)
IPC_MESSAGE_FORWARD(AutofillHostMsg_RequestAutocomplete,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnRequestAutocomplete)
- IPC_MESSAGE_FORWARD(AutofillHostMsg_ClickFailed, &autofill_manager_,
+ IPC_MESSAGE_FORWARD(AutofillHostMsg_ClickFailed, autofill_manager_.get(),
AutofillManager::OnClickFailed)
IPC_MESSAGE_FORWARD(AutofillHostMsg_MaybeShowAutocheckoutBubble,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::OnMaybeShowAutocheckoutBubble)
IPC_MESSAGE_FORWARD(AutofillHostMsg_RemoveAutocompleteEntry,
- &autofill_manager_,
+ autofill_manager_.get(),
AutofillManager::RemoveAutocompleteEntry)
IPC_MESSAGE_UNHANDLED(handled = false)
IPC_END_MESSAGE_MAP()
@@ -131,13 +134,19 @@ void AutofillDriverImpl::DidNavigateMainFrame(
const content::LoadCommittedDetails& details,
const content::FrameNavigateParams& params) {
if (details.is_navigation_to_different_page())
- autofill_manager_.Reset();
+ autofill_manager_->Reset();
}
void AutofillDriverImpl::SetAutofillExternalDelegate(
scoped_ptr<AutofillExternalDelegate> delegate) {
- autofill_external_delegate_.reset(delegate.release());
- autofill_manager_.SetExternalDelegate(autofill_external_delegate_.get());
+ autofill_external_delegate_ = delegate.Pass();
+ autofill_manager_->SetExternalDelegate(autofill_external_delegate_.get());
+}
+
+void AutofillDriverImpl::SetAutofillManager(
+ scoped_ptr<AutofillManager> manager) {
+ autofill_manager_ = manager.Pass();
+ autofill_manager_->SetExternalDelegate(autofill_external_delegate_.get());
}
} // namespace autofill
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/17450010/29001
Message was sent while issue was closed.
Change committed as 208039 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
