[Extension Toolbar] Refactor and finish pop out logic for actions

[Extension Toolbar] Refactor and finish pop out logic for actions

* Pull out the logic to "pop out" actions when they want to act
  into a separate inner class of ToolbarActionsBar.
* Make unit tests for ToolbarActionsBar also test the overflow
  container.
* Turn off "popping out" actions by default.

UI has decided that, for the time being, we want to hold off
on popping out actions. Since this is still in heavy flux,
keep it in the code base (and tested) to prevent code rot.

BUG=417441
Committed: https://crrev.com/f78536e0daa95933dbd5f4c868b201356dfcdf9b
Cr-Commit-Position: refs/heads/master@{#308638}

[Devlin, 2014-12-08 23:37:32]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-12-08 23:37:36]

Patchset #1 (id:20001) has been deleted

[Devlin, 2014-12-08 23:37:41]

Patchset #1 (id:40001) has been deleted

[Devlin, 2014-12-08 23:37:45]

Patchset #1 (id:60001) has been deleted

[Devlin, 2014-12-08 23:37:49]

Patchset #1 (id:80001) has been deleted

[Devlin, 2014-12-08 23:37:55]

Patchset #1 (id:100001) has been deleted

[Devlin, 2014-12-12 01:06:24]

Patchset #1 (id:120001) has been deleted

[Devlin, 2014-12-12 01:06:31]

Patchset #1 (id:140001) has been deleted

[Devlin, 2014-12-12 01:06:38]

Patchset #1 (id:160001) has been deleted

[Devlin, 2014-12-12 01:06:45]

Patchset #1 (id:180001) has been deleted

[Devlin, 2014-12-12 01:06:51]

Patchset #1 (id:200001) has been deleted

[Devlin, 2014-12-12 01:06:58]

Patchset #1 (id:220001) has been deleted

[Devlin, 2014-12-12 01:07:06]

Patchset #1 (id:240001) has been deleted

[Devlin, 2014-12-12 07:03:25]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Devlin, 2014-12-12 07:03:26]

Hey Finnur - in true Google fashion, it looks like right when I finished coding
the edge cases for the (updated) popping out logic, we decided to hold off on
popping out behavior (see bug for new behavior preference).

Since we're still very flexible on all this, and want to be able to switch
quickly (maybe experimenting with both?), rather than ripping out all the code
for popping out actions, I've refactored it to be in its own class, so it can be
flipped with a bool.  Would love any feedback on this.

[Finnur, 2014-12-12 15:06:07]

