First cut at implementation of FindBar for views / gtk...

First cut at implementation of FindBar for views / gtk
Also had to implement change notification for TextField on views / gtk

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

[sky, 2009-09-08 16:19:42]

http://codereview.chromium.org/200035/diff/6014/6034
File chrome/browser/views/find_bar_host.h (right):

http://codereview.chromium.org/200035/diff/6014/6034#newcode34
Line 34: // find-in-page functionality. It uses the FindBarHost implementation
to
This second sentence doesn't seem right (it wasn't right before either). Doesn't
'it' here refer to the FindBarHost, so that this reads as 'FindBarHost uses the
FindBarHost implementation...'

http://codereview.chromium.org/200035/diff/6014/6021
File chrome/browser/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/200035/diff/6014/6021#newcode23
Line 23: #include "views/widget/widget_gtk.h"
This looks like more includes than you need.

http://codereview.chromium.org/200035/diff/6014/6031
File chrome/browser/views/find_bar_host_win.cc (right):

http://codereview.chromium.org/200035/diff/6014/6031#newcode21
Line 21: #include "views/widget/widget_win.h"
This looks like more includes than you need.

http://codereview.chromium.org/200035/diff/6014/6031#newcode195
Line 195: DCHECK(!esc_accel_target_registered_);
Can't this be common to both? Jay make the VK constants portable
(base::VKEY_...) by way of base/keyboard_codes.h

http://codereview.chromium.org/200035/diff/6014/6031#newcode202
Line 202: DCHECK(esc_accel_target_registered_);
Can this be common too?

http://codereview.chromium.org/200035/diff/6014/6018
File views/controls/textfield/native_textfield_gtk.h (right):

http://codereview.chromium.org/200035/diff/6014/6018#newcode48
Line 48: GtkWidget* entry,
If these wrapped lines won't align at the (, then indent them 4.

http://codereview.chromium.org/200035/diff/6014/6018#newcode55
Line 55: gboolean OnChanged();
nit: newline between this and DISALLOW.

http://codereview.chromium.org/200035/diff/6014/6019
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/200035/diff/6014/6019#newcode174
Line 174: #if defined(OS_WIN)
Can this be key.key == base::VKEY_RETURN?

http://codereview.chromium.org/200035/diff/6014/6019#newcode183
Line 183: #if defined(OS_WIN)
Can this be key.key == base::VKEY_ESCAPE?

http://codereview.chromium.org/200035/diff/6014/6015
File views/widget/root_view.cc (right):

http://codereview.chromium.org/200035/diff/6014/6015#newcode368
Line 368: if (!gtk_widget_is_focus(widget)) {
nit: no {}

[jcampan, 2009-09-08 21:42:40]

I reviewed mostly the focus changes in root_view and widget_gtk.

http://codereview.chromium.org/200035/diff/6014/6021
File chrome/browser/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/200035/diff/6014/6021#newcode28
Line 28: : WidgetGtk(TYPE_CHILD),
Nit: should be 4 spaces indented

http://codereview.chromium.org/200035/diff/6014/6015
File views/widget/root_view.cc (right):

http://codereview.chromium.org/200035/diff/6014/6015#newcode362
Line 362: HWND hwnd = GetWidget()->GetNativeView();
This code could be made platform independant with something like:
FocusManager* focus_manager = GetFocusManager();
if (focus_manager)
  focus_manager->FocusNativeView(GetWidget()->GetNativeView());
else
  NOTREACHED();
}

[DaveMoore, 2009-09-09 21:42:24]

Fixed issues. Also changed Textfield::Keystroke to be a class that uses the new
keycode stuff. I made the enum in keycodes be a typedef so that Keystroke could
use it in its api. This allowed me to make more of the find bar code be shared
across win and gtk.

This caused the change to grow in the number of files as the Textfield api
changed, but most are one-line changes.

http://codereview.chromium.org/200035/diff/6014/6034
File chrome/browser/views/find_bar_host.h (right):

http://codereview.chromium.org/200035/diff/6014/6034#newcode34
Line 34: // find-in-page functionality. It uses the FindBarHost implementation
to
On 2009/09/08 16:19:42, sky wrote:
> This second sentence doesn't seem right (it wasn't right before either).
Doesn't
> 'it' here refer to the FindBarHost, so that this reads as 'FindBarHost uses
the
> FindBarHost implementation...'

Done.

http://codereview.chromium.org/200035/diff/6014/6021
File chrome/browser/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/200035/diff/6014/6021#newcode23
Line 23: #include "views/widget/widget_gtk.h"
On 2009/09/08 16:19:42, sky wrote:
> This looks like more includes than you need.

Done.

http://codereview.chromium.org/200035/diff/6014/6031
File chrome/browser/views/find_bar_host_win.cc (right):

