Move InfoBarResponder into its own file.

Move InfoBarResponder into its own file.

Committed: https://crrev.com/d21e80f9dba8597df808e6083e58b0b956352e1c
Cr-Commit-Position: refs/heads/master@{#327028}

[Michael van Ouwerkerk, 2015-04-22 17:11:27]

mvanouwerkerk@chromium.org changed reviewers:
+ pkasting@chromium.org

[Michael van Ouwerkerk, 2015-04-22 17:11:27]

Peter, could you take a look please? I wasn't sure whether this is the right way
to set up a reusable test helper class.

[Peter Kasting, 2015-04-23 00:14:08]

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_responder.cc (right):

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:25: has_observed_ = true;
Seems like you could also just do "infobar_service_->RemoveObserver(this);", and
eliminate the need for this variable entirely.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:30: // Respond to the infobar
asynchronously, like a person.
Nit: I don't think this comment adds anything

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:37: if (accept_) {
Nit: No {}

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_responder.h (right):

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:19: // tests.
Nit: How about:

Used by test code to asynchronously respond to the first infobar shown, which
must have a ConfirmInfoBarDelegate.  This can be used to ensure various
interaction flows work correctly.

The asynchronous response matches how real users will use the infobar.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:22: InfoBarResponder(InfoBarService*
infobar_service, bool accept);
Nit: Change name to |should_accept| (also the member variable) and add comment:

If |should_accept| is true, the responder will asynchronously Accept() the
infobar; otherwise it will Cancel() it.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:25: //
infobars::InfoBarManager::Observer
Nit: Add trailing colon

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:33: bool has_observed_;
Nit: |infobar_seen_| might be slightly more descriptive

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:34: };
Nit: DISALLOW_COPY_AND_ASSIGN

[Michael van Ouwerkerk, 2015-04-23 13:36:10]

Thanks Peter, please take another look.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_responder.cc (right):

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:25: has_observed_ = true;
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Seems like you could also just do "infobar_service_->RemoveObserver(this);",
and
> eliminate the need for this variable entirely.

I thought I tried that when I originally wrote this, but testing it now it seems
fine, so Done.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:30: // Respond to the infobar
asynchronously, like a person.
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: I don't think this comment adds anything

Done.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:37: if (accept_) {
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_responder.h (right):

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:19: // tests.
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: How about:
> 
> Used by test code to asynchronously respond to the first infobar shown, which
> must have a ConfirmInfoBarDelegate.  This can be used to ensure various
> interaction flows work correctly.
> 
> The asynchronous response matches how real users will use the infobar.

Done.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:22: InfoBarResponder(InfoBarService*
infobar_service, bool accept);
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: Change name to |should_accept| (also the member variable) and add
comment:
> 
> If |should_accept| is true, the responder will asynchronously Accept() the
> infobar; otherwise it will Cancel() it.

Done.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:25: //
infobars::InfoBarManager::Observer
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: Add trailing colon

Done.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:33: bool has_observed_;
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: |infobar_seen_| might be slightly more descriptive

No longer needed.

https://codereview.chromium.org/1100953002/diff/20001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.h:34: };
On 2015/04/23 00:14:08, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Done.

[Michael van Ouwerkerk, 2015-04-23 13:36:42]

mvanouwerkerk@chromium.org changed reviewers:
+ peter@chromium.org

[Michael van Ouwerkerk, 2015-04-23 13:36:43]

Mr Beverloo, could you take a look please as owner of notifications?

[Peter Kasting, 2015-04-23 22:54:48]

LGTM

https://codereview.chromium.org/1100953002/diff/40001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_responder.cc (right):

https://codereview.chromium.org/1100953002/diff/40001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:20:
infobar_service_->RemoveObserver(this);
Nit: Might want to comment: "This is safe even if we were already removed as an
observer in OnInfoBarAdded()."

[Michael van Ouwerkerk, 2015-04-24 11:16:38]

Thanks!

https://codereview.chromium.org/1100953002/diff/40001/chrome/browser/infobars...
File chrome/browser/infobars/infobar_responder.cc (right):

https://codereview.chromium.org/1100953002/diff/40001/chrome/browser/infobars...
chrome/browser/infobars/infobar_responder.cc:20:
infobar_service_->RemoveObserver(this);
On 2015/04/23 22:54:48, Peter Kasting wrote:
> Nit: Might want to comment: "This is safe even if we were already removed as
an
> observer in OnInfoBarAdded()."

Done.

[Peter Beverloo, 2015-04-26 19:59:57]

lgtm

[Michael van Ouwerkerk, 2015-04-27 11:03:40]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2015-04-27 11:03:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1100953002/#ps60001
(title: "Add comment.")

[commit-bot: I haz the power, 2015-04-27 11:03:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100953002/60001

[commit-bot: I haz the power, 2015-04-27 12:50:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-27 12:51:17]

Patchset 4 (id:??) landed as
https://crrev.com/d21e80f9dba8597df808e6083e58b0b956352e1c
Cr-Commit-Position: refs/heads/master@{#327028}