Oh, man. I sympathize. :(

My brain is a bit fried after a long week and some long-winded reviews today,
but I didn't want to have you wait until after the weekend, and managed to get
through this review. Hope I didn't miss anything glaringly obvious. :)

LGTM

https://codereview.chromium.org/766263003/diff/260001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/260001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:80: // Find where the correct
index (it's guaranteed to be after our current
nit: Missing the word 'is'?

https://codereview.chromium.org/766263003/diff/260001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:783:
SortContainer(&toolbar_actions_.get(), model_->toolbar_items(), +compare);
The + there is causing a compile failure.

[Devlin, 2014-12-12 18:27:07]

https://codereview.chromium.org/766263003/diff/260001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/260001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:80: // Find where the correct
index (it's guaranteed to be after our current
On 2014/12/12 15:06:07, Finnur wrote:
> nit: Missing the word 'is'?

Done.

https://codereview.chromium.org/766263003/diff/260001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:783:
SortContainer(&toolbar_actions_.get(), model_->toolbar_items(), +compare);
On 2014/12/12 15:06:06, Finnur wrote:
> The + there is causing a compile failure.

Ah, C++11 lambdas, aren't you lovely.

Fixed.

[Devlin, 2014-12-12 20:09:48]

Patchset #3 (id:300001) has been deleted

[Devlin, 2014-12-12 22:07:18]

rdevlin.cronin@chromium.org changed reviewers:
+ avi@chromium.org, sky@chromium.org

[Devlin, 2014-12-12 22:07:18]

+Avi for cocoa, Scott for ui/views and ui/toolbar.

Cheers!

[Avi (use Gerrit), 2014-12-12 23:53:59]

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.h (right):

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.h:71:
mainController:(BrowserActionsController*)mainController;
Document that parameter. "If this controller is for the overflow, then the
|mainController| parameter is of the controller for the main set of icons. If
|mainController| is nil, this controller is the controller of the main set of
icons" or something like that.

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:70: // because of MSVC
compilation limitations.
I'm studying the code because this comment doesn't explain things. "to match
|reference|"?

Let me rewrite this comment to be correct from what I understand.

// Takes a reference vector |reference| of length n, where n is less than or
equal to the length
// of |to_sort|, and rearranges |to_sort| so that |to_sort|'s first n elements
match the n elements
// of |reference|. The order of the remaining elements of |to_sort| (if there
are any) is unspecified.
// |equal| is used to compare the elements of |to_sort| and |reference|.

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc (right):

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:45:
static_cast<int>(toolbar_actions.size()));
That's what ASSERT_EQ is for, to stop things cold if it fails.

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:279:
testing::AssertionFailure() << "main bar error:\n" << main_bar_error <<
What is this? What happened to ASSERT_EQ/EXPECT_EQ, and their friends?

[Devlin, 2014-12-13 00:16:25]

Patchset #4 (id:340001) has been deleted

[Devlin, 2014-12-13 00:18:18]

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.h (right):

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.h:71:
mainController:(BrowserActionsController*)mainController;
On 2014/12/12 23:53:59, Avi wrote:
> Document that parameter. "If this controller is for the overflow, then the
> |mainController| parameter is of the controller for the main set of icons. If
> |mainController| is nil, this controller is the controller of the main set of
> icons" or something like that.

Done.

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:70: // because of MSVC
compilation limitations.
On 2014/12/12 23:53:59, Avi wrote:
> I'm studying the code because this comment doesn't explain things. "to match
> |reference|"?
> 
> Let me rewrite this comment to be correct from what I understand.
> 
> // Takes a reference vector |reference| of length n, where n is less than or
> equal to the length
> // of |to_sort|, and rearranges |to_sort| so that |to_sort|'s first n elements
> match the n elements
> // of |reference|. The order of the remaining elements of |to_sort| (if there
> are any) is unspecified.
> // |equal| is used to compare the elements of |to_sort| and |reference|.

Yeah, sorry, the comment was a little unclear. Rephrased.

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc (right):

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:45:
static_cast<int>(toolbar_actions.size()));
On 2014/12/12 23:53:59, Avi wrote:
> That's what ASSERT_EQ is for, to stop things cold if it fails.

See comment below about line numbers.  Also, we don't necessarily want to stop
the whole test cold - only this particular check.

https://codereview.chromium.org/766263003/diff/320001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:279:
testing::AssertionFailure() << "main bar error:\n" << main_bar_error <<
On 2014/12/12 23:53:59, Avi wrote:
> What is this? What happened to ASSERT_EQ/EXPECT_EQ, and their friends?

TL;DR: EXPECT/ASSERT are great in the body of a test, but not in multiply-called
helper functions.

Using EXPECT/ASSERT in helper functions is massively unhelpful in unit tests. 
In a test like this, we have the basic format of:

Test {
  DoSomething;
  CheckOrder();
  DoSomethingElse;
  CheckOrder();
  ....
}

where CheckOrder() is a helper function that checks the order.  But if this
CheckOrder() function uses EXPECT/ASSERT directly, then all the line numbers for
the failures are identical - which is very unhelpful when it could have been the
first, second, or nth CheckEqual() that failed.  By returning a
testing::AssertionResult, we can instead do:
Test {
  DoSomething;
  EXPECT_TRUE(CheckOrder());
  DoSomethingElse;
  EXPECT_TRUE(CheckOrder());
  ....
}
and then, when one of them fails, we get a real line number - allowing for much
more useful logs for what step of the test actually failed.

[Avi (use Gerrit), 2014-12-13 03:37:54]

lgtm

Gotcha.

[Devlin, 2014-12-15 19:00:50]

Patchset #5 (id:380001) has been deleted

[Devlin, 2014-12-15 23:24:34]

Scott, friendly ping. :)

[sky, 2014-12-16 00:53:46]

