Cleanup: Remove pointless GetInsets() override. Simplify |container_size_| t...

Cleanup: Remove pointless GetInsets() override.  Simplify |container_size_| to just be |container_width_|.  Fix indenting/alignment, especially of function parameters.  L"" -> std::wstring().  Don't handle assertion violations (style guide/simplicity).  Reduce indenting via early-return or simpler-path-return.  Streamline code where possible.  Definition order should match declaration order.  EXPECT_STREQ -> EXPECT_EQ.

BUG=50107
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53841

[Finnur, 2010-07-27 09:02:23]

Nice! Thanks.

Just a few minor things (below).

http://codereview.chromium.org/3076001/diff/6001/7001
File chrome/browser/views/browser_actions_container.cc (left):

http://codereview.chromium.org/3076001/diff/6001/7001#oldcode578
chrome/browser/views/browser_actions_container.cc:578: this);  //
ExtensionPopupDelegate
nit: same question here (comment).

http://codereview.chromium.org/3076001/diff/6001/7001
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/3076001/diff/6001/7001#newcode138
chrome/browser/views/browser_actions_container.cc:138:
panel_->OnBrowserActionExecuted(this, false);
nit: Doesn't the style guide say you should document what you are passing in
when you have simple bool/int params?

http://codereview.chromium.org/3076001/diff/6001/7001#newcode506
chrome/browser/views/browser_actions_container.cc:506: inspect_with_devtools,
ExtensionPopup::BUBBLE_CHROME, this);
nit: same question here (comments deleted).

http://codereview.chromium.org/3076001/diff/6001/7001#newcode523
chrome/browser/views/browser_actions_container.cc:523: kButtonSize);
I'm not a huge fan of this consolidation. The way it was before was more
readable and easier to debug when you step through it and I don't really see any
benefits (the compiler is going to create unnamed temporaries anyway, right?)...

http://codereview.chromium.org/3076001/diff/6001/7001#newcode594
chrome/browser/views/browser_actions_container.cc:594: static const int
kDropIndicatorWidth = 2;
Is this here because the other one was moved? Just curious...

[Peter Kasting, 2010-07-27 18:24:42]

http://codereview.chromium.org/3076001/diff/6001/7001
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/3076001/diff/6001/7001#newcode138
chrome/browser/views/browser_actions_container.cc:138:
panel_->OnBrowserActionExecuted(this, false);
On 2010/07/27 09:02:23, Finnur wrote:
> nit: Doesn't the style guide say you should document what you are passing in
> when you have simple bool/int params?

The precise style guide text is:

"When you pass in NULL, boolean, or literal integer values to functions, you
should consider adding a comment about what they are, or make your code
self-documenting by using constants."

Thus it's suggested, but not required.  There are a couple reasons I removed the
"// inspect_with_devtools" comments in this file.

First, they didn't seem to add clarity.  In some cases, like the one here, the
comment looked like a comment on the whole function call rather than on one
argument, which was confusing.  And even in the other cases,
"inspect_with_devtools" on its own is an argument name, not an explanation, and
is meaningless without going and studying the original function definition.  If
someone has to do that anyway, adding the arg name here helps little.

Second, in some cases (though not this particular one), the result was
noticeably less noisy, because in addition to the comment disappearing, things
could beome single-line, and then sometimes lose surrounding braces too.

Third, we don't annotate almost any other instance of bool, int, or NULL
arguments to functions, in this or any other file in the codebase, so it felt
inconsistent.

If you think these added a lot of clarity, though, I can restore them.  Let me
know.

http://codereview.chromium.org/3076001/diff/6001/7001#newcode523
chrome/browser/views/browser_actions_container.cc:523: kButtonSize);
On 2010/07/27 09:02:23, Finnur wrote:
> I'm not a huge fan of this consolidation. The way it was before was more
> readable and easier to debug when you step through it and I don't really see
any
> benefits (the compiler is going to create unnamed temporaries anyway,
right?)...

I partly did this because the final form (after the functional patch coming up
later) is even (slightly) simpler:

  return gfx::Size(std::min(std::max(ContainerMinSize(),
    container_width_ - resize_amount_), IconCountToWidth(-1, false)), 0);

Because it's now two lines, it's easier to read than what's in this patch.

But if you still don't like that, then one possibility would be:

  int resized_width = container_width_ - resize_amount_;
  return gfx::Size(std::min(std::max(ContainerMinSize(), resized_width),
                            IconCountToWidth(-1, false)), 0);

Another would be:

  int clamped width = std::min(
      std::max(ContainerMinSize(), container_width_ - resize_amount_),
      IconCountToWidth(-1, false));
  return gfx::Size(clamped_width, 0);

All of these seem a bit less verbose than the original.  WDYT?

http://codereview.chromium.org/3076001/diff/6001/7001#newcode594
chrome/browser/views/browser_actions_container.cc:594: static const int
kDropIndicatorWidth = 2;
On 2010/07/27 09:02:23, Finnur wrote:
> Is this here because the other one was moved? Just curious...

Since it's only used here it seemed clearer to just declare it here.  This is
also motivated by the fact that, after the upcoming functional change, there are
almost no constants atop this file.

[Finnur, 2010-07-27 20:13:17]

Sorry for the delay -- got distracted.

LGTM, one nit (below).

http://codereview.chromium.org/3076001/diff/6001/7001
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/3076001/diff/6001/7001#newcode138
chrome/browser/views/browser_actions_container.cc:138:
panel_->OnBrowserActionExecuted(this, false);
OK

> The precise style guide text is:

http://codereview.chromium.org/3076001/diff/6001/7001#newcode523
chrome/browser/views/browser_actions_container.cc:523: kButtonSize);
I think the last version (clamped_width) is a decent compromise.

On 2010/07/27 18:24:43, Peter Kasting wrote:
> WDYT?