Make BrowserWindow::CreateFindBar non-static so that it can be overridden by Panels.

Make BrowserWindow::CreateFindBar non-static so that it can be overridden by Panels.

BUG=None
TEST=PanelBrowserTest.FindBar

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

[jennb, 2011-07-30 01:08:29]

prasadt, jianli, dimich - could one of you take this?

static BrowserWindow::CreateFindBar code cast browser->window() to the native
BrowserWindow. For Panels, browser->window() returns a Panel that cannot be
static cast to one of these types. Instead, Panels should use the NativePanel.
Converted the static CreateFindBar to a virtual method in order to override
behavior for Panels.

I've tested on GTK and Windows - the new test passes. Using the try bot to test
Cocoa changes.  Feel free to wait on the patch until I know if it works for
Cocoa.

Jenn

[Dmitry Titov, 2011-08-01 18:17:06]

By 'take' I think you meant 'take ownership of this patch', not 'please review
it'. If so, no problem.

It failed on Mac. I'll look into it after I'm done with magic-iframe XHR issue
which is a priority for me now. Until then, it's free for grabs :-)

[jianli, 2011-08-01 18:18:46]

I am looking into this patch now.

On Mon, Aug 1, 2011 at 11:17 AM, <dimich@chromium.org> wrote:

> By 'take' I think you meant 'take ownership of this patch', not 'please
> review
> it'. If so, no problem.
>
> It failed on Mac. I'll look into it after I'm done with magic-iframe XHR
> issue
> which is a priority for me now. Until then, it's free for grabs :-)
>
>
http://codereview.chromium.**org/7538010/<http://codereview.chromium.org/7538...
>

[prasadt, 2011-08-01 18:40:22]

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_factory.mm (left):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_factory.mm:28:
static_cast<BrowserWindowCocoa*>(browser->window());
How about doing an IsPanel() check on the browser object and do the right thing?
 I think that'll simplify the check-in quite a bit and won't touch other
platforms.

[jianli, 2011-08-01 18:48:55]

Though your proposal simplifies the logic, it does not fix the underlying
bad practice which tries to static_cast to the derived class. I think we
need to fix it now because otherwise it will bring up other problem.

On Mon, Aug 1, 2011 at 11:40 AM, <prasadt@chromium.org> wrote:

>
> http://codereview.chromium.**org/7538010/diff/1001/chrome/**
>
browser/ui/cocoa/browser_**window_factory.mm<http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/cocoa/browser_window_factory.mm>
> File
chrome/browser/ui/cocoa/browse**r_window_factory.mm<http://browser_window_factory.mm>(left):
>
> http://codereview.chromium.**org/7538010/diff/1001/chrome/**
>
browser/ui/cocoa/browser_**window_factory.mm#oldcode28<http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/cocoa/browser_window_factory.mm#oldcode28>
>
chrome/browser/ui/cocoa/browse**r_window_factory.mm:28<http://browser_window_factory.mm:28>
> :
> static_cast<**BrowserWindowCocoa*>(browser->**window());
> How about doing an IsPanel() check on the browser object and do the
> right thing?  I think that'll simplify the check-in quite a bit and
> won't touch other platforms.
>
>
>
http://codereview.chromium.**org/7538010/<http://codereview.chromium.org/7538...
>

[jianli, 2011-08-01 18:52:12]

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/browser_wi...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/browser_wi...
chrome/browser/ui/browser_window.h:332: virtual FindBar* CreateFindBar() = 0;
It seems that we can add const modifier to this method.

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/browser_wi...
chrome/browser/ui/browser_window.h:333: 
One extra empty line.

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/nat...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:13: class Panel;
Can you remove this since you're in this file?

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:59: virtual FindBar*
CreatePanelFindBar() = 0;
Better to add forward declaration for FindBar though it has been provided in one
of panel.h including files.

[prasadt, 2011-08-01 19:42:47]

lgtm

If we want to do this as cleanup work, that's fine by me.

