Remove TabContents from autofill unittests.

Remove TabContents from autofill unittests.

BUG=107201
TEST=no visible change


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163715

[Avi (use Gerrit), 2012-10-16 22:04:28]

dhollowa: please also look at my comment; can we roll the two together?
joi: yeah, sorry about the DEPS...

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/DEPS
File chrome/browser/autofill/DEPS (right):

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/DE...
chrome/browser/autofill/DEPS:30:
"!chrome/browser/password_manager/password_manager_delegate_impl.h",
This sucks, I know, but these are real dependencies that were hidden behind the
TabContents.

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_external_delegate.cc (right):

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_external_delegate.cc:114:
autofill_manager_->OnDidShowAutofillSuggestions(
This is a bug in your current unit tests. Fixed here.

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:210:
AutocompleteHistoryManager::CreateForWebContents(web_contents);
This is a very interesting question. Is the AutocompleteHistoryManager useful
apart from the AutofillManager? Right now, the AutofillManager flat-out requires
the AutocompleteHistoryManager, and heck, the AutocompleteHistoryManager's unit
test requires instantiation of an AutofillManager.

I think I might not have gone far enough; perhaps we should just make the
AutocompleteHistoryManager a member of AutofillManager and be done with it.

[Jói, 2012-10-16 22:08:04]

LGTM, will let David respond on your question about AutocompleteHistoryManager.

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/DEPS
File chrome/browser/autofill/DEPS (right):

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/DE...
chrome/browser/autofill/DEPS:30:
"!chrome/browser/password_manager/password_manager_delegate_impl.h",
On 2012/10/16 22:04:28, Avi wrote:
> This sucks, I know, but these are real dependencies that were hidden behind
the
> TabContents.

No prob!

[Avi (use Gerrit), 2012-10-16 22:12:18]

Nice. Oh, I just removed a file from your DEPS list that didn't exist.

[Jói, 2012-10-16 22:14:21]

Cool, thanks.

On Tue, Oct 16, 2012 at 10:12 PM,  <avi@chromium.org> wrote:
> Nice. Oh, I just removed a file from your DEPS list that didn't exist.
>
> https://codereview.chromium.org/11185011/

[Avi (use Gerrit), 2012-10-18 22:29:31]

Dave, ping. What do you think?

[dhollowa, 2012-10-18 23:08:46]

On 2012/10/18 22:29:31, Avi wrote:
> Dave, ping. What do you think?

Hi Avi, sorry, this slipped by me.  Ilya is closest to the history-mgr
refactorings these days.  So I'm going to cc myself, and add him as reviewer to
answer your Q.

[Ilya Sherman, 2012-10-19 00:05:27]

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11185011/diff/2001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:210:
AutocompleteHistoryManager::CreateForWebContents(web_contents);
On 2012/10/16 22:04:28, Avi wrote:
> This is a very interesting question. Is the AutocompleteHistoryManager useful
> apart from the AutofillManager? Right now, the AutofillManager flat-out
requires
> the AutocompleteHistoryManager, and heck, the AutocompleteHistoryManager's
unit
> test requires instantiation of an AutofillManager.
> 
> I think I might not have gone far enough; perhaps we should just make the
> AutocompleteHistoryManager a member of AutofillManager and be done with it.

I think that's reasonable.

[Avi (use Gerrit), 2012-10-19 01:14:40]

I misread things, and the AutocompleteHistoryManager's test doesn't actually
require an AutofillManager. But yeah, making it a private implementation detail
of AutofillManager shouldn't be hard; I'll have a new patch up tomorrow.

[Avi (use Gerrit), 2012-10-23 17:47:30]

Ilya, please take a look at this new version.

[Ilya Sherman, 2012-10-23 21:33:52]

LGTM

[commit-bot: I haz the power, 2012-10-23 21:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11185011/19023

[commit-bot: I haz the power, 2012-10-23 21:40:25]

Presubmit check for 11185011-19023 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You added one or more #includes of files that are temporarily
allowed but being removed. Can you avoid introducing the
#include? See relevant DEPS file(s) for details and contacts.
  chrome/browser/autofill/autofill_manager_unittest.cc
      Illegal include: "chrome/browser/password_manager/password_manager.h"
    Because of "!chrome/browser/password_manager/password_manager.h" from
chrome/browser/autofill's specific_include_rules. \
  chrome/browser/autofill/autofill_manager_unittest.cc
      Illegal include:
"chrome/browser/password_manager/password_manager_delegate_impl.h"
    Because of
"!chrome/browser/password_manager/password_manager_delegate_impl.h" from
chrome/browser/autofill's specific_include_rules. \
  chrome/browser/autofill/autofill_metrics_unittest.cc
      Illegal include: "chrome/browser/infobars/infobar_tab_helper.h"
    Because of "!chrome/browser/infobars/infobar_tab_helper.h" from
chrome/browser/autofill's specific_include_rules.

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views

Presubmit checks took 1.4s to calculate.

[Avi (use Gerrit), 2012-10-23 21:42:41]

Ben, OWNER review for ui?

[Ben Goodger (Google), 2012-10-23 21:50:02]

lgtm

[commit-bot: I haz the power, 2012-10-23 21:54:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11185011/19023

[commit-bot: I haz the power, 2012-10-23 23:14:00]

Change committed as 163715