Unit test for BrowserViewLayout that does not depend on BrowserView

Unit test for BrowserViewLayout that does not depend on BrowserView

* Break off the friendship between BrowserView and BrowserViewLayout
* Add BrowserViewLayoutDelegate
* Inject child views into BrowserViewLayout
* Convert several views to be held as views::View

BUG=233374
TEST=unit_tests BrowserViewLayout*

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

[James Cook, 2013-05-14 23:25:13]

Scott, PTAL. There are still many more test cases to build, but I'd like to land
the basic framework.

[sky, 2013-05-15 00:37:22]

Nice!

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.h:38: class
BrowserViewLayoutDelegate {
put in its own header.

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.h:40: virtual
~BrowserViewLayoutDelegate() {}
nit: make protected as BrowserViewLayout doesn't own the delegate. Or make
BrowserViewLayout own the delegate since the delegate isn't implemented on an
existing object.

[James Cook, 2013-05-15 16:45:39]

sky, please have another look

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.h:38: class
BrowserViewLayoutDelegate {
On 2013/05/15 00:37:22, sky wrote:
> put in its own header.

Done.

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.h:40: virtual
~BrowserViewLayoutDelegate() {}
On 2013/05/15 00:37:22, sky wrote:
> nit: make protected as BrowserViewLayout doesn't own the delegate. Or make
> BrowserViewLayout own the delegate since the delegate isn't implemented on an
> existing object.

I don't understand this suggestion. BrowserView owns an implementation of this
delegate via a scoped_ptr<>. It does that so I can use the same function names
in the delegate as exist in BrowserVIew (via BrowserWindow).  Is there a
particular advantage to having BrowserViewLayout own its delegate?

[sky, 2013-05-15 17:24:51]

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/14589016/diff/8001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.h:40: virtual
~BrowserViewLayoutDelegate() {}
On 2013/05/15 16:45:39, James Cook (Chromium) wrote:
> On 2013/05/15 00:37:22, sky wrote:
> > nit: make protected as BrowserViewLayout doesn't own the delegate. Or make
> > BrowserViewLayout own the delegate since the delegate isn't implemented on
an
> > existing object.
> 
> I don't understand this suggestion. BrowserView owns an implementation of this
> delegate via a scoped_ptr<>. It does that so I can use the same function names
> in the delegate as exist in BrowserVIew (via BrowserWindow).  Is there a
> particular advantage to having BrowserViewLayout own its delegate?

Makes ownership simpler. BrowserView has and owns the delegate, but it never
uses it. Your test also now has to deal with ownership of the delegate as well.
If you instead make BrowserViewLayout own the delegate then none of the other
sites need to track ownership.

The typical pattern is to implement the delegate on an existing object, in which
case the consumer of the delegate BrowserViewLayout here) can't own the
delegate. That isn't the case with your patch since BrowserView can't directly
implement the delegate.

I think making BrowserViewLayout own the delegate simplifies things slightly,
but it isn't that typical. I'm fine with either of the following:
. make destructor protected.
. make BrowserViewLayout own the delegate.

[James Cook, 2013-05-15 20:46:43]

sky, PTAL. I made the BrowserViewLayout own its delegate.

Just for my education.... I still don't understand how in the old code the
BrowserViewLayoutDelegate destructor can be made protected. It doesn't compile
when I do that (and I don't expect it would, since BrowserView owned one via a
scoped_ptr). I expect the protected-destructor only work if BrowserView derives
from/implements BrowserViewLayoutDelegate?

[sky, 2013-05-15 20:49:43]

For the specific pattern I was referring to to work you need to make
BrowserView own the specific BrowserViewLayoutDelegate implementation,
not the general class.

On Wed, May 15, 2013 at 1:46 PM,  <jamescook@chromium.org> wrote:
> sky, PTAL. I made the BrowserViewLayout own its delegate.
>
> Just for my education.... I still don't understand how in the old code the
> BrowserViewLayoutDelegate destructor can be made protected. It doesn't
> compile
> when I do that (and I don't expect it would, since BrowserView owned one via
> a
> scoped_ptr). I expect the protected-destructor only work if BrowserView
> derives
> from/implements BrowserViewLayoutDelegate?
>
>
> https://codereview.chromium.org/14589016/

[sky, 2013-05-15 20:49:58]

LGTM

[James Cook, 2013-05-15 20:51:37]

On 2013/05/15 20:49:58, sky wrote:
> LGTM

Ah, that makes sense. Thanks!

[commit-bot: I haz the power, 2013-05-15 20:51:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/14589016/22001

[commit-bot: I haz the power, 2013-05-16 01:15:49]

Change committed as 200407