On 2011/08/01 18:48:55, jianli wrote:
> Though your proposal simplifies the logic, it does not fix the underlying
> bad practice which tries to static_cast to the derived class. I think we
> need to fix it now because otherwise it will bring up other problem.
> 
> On Mon, Aug 1, 2011 at 11:40 AM, <mailto:prasadt@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/7538010/diff/1001/chrome/**
> >
>
browser/ui/cocoa/browser_**window_factory.mm<http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/cocoa/browser_window_factory.mm>
> > File
>
chrome/browser/ui/cocoa/browse**r_window_factory.mm<http://browser_window_factory.mm>(left):
> >
> > http://codereview.chromium.**org/7538010/diff/1001/chrome/**
> >
>
browser/ui/cocoa/browser_**window_factory.mm#oldcode28<http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/cocoa/browser_window_factory.mm#oldcode28>
> >
>
chrome/browser/ui/cocoa/browse**r_window_factory.mm:28<http://browser_window_factory.mm:28>
> > :
> > static_cast<**BrowserWindowCocoa*>(browser->**window());
> > How about doing an IsPanel() check on the browser object and do the
> > right thing?  I think that'll simplify the check-in quite a bit and
> > won't touch other platforms.
> >
> >
> >
>
http://codereview.chromium.**org/7538010/%3Chttp://codereview.chromium.org/75...>
> >

[Dmitry Titov, 2011-08-01 19:57:48]

a drive by (for the record):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:25:
scoped_nsobject<FindBarCocoaController> findBarCocoaController_;
Why do we need to retain the controller here? I think this is to release the
controller when the Panel is closed. I wonder if we can simply hang on the views
hierarchy here, w/o having this pointer for lifetime purposes.
At least this has to be documented. A comment right here that explains why we
retain the controller would be useful.

[jennb, 2011-08-05 04:29:55]

On 2011/08/01 19:57:48, Dmitry Titov wrote:
> a drive by (for the record):
> 
>
http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/pan...
> File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):
> 
>
http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_window_controller_cocoa.h:25:
> scoped_nsobject<FindBarCocoaController> findBarCocoaController_;
> Why do we need to retain the controller here? I think this is to release the
> controller when the Panel is closed. I wonder if we can simply hang on the
views
> hierarchy here, w/o having this pointer for lifetime purposes.
> At least this has to be documented. A comment right here that explains why we
> retain the controller would be useful.

I'm not sure what "hang on the views hierarchy" means... Also, sorry for
confusion. By "take this", I meant "review this".  I'll be back to working on it
when I get back and will address the other issues raised.

[jennb, 2011-08-08 17:26:02]

Patch set 4 is the same as patch set 3 with a small indentation fix.

Stopped storing pointer to FindBarCocoaController in the panel window controller
as that class doesn't use the pointer directly. The pointer can be added when it
is actually needed.

Ready for another look, thanks!

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/browser_wi...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/browser_wi...
chrome/browser/ui/browser_window.h:332: virtual FindBar* CreateFindBar() = 0;
On 2011/08/01 18:52:13, jianli wrote:
> It seems that we can add const modifier to this method.

As discussed, leaving as-is. I can't guarantee that implementations won't want
to store a pointer or some other change when creating a find bar.

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/browser_wi...
chrome/browser/ui/browser_window.h:333: 
On 2011/08/01 18:52:13, jianli wrote:
> One extra empty line.

Done.

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/nat...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:13: class Panel;
On 2011/08/01 18:52:13, jianli wrote:
> Can you remove this since you're in this file?

Done.

http://codereview.chromium.org/7538010/diff/1001/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:59: virtual FindBar*
CreatePanelFindBar() = 0;
On 2011/08/01 18:52:13, jianli wrote:
> Better to add forward declaration for FindBar though it has been provided in
one
> of panel.h including files.

Done.

[Dmitry Titov, 2011-08-08 20:08:30]

lgtm for Mac with few nits:

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.h (right):

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.h:71: bool has_find_bar_; //
Find bar should only be created once per panel.
Can we make this data member that is only used in DCHECK to be DEBUG-only?

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:141:
DCHECK(!has_find_bar_);
It's nice to do it this way, but I don't have a strong opinion on that:
DCHECK(!foo) << "foo must be only teleported once";

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:43: // Retains the
given FindBarCocoaController and adds its view to this
In the comment,  no need to say "Retains", since the code currently doesn't.

[jianli, 2011-08-08 20:18:00]

LGTM.

[jennb, 2011-08-08 21:21:20]

Will commit with the changes mentioned below after mac try job.

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.h (right):

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.h:71: bool has_find_bar_; //
Find bar should only be created once per panel.
On 2011/08/08 20:08:31, Dmitry Titov wrote:
> Can we make this data member that is only used in DCHECK to be DEBUG-only?

As discussed, will not change. DCHECK is not the same as DEBUG-only. DCHECK can
be enabled for non-official release builds.

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:141:
DCHECK(!has_find_bar_);
On 2011/08/08 20:08:31, Dmitry Titov wrote:
> It's nice to do it this way, but I don't have a strong opinion on that:
> DCHECK(!foo) << "foo must be only teleported once";

Done.

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7538010/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:43: // Retains the
given FindBarCocoaController and adds its view to this
On 2011/08/08 20:08:31, Dmitry Titov wrote:
> In the comment,  no need to say "Retains", since the code currently doesn't. 

Oops. Forgot to update that comment. Good catch. Fixed.