http://codereview.chromium.org/200035/diff/6014/6031#newcode21
Line 21: #include "views/widget/widget_win.h"
On 2009/09/08 16:19:42, sky wrote:
> This looks like more includes than you need.

Done.

http://codereview.chromium.org/200035/diff/6014/6031#newcode195
Line 195: DCHECK(!esc_accel_target_registered_);
On 2009/09/08 16:19:42, sky wrote:
> Can't this be common to both? Jay make the VK constants portable
> (base::VKEY_...) by way of base/keyboard_codes.h

Done.

http://codereview.chromium.org/200035/diff/6014/6031#newcode202
Line 202: DCHECK(esc_accel_target_registered_);
On 2009/09/08 16:19:42, sky wrote:
> Can this be common too?

Done.

http://codereview.chromium.org/200035/diff/6014/6018
File views/controls/textfield/native_textfield_gtk.h (right):

http://codereview.chromium.org/200035/diff/6014/6018#newcode48
Line 48: GtkWidget* entry,
On 2009/09/08 16:19:42, sky wrote:
> If these wrapped lines won't align at the (, then indent them 4.

Done.

http://codereview.chromium.org/200035/diff/6014/6018#newcode55
Line 55: gboolean OnChanged();
On 2009/09/08 16:19:42, sky wrote:
> nit: newline between this and DISALLOW.

Done.

http://codereview.chromium.org/200035/diff/6014/6019
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/200035/diff/6014/6019#newcode174
Line 174: #if defined(OS_WIN)
On 2009/09/08 16:19:42, sky wrote:
> Can this be key.key == base::VKEY_RETURN?

Done.

http://codereview.chromium.org/200035/diff/6014/6019#newcode183
Line 183: #if defined(OS_WIN)
On 2009/09/08 16:19:42, sky wrote:
> Can this be key.key == base::VKEY_ESCAPE?

Done.

http://codereview.chromium.org/200035/diff/6014/6015
File views/widget/root_view.cc (right):

