Make autofill stop depending on InfoBarService

chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h

Index: chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h
diff --git a/chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h b/chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..c04b6f0d1109c2f23b9c1da463178f241810f25a
--- /dev/null
+++ b/chrome/browser/autofill/autofill_cc_import_confirmation_delegate.h
@@ -0,0 +1,25 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_IMPORT_CONFIRMATION_DELEGATE_H_
+#define CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_IMPORT_CONFIRMATION_DELEGATE_H_
+
+namespace autofill {
+
+class AutofillCCImportConfirmationDelegate {
+ public:
+  virtual ~AutofillCCImportConfirmationDelegate() {}
+
+  //!!! Add comments
+
+  virtual void DidShowConfirmation() = 0;
+  virtual void DidAccept() = 0;
+  virtual void DidDeny() = 0;
+  virtual void DidCancel() = 0;
+  virtual void DidIgnore() = 0;
+};

[Ilya Sherman, 2013/03/04 21:07:34]

I don't think this interface is really necessary to separate out from the
implementation class.

[kaiwang, 2013/03/05 01:48:56]

I think there are 2 benefits to define such an interface
1. The engineers working on UI don't need to care about the impl detail about
this delegate. Looking at such clean interface is easier for them.
2. Impl of this interface (AutofillCCImportConfirmationHandler) may have other
public functions added later to interact with other part of autofill. UI code
should not know those functions.

[Ilya Sherman, 2013/03/05 02:50:39]

I rather disagree.  About 50% of the time when I look at a class's API, I
double-check the implementation file to make sure I'm understanding what the
code does.  Separating the header files just makes this a little harder.  IMO
what's helpful is having a small public API, which is the case other way.
We can cross that bridge when we come to it.  Right now that's not a concern.

[kaiwang, 2013/03/05 03:57:23]

Seems the only CONs is about code reading. I really think the PROs it gains are
more important.
IMO, chromium code has many "unnecessary" interfaces. E.g. BrowserProcess vs
BrowserProcessImpl, TaskRunner vs MessageLoop, etc. Even in autofill, there are
AutofillPopupDelegate, AutofillDialogView, etc

I do feel harder to read code in this way. But the good thing is they helps a
lot to decouple components and prevent future dependency problems. For this CL,
I'm trying to  decouple components/autofill and chrome/browser/autofill.

With an interface, it's also very easy to write a mock and do testing, while
mocking an concrete class is always hard and easy to cause further problems (any
changes to the class  may break testing).

[Ilya Sherman, 2013/03/05 04:37:39]

I'm less familiar with the history of the non-Autofill interfaces, but the
Autofill ones all serve some specific purpose.  The two you listed both exist
because they are implemented by multiple classes each.
Could you explain how the pure interface helps decouple these components, vs.
combining this with the single class that implements the interface?  Maybe I'm
not following what the game plan is...  Is the plan to export this interface
from the component, while keeping the implementation class private to the
component?
You can use exactly the same mock code to mock the concrete class as you would
to mock out this interface, because the public interfaces of the two classes are
identical.

[kaiwang, 2013/03/05 18:38:16]

BrowserProcess has only BrowserProcessImpl and testing subclasses,right?
By decoupling, I mean "depend as less as possible". Interface is less than a
concrete class :)
I think the further problem is as I mentioned, the concrete class may grow out
of our expectation. As the result, the other component will be actually
depending on more and more.
Yes, we can. Just when a concrete class grows larger, the mock will be hard to
write and understand. E.g. see the old AutofillExternalDelegate mocks I deleted
in https://chromiumcodereview.appspot.com/12340065/

[Ilya Sherman, 2013/03/05 22:56:57]

Like I said, I'm not familiar with the design decisions for this interface. 
But, the BrowserProcess interface is pretty large on its own, and the
BrowserProcessImpl interface is also quite large.  That's pretty different from
the interface we're discussing here.
There are two reasons why I don't find this reasoning compelling in this case:
(1) This code is very simple, and not under development.  There's very little
reason to expect the API to change.
(2) This interface's single purpose *is* to glue together the two components. 
There's no reason to expect the Impl interface to change without corresponding
changes in the abstract interface.  Hence, even if the interface does change,
separating the two won't help keep the code less coupled.

If the primary motivation for separating out the interface is that the concrete
class *might* change in the future, then I strongly disagree that there is
compelling reason to keep the classes separate.

+
+}  // namespace autofill
+
+#endif  // CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_IMPORT_CONFIRMATION_DELEGATE_H_