Factor out the layout code from OpaqueBrowserFrameView for testing.

Factor out the layout code from OpaqueBrowserFrameView for testing.

As is, OpaqueBrowserFrameView is untestable due to its
dependencies. This patch separates out all the layout and sizing logic
into a LayoutManager subclass, makes OpaqueBrowserFrameView use that,
and then adds tests for the new LayoutManager.

Note that I've deliberately made as few cleanups to the layout logic as
possible. The point is to get this code under test before I start more
radical changes.

BUG=281788
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220683

[sky, 2013-08-30 16:35:18]

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/view_ids.h
File chrome/browser/ui/view_ids.h (right):

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/view_id...
chrome/browser/ui/view_ids.h:17: // Views which make up the skyline.
Document these are views only.

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (left):

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:284: #if
!defined(OS_CHROMEOS)
It looks like you nuked this in the new code. Intentional?

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc (right):

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:99: const
gfx::Size& tabstrip_preferred_size, int available_width) const {
nit: wrap each param to its own line (here and every where).

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:146: if
(OpaqueBrowserFrameViewLayout::ShouldAddDefaultCaptionButtons()) {
nit: don't need OpaqueBrowserFrameViewLayout::

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout.cc:389:
close_button_ = static_cast<views::ImageButton*>(view);
Can you DCHECK on the view class names of the ones that need a specific type?
You'll need to add a kViewClass/GetClassName override for ImageButton (Label
already has one).

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_delegate.h
(right):

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view_layout_delegate.h:58: };
protected virtual destructor.

[Elliot Glaysher, 2013-08-30 17:55:01]

ptal

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (left):

https://codereview.chromium.org/23531006/diff/59001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:284: #if
!defined(OS_CHROMEOS)
On 2013/08/30 16:35:18, sky wrote:
> It looks like you nuked this in the new code. Intentional?

Intentional, and I had to do some history diving to figure out why they were
here. These ifdefs were added back in the ViewsGtk days when we used
OpaqueBrowserFrameView. These days, chromeos uses BrowserNonClientFrameViewAsh
instead, so this shouldn't change any behaviour, while removing a check that was
screwing up the calculations in unit tests.

[sky, 2013-08-30 19:50:29]

LGTM

[commit-bot: I haz the power, 2013-08-30 19:55:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/23531006/83001

[commit-bot: I haz the power, 2013-08-30 22:56:45]

Change committed as 220683