Add tests for the BrowserActionsContainer in overflow mode

Add tests for the BrowserActionsContainer in overflow mode
We don't have any.  We should.  Add some! :)
BUG=391280
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291514

[Finnur, 2014-08-22 16:54:58]

LGTM, with nits.

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:389: "
has improper visibility in main: " << visible;
Don't you want main_bar_->GetBrowserActionViewAt(i)->visible() instead of
|visible| here (because you should point out what it is, not what you expect it
to be)?

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:404: //
using files.
Yup, I like that idea.

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:446:
EXPECT_TRUE(ViewOrdersMatch());
This call is redundant, it is implied by calling VerifyVisibleCount().

[Devlin, 2014-08-22 17:00:15]

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:389: "
has improper visibility in main: " << visible;
On 2014/08/22 16:54:58, Finnur wrote:
> Don't you want main_bar_->GetBrowserActionViewAt(i)->visible() instead of
> |visible| here (because you should point out what it is, not what you expect
it
> to be)?

Yeah, probably. :)  Done.  (For ease, just made this !visible)

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:404: //
using files.
On 2014/08/22 16:54:58, Finnur wrote:
> Yup, I like that idea.

Mmkay, will do in a follow up :)

https://codereview.chromium.org/469973005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:446:
EXPECT_TRUE(ViewOrdersMatch());
On 2014/08/22 16:54:58, Finnur wrote:
> This call is redundant, it is implied by calling VerifyVisibleCount().

Yeah... I went back and forth on this.  My basic thinking was that we want to
explicitly show that we're testing it, but I guess it is kinda superfluous. 
Removed.

[Devlin, 2014-08-22 17:00:40]

+Peter for views/toolbar OWNERS.

[Peter Kasting, 2014-08-22 18:00:52]

LGTM

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:26:
using testing::AssertionSuccess;
Nit: Avoid using statements unless they significantly improve readability. 
ExtensionToolbarModel is already used in a qualified way in several places in
the file; the other two you added are used a small number of times.  I'd remove
all of these, and see if it's possible to remove the existing one as well.

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:339: };
Nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:407:
.AppendASCII("browser_action")
Nit: Why not pull
"test_data_dir_.AppendASCII("api_test").AppendASCII("browser_action")" out to a
temp?

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:419: //
from time to time.
Nit: Be less colloquial; say precisely when you need to call layout ("we need to
manually retrigger layout each time we change the number of icons in the bar",
that sort of thing).

[Devlin, 2014-08-22 20:10:59]

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:26:
using testing::AssertionSuccess;
On 2014/08/22 18:00:52, Peter Kasting wrote:
> Nit: Avoid using statements unless they significantly improve readability. 
> ExtensionToolbarModel is already used in a qualified way in several places in
> the file; the other two you added are used a small number of times.  I'd
remove
> all of these, and see if it's possible to remove the existing one as well.

Done.

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:339: };
On 2014/08/22 18:00:52, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:407:
.AppendASCII("browser_action")
On 2014/08/22 18:00:52, Peter Kasting wrote:
> Nit: Why not pull
> "test_data_dir_.AppendASCII("api_test").AppendASCII("browser_action")" out to
a
> temp?

Done.

https://codereview.chromium.org/469973005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:419: //
from time to time.
On 2014/08/22 18:00:52, Peter Kasting wrote:
> Nit: Be less colloquial; say precisely when you need to call layout ("we need
to
> manually retrigger layout each time we change the number of icons in the bar",
> that sort of thing).

Done.

[Devlin, 2014-08-22 20:13:32]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-08-22 20:14:35]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/469973005/...

[commit-bot: I haz the power, 2014-08-22 21:49:00]

Committed patchset #3 (80001) as 291514