I started looking at this and came to the conclusion you and Finnr should be
owners in c/b/ui/toolbar. Can you make such a change in this patch? That way I
don't need to try and understand all this.

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:79: DCHECK_GE(to_sort->size(),
reference.size()) <<
This made me think too much. Did you consider the hopefully smaller:
swap to_sort with a temp
insert reference into to_sort.
insert difference of temp and reference into to_sort.
?

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:217: ++(resized_count);
nit: no need for () here.

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:139: static bool
pop_out_actions_to_run;
move private and add for_testing setter.

[Devlin, 2014-12-16 01:01:19]

(Responding to questions; no patch change yet)

On 2014/12/16 00:53:46, sky wrote:
> I started looking at this and came to the conclusion you and Finnr should be
> owners in c/b/ui/toolbar. Can you make such a change in this patch? That way I
> don't need to try and understand all this.

Happy to (with a comment for "extension-y stuff").  Should this be for
ui/toolbar and ui/views/toolbar, or only the former?

Finnur, are you okay with this?

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:79: DCHECK_GE(to_sort->size(),
reference.size()) <<
On 2014/12/16 00:53:46, sky wrote:
> This made me think too much. Did you consider the hopefully smaller:
> swap to_sort with a temp
> insert reference into to_sort.
> insert difference of temp and reference into to_sort.
> ?

I don't quite follow - |to_sort| and |reference| are potentially different types
(which is one of the reasons we need this complicated sort function) - how would
we insert one into the other?

[sky, 2014-12-16 01:07:09]

On 2014/12/16 01:01:19, Devlin wrote:
> (Responding to questions; no patch change yet)
> 
> On 2014/12/16 00:53:46, sky wrote:
> > I started looking at this and came to the conclusion you and Finnr should be
> > owners in c/b/ui/toolbar. Can you make such a change in this patch? That way
I
> > don't need to try and understand all this.
> 
> Happy to (with a comment for "extension-y stuff").  Should this be for
> ui/toolbar and ui/views/toolbar, or only the former?

You two have submitted enough patches that warrant both. Do it on a per file
basis as there aren't that many extension specific files.

> Finnur, are you okay with this?
> 
>
https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
> File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):
> 
>
https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
> chrome/browser/ui/toolbar/toolbar_actions_bar.cc:79:
DCHECK_GE(to_sort->size(),
> reference.size()) <<
> On 2014/12/16 00:53:46, sky wrote:
> > This made me think too much. Did you consider the hopefully smaller:
> > swap to_sort with a temp
> > insert reference into to_sort.
> > insert difference of temp and reference into to_sort.
> > ?
> 
> I don't quite follow - |to_sort| and |reference| are potentially different
types
> (which is one of the reasons we need this complicated sort function) - how
would
> we insert one into the other?

My mistake. I thought they were the same type.

[Finnur, 2014-12-16 10:56:01]

> I started looking at this and came to the conclusion you and Finnr should be
owners

Scott, I didn't realize you were familiar with the old-school way of writing
Icelandic male names. Well played, sir.

> Finnur, are you okay with this?

Yeah, sure. I guess it is probably cleaner (and less likely to go out of sync)
if the files live in ui/views/toolbar/extensions and we become owners of that,
but per-file is also fine by me.

[Devlin, 2014-12-16 17:04:22]

OWNERS format look okay, Scott?

On 2014/12/16 10:56:01, Finnur wrote:
> Yeah, sure. I guess it is probably cleaner (and less likely to go out of sync)
> if the files live in ui/views/toolbar/extensions and we become owners of that,
> but per-file is also fine by me.

I think I'd like to hold off on that for now - we're still experimenting with
the idea of "chrome/component actions", so I think keeping most of these
conceptually separate from extensions is a plus.  Also, I think it makes the
changes here easier to read.  Definitely agree that these are a little scattered
- I'll make it a priority to try and move/rename appropriate in the near future
to make it more clear.

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/766263003/diff/400001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:139: static bool
pop_out_actions_to_run;
On 2014/12/16 00:53:46, sky wrote:
> move private and add for_testing setter.

Done.

[sky, 2014-12-16 17:06:55]

Yes, LGTM - thanks!

[Devlin, 2014-12-16 17:17:09]

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

[commit-bot: I haz the power, 2014-12-16 17:17:37]

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

[commit-bot: I haz the power, 2014-12-16 19:44:49]

Committed patchset #6 (id:420001)

[commit-bot: I haz the power, 2014-12-16 19:45:40]

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