http://codereview.chromium.org/200035/diff/6014/6015#newcode368
Line 368: if (!gtk_widget_is_focus(widget)) {
On 2009/09/08 16:19:42, sky wrote:
> nit: no {}

Done.

[sky, 2009-09-09 22:12:52]

http://codereview.chromium.org/200035/diff/1023/2065
File chrome/browser/views/find_bar_host.cc (right):

http://codereview.chromium.org/200035/diff/1023/2065#newcode313
Line 313: #if defined(OS_WIN)
Can the whole #if be nuked now?

http://codereview.chromium.org/200035/diff/1023/2069
File chrome/browser/views/find_bar_host.h (right):

http://codereview.chromium.org/200035/diff/1023/2069#newcode147
Line 147: views::Widget* CreateHost();
'Allows implementation' sounds odd here. Why not just 'Creates and returns the
native Widget.'

http://codereview.chromium.org/200035/diff/1023/2069#newcode149
Line 149: void SetDialogPositionNative(const gfx::Rect& new_pos, bool
no_redraw);
nit: I would call these Impl methods, but if you really like native keep it.

http://codereview.chromium.org/200035/diff/1023/2069#newcode156
Line 156: const TabContents* contents,
nit: 4 space indent.

http://codereview.chromium.org/200035/diff/1023/2057
File chrome/browser/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/200035/diff/1023/2057#newcode18
Line 18: : WidgetGtk(TYPE_CHILD),
4 space indent this.

http://codereview.chromium.org/200035/diff/1023/2057#newcode37
Line 37: // don't have to implement it for gtk
I would make these TODOs NOTIMPLEMENTED so that we don't miss them.

http://codereview.chromium.org/200035/diff/1023/2066
File chrome/browser/views/find_bar_host_win.cc (right):

http://codereview.chromium.org/200035/diff/1023/2066#newcode174
Line 174: new_pos.width(), new_pos.height(), swp_flags);
align with ( on previous line.

http://codereview.chromium.org/200035/diff/1023/2066#newcode182
Line 182: find_bar_controller_->tab_contents()->view()->GetNativeView(),
4 space indent these.

http://codereview.chromium.org/200035/diff/1023/2073
File chrome/chrome.gyp (right):

http://codereview.chromium.org/200035/diff/1023/2073#newcode1963
Line 1963: 'browser/views/find_bar_host.cc',
find_bar_host should be before find_bar_view.

http://codereview.chromium.org/200035/diff/1023/2053
File views/controls/textfield/native_textfield_gtk.cc (right):

http://codereview.chromium.org/200035/diff/1023/2053#newcode147
Line 147: bool handled = false;
nit: nuke the handled and just return from within HandleKeystroke. If you love
the ternary operator use it.

http://codereview.chromium.org/200035/diff/1023/2053#newcode164
Line 164: if (controller) {
no {}

http://codereview.chromium.org/200035/diff/1023/2056
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/200035/diff/1023/2056#newcode50
Line 50: unsigned int message;
If you're going to make this a class, then the fields should all be private with
accessors.

[DaveMoore, 2009-09-09 22:43:57]

Issues fixed.

http://codereview.chromium.org/200035/diff/1023/2065
File chrome/browser/views/find_bar_host.cc (right):

http://codereview.chromium.org/200035/diff/1023/2065#newcode313
Line 313: #if defined(OS_WIN)
On 2009/09/09 22:12:52, sky wrote:
> Can the whole #if be nuked now?

Done.

http://codereview.chromium.org/200035/diff/1023/2069
File chrome/browser/views/find_bar_host.h (right):

http://codereview.chromium.org/200035/diff/1023/2069#newcode147
Line 147: views::Widget* CreateHost();
On 2009/09/09 22:12:52, sky wrote:
> 'Allows implementation' sounds odd here. Why not just 'Creates and returns the
> native Widget.'

Done.

http://codereview.chromium.org/200035/diff/1023/2069#newcode156
Line 156: const TabContents* contents,
On 2009/09/09 22:12:52, sky wrote:
> nit: 4 space indent.

Done.

http://codereview.chromium.org/200035/diff/1023/2057
File chrome/browser/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/200035/diff/1023/2057#newcode18
Line 18: : WidgetGtk(TYPE_CHILD),
On 2009/09/09 22:12:52, sky wrote:
> 4 space indent this.

Done.

http://codereview.chromium.org/200035/diff/1023/2057#newcode37
Line 37: // don't have to implement it for gtk
On 2009/09/09 22:12:52, sky wrote:
> I would make these TODOs NOTIMPLEMENTED so that we don't miss them.

Done.

http://codereview.chromium.org/200035/diff/1023/2066
File chrome/browser/views/find_bar_host_win.cc (right):

http://codereview.chromium.org/200035/diff/1023/2066#newcode174
Line 174: new_pos.width(), new_pos.height(), swp_flags);
On 2009/09/09 22:12:52, sky wrote:
> align with ( on previous line.

Done.

http://codereview.chromium.org/200035/diff/1023/2066#newcode182
Line 182: find_bar_controller_->tab_contents()->view()->GetNativeView(),
On 2009/09/09 22:12:52, sky wrote:
> 4 space indent these.

Done.

http://codereview.chromium.org/200035/diff/1023/2073
File chrome/chrome.gyp (right):

http://codereview.chromium.org/200035/diff/1023/2073#newcode1963
Line 1963: 'browser/views/find_bar_host.cc',
On 2009/09/09 22:12:52, sky wrote:
> find_bar_host should be before find_bar_view.

Done.

http://codereview.chromium.org/200035/diff/1023/2053
File views/controls/textfield/native_textfield_gtk.cc (right):

http://codereview.chromium.org/200035/diff/1023/2053#newcode147
Line 147: bool handled = false;
On 2009/09/09 22:12:52, sky wrote:
> nit: nuke the handled and just return from within HandleKeystroke. If you love
> the ternary operator use it.

Done.

http://codereview.chromium.org/200035/diff/1023/2053#newcode164
Line 164: if (controller) {
On 2009/09/09 22:12:52, sky wrote:
> no {}

Done.

http://codereview.chromium.org/200035/diff/1023/2056
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/200035/diff/1023/2056#newcode50
Line 50: unsigned int message;
On 2009/09/09 22:12:52, sky wrote:
> If you're going to make this a class, then the fields should all be private
with
> accessors.

Done.

[sky, 2009-09-09 22:49:41]

LGTM with the following fixed.

http://codereview.chromium.org/200035/diff/3071/4115
File chrome/browser/views/find_bar_host.h (right):

http://codereview.chromium.org/200035/diff/3071/4115#newcode156
Line 156: const TabContents* contents,
It looks like you're at 5 now.

http://codereview.chromium.org/200035/diff/3071/4102
File views/controls/textfield/textfield.h (right):

http://codereview.chromium.org/200035/diff/3071/4102#newcode72
Line 72: #endif
DISALLOW_COPY_AND_ASSIGN

[Finnur, 2009-09-21 17:11:47]

http://codereview.chromium.org/200035/diff/3071/4108
File chrome/browser/views/find_bar_win.cc (left):

http://codereview.chromium.org/200035/diff/3071/4108#oldcode300
Line 300: return false;
Dave, this code got dropped when this was moved to find_bar_host.cc, which
reintroduces bug 10509 listed in the comment above. Can you take a look?