Add a unittest for PasswordAutofillAgent

Add a unittest for PasswordAutofillAgent

Currently only contains a test for functionality added in https://codereview.chromium.org/231283003/.

Closed, because replaced by https://codereview.chromium.org/257803004/.

BUG=347927

[vabr (Chromium), 2014-04-24 16:06:55]

Hi Ilya,

Please review the unit test for PasswordAutofillAgent we spoke about.

I had some troubles implementing it, because of the use of RenderView in the
agent. Ways I considered:

1) Use some existing test infrastructure, like, e.g., RenderViewTest.
   -> Decided against it, because it seems to me that it's too heavy-weight and
meant for browser tests. In other words, if the test needs something offered
there, then it likely should be a browser test.

2) Construct a RenderViewImpl from scratch.
   -> Decided against it, that would mean a lot of support code, and end up in
duplicating RenderViewTest et al.

3) Stub the RenderView (also presented here as patch set 1)
   -> Decided against it, because the //content code relies on the fact that the
only class implementing RenderView is RenderViewImpl. In particular,
RenderViewObserver::RenderViewObserver uses a cast to RenderViewImpl, which
would lead the approach of patch set 1 to a crash. I'm sure this could be fixed
somehow, but a proper fix (not ad hoc patching like below :)) would probably be
a major refactoring in //content.

4) Allow PasswordAutofillAgent take a NULL RenderView (here patch set 2)
   -> That's what I chose. It's a little dirty, and it modifies production code
for test usage, which makes me sad. But that's already done in the agent's
parent (see the comment inside RenderViewObserver::RenderViewObserver), and
seems like the least evil of all of the above. Also, RenderView is only used
scarcely in the agent, and when the test starts to touch code using it, it
should again be a browser test, IMO.

I'm happy to hear any further suggestions, and of course any review comments
from you.

Thanks a lot for the long row of educational reviews I already got from you! :)

Vaclav

[Ilya Sherman, 2014-04-25 06:29:42]

Thanks for investigating this!  Based on your results, it sounds like maybe it's
just not worth writing a unit test for this class, and instead we should stick
with browser tests.  I'd rather have production code not make sacrifices for the
sake of test-specific conditions, and instead just have slightly more
heavy-weight tests.

[vabr (Chromium), 2014-04-25 11:05:53]

On 2014/04/25 06:29:42, Ilya Sherman wrote:
> Thanks for investigating this!  Based on your results, it sounds like maybe
it's
> just not worth writing a unit test for this class, and instead we should stick
> with browser tests.  I'd rather have production code not make sacrifices for
the
> sake of test-specific conditions, and instead just have slightly more
> heavy-weight tests.

Fair enough, thanks Ilya!
I filed a new CL for the browser test:
https://codereview.chromium.org/257803004/, and added you as a reviewer.
I'll close this one, to keep it as a trace of the decision to not make it a unit
test.

Cheers,
Vaclav