[Thiemo Nagel, 2015-11-30 16:52:16]

tnagel@chromium.org changed reviewers:
+ tnagel@chromium.org

[Thiemo Nagel, 2015-11-30 16:52:17]

Hi Devlin,

while looking into a crash in ToolbarActionsBar::ReorderActions() (cf.
https://crbug.com/563557) I've come across the SortContainer() code and I think
I've found a bug there and also I have a few comments.  So the easiest thing
seemed to resurrect the old code review.

I hope you don't mind,
Thiemo

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:81: if (reference.empty())
Once the "-1" has been dropped below, this check can be removed.

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:85: for (size_t i = 0; i <
reference.size() - 1; ++i) {
This looks like an off-by-one: As far as I can see, the "-1" must be removed to
be consistent with the description in the comment.

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:86: if (!equal(to_sort->at(i),
reference[i])) {
For efficiency and readability, I'd suggest to replace "->at(i)" by "[i]" and --
to keep the same coverage -- convert the DCHECK in line 79 to a CHECK.

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:90: while
(!equal(to_sort->at(j), reference[i])) {
Using "at()" here is a very implicit way of saying: "I'll blow up if to_sort is
not a superset of reference."  I wonder if that could be made more explicit?

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:95: std::swap(to_sort->at(i),
to_sort->at(j));
"at()" doesn't seem necessary here since both indices i and j have already been
checked above.

[Devlin, 2015-11-30 17:07:03]

TL;DR: The at()s should be updated.  The real bug is a corner case that would
have triggered the DCHECK to fail, so maybe it should be a CHECK.  I'm working
on fixing that case separately.

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:85: for (size_t i = 0; i <
reference.size() - 1; ++i) {
On 2015/11/30 16:52:16, Thiemo Nagel wrote:
> This looks like an off-by-one: As far as I can see, the "-1" must be removed
to
> be consistent with the description in the comment.

If every element up to the last one is in the proper place, the last once cannot
be out of place (where else would it go?).  We also use j = i+1, which will blow
up if i == reference.size().

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:86: if (!equal(to_sort->at(i),
reference[i])) {
On 2015/11/30 16:52:17, Thiemo Nagel wrote:
> For efficiency and readability, I'd suggest to replace "->at(i)" by "[i]" and
--
> to keep the same coverage -- convert the DCHECK in line 79 to a CHECK.

Huh.  Didn't realize there was a significant difference in [] vs at().  Yes,
this should be updated.

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:90: while
(!equal(to_sort->at(j), reference[i])) {
On 2015/11/30 16:52:17, Thiemo Nagel wrote:
> Using "at()" here is a very implicit way of saying: "I'll blow up if to_sort
is
> not a superset of reference."  I wonder if that could be made more explicit?

well, in theory, the at() is guaranteed to be safe because of the DCHECK on line
79 (the bug is of course a missing corner case).

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:95: std::swap(to_sort->at(i),
to_sort->at(j));
On 2015/11/30 16:52:16, Thiemo Nagel wrote:
> "at()" doesn't seem necessary here since both indices i and j have already
been
> checked above.

Ditto - didn't realize at() and [] were significantly different.

[Thiemo Nagel, 2015-11-30 17:35:42]

> TL;DR: The at()s should be updated.

Awesome, thanks for the quick reply!  Would you mind adding me to that CL so
that we can continue the discussion there?

I think it would be great if the assumptions that are being made about |to_sort|
and |reference| could be documented at the top of SortContainer() and also
whether the code will crash or whether it will result in undefined behaviour if
the assumptions are violated.  (In my opinion both could be valid as long as the
caller's expectations are managed accordingly.)

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:85: for (size_t i = 0; i <
reference.size() - 1; ++i) {
> If every element up to the last one is in the proper place, the last once
cannot
> be out of place (where else would it go?).  We also use j = i+1, which will
blow
> up if i == reference.size().

Thank you, now I see.  That might be worth a comment.  ;)

https://codereview.chromium.org/766263003/diff/420001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:90: while
(!equal(to_sort->at(j), reference[i])) {
On 2015/11/30 17:07:03, Devlin wrote:
> well, in theory, the at() is guaranteed to be safe because of the DCHECK on
line
> 79

Yes, in combination with that on line 92.