Add the virtual keyboard to BrowserFrameChromeos.

Add the virtual keyboard to BrowserFrameChromeos.

BUG=none
TEST=focusing a text field shows the placeholder keyboard

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

[sky, 2010-12-09 22:19:03]

http://codereview.chromium.org/5747001/diff/1/chrome/browser/chromeos/frame/b...
File chrome/browser/chromeos/frame/browser_frame_chromeos.cc (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/chromeos/frame/b...
chrome/browser/chromeos/frame/browser_frame_chromeos.cc:11: #if
defined(TOUCH_UI)
conditional includes should be grouped after non-conditional includes.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/chromeos/frame/b...
chrome/browser/chromeos/frame/browser_frame_chromeos.cc:44: #if
defined(TOUCH_UI)
Add a factory method/function some where so that you don't need the ifdef here.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/o...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:41: BrowserView*
GetBrowserView() const { return browser_view_; }
Inlined methods should be unix_hacker_style.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
File chrome/browser/ui/views/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:25:
registrar_.Add(this,
initialize keyboard_ to NULL here.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:40: gfx::Rect*
client_view_bounds = this->client_view_bounds();
Adjusting the height in this manner is subtle and easily broken. You're also
assuming it's ok to adjust the client height after the layout, which is brittle.

This code would be less fragile if you change
OpaqueBrowserFrameView::CalculateClientAreaBounds to call into a method to get
the bottom height that is overriden in this class.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:86:
GetParent()->Layout();
Shouldn't this result in a Layout on this class to? I guess I'm saying is 82
really necessary?

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:95: if
(is_editable_node != keyboard_->IsVisible()) {
Rather than having the check here, wouldn't it be nice to make
UpdateKeyboardAndLayout do nothing if the states already match?

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:98: } else {
You should at least have a DCHECK here on the type.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
File chrome/browser/ui/views/frame/touch_browser_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FRAME_TOUCH_BROWSER_FRAME_VIEW_H_
I suggested this to Chad as well. I think it's worth having ui/touch/... to make
it clear what files are used.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.h:40:
DISALLOW_COPY_AND_ASSIGN(TouchBrowserFrameView);
newline between 39 and 40.

[bryeung, 2010-12-10 17:23:19]

Thanks for the comments Scott.  I'm much happier with this CL now.  Hopefully
you are too :-)

http://codereview.chromium.org/5747001/diff/1/chrome/browser/chromeos/frame/b...
File chrome/browser/chromeos/frame/browser_frame_chromeos.cc (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/chromeos/frame/b...
chrome/browser/chromeos/frame/browser_frame_chromeos.cc:11: #if
defined(TOUCH_UI)
On 2010/12/09 22:19:03, sky wrote:
> conditional includes should be grouped after non-conditional includes.

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/chromeos/frame/b...
chrome/browser/chromeos/frame/browser_frame_chromeos.cc:44: #if
defined(TOUCH_UI)
On 2010/12/09 22:19:03, sky wrote:
> Add a factory method/function some where so that you don't need the ifdef
here.

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/o...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/o...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:41: BrowserView*
GetBrowserView() const { return browser_view_; }
On 2010/12/09 22:19:03, sky wrote:
> Inlined methods should be unix_hacker_style.

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
File chrome/browser/ui/views/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:25:
registrar_.Add(this,
On 2010/12/09 22:19:03, sky wrote:
> initialize keyboard_ to NULL here.

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:40: gfx::Rect*
client_view_bounds = this->client_view_bounds();
On 2010/12/09 22:19:03, sky wrote:
> Adjusting the height in this manner is subtle and easily broken. You're also
> assuming it's ok to adjust the client height after the layout, which is
brittle.
> 
> This code would be less fragile if you change
> OpaqueBrowserFrameView::CalculateClientAreaBounds to call into a method to get
> the bottom height that is overriden in this class.

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:86:
GetParent()->Layout();
On 2010/12/09 22:19:03, sky wrote:
> Shouldn't this result in a Layout on this class to? I guess I'm saying is 82
> really necessary?

Yep, you're right.  I'll use a smaller hammer :-)

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:95: if
(is_editable_node != keyboard_->IsVisible()) {
On 2010/12/09 22:19:03, sky wrote:
> Rather than having the check here, wouldn't it be nice to make
> UpdateKeyboardAndLayout do nothing if the states already match?

Yes: good idea.  Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.cc:98: } else {
On 2010/12/09 22:19:03, sky wrote:
> You should at least have a DCHECK here on the type.

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
File chrome/browser/ui/views/frame/touch_browser_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FRAME_TOUCH_BROWSER_FRAME_VIEW_H_
On 2010/12/09 22:19:03, sky wrote:
> I suggested this to Chad as well. I think it's worth having ui/touch/... to
make
> it clear what files are used. 

Done.

http://codereview.chromium.org/5747001/diff/1/chrome/browser/ui/views/frame/t...
chrome/browser/ui/views/frame/touch_browser_frame_view.h:40:
DISALLOW_COPY_AND_ASSIGN(TouchBrowserFrameView);
On 2010/12/09 22:19:03, sky wrote:
> newline between 39 and 40.

Done.

[sky, 2010-12-10 18:18:20]

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/browser_frame_chromeos.cc (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/browser_frame_chromeos.cc:34: bool is_popup =
browser_view()->IsBrowserTypePanel();
This doesn't match the old code. Does it matter?

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:74: // If we have not
created the keyboard yet, then we can safely skip
Wouldn't it be better to have this method call InitVirtualKeyboard if needed?

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:96:
UpdateKeyboardAndLayout(*Details<const bool>(details).ptr());
You're going to support multiple windows, right? You should only show the
keyboard if focus changed in this window.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:39: bool is_popup,
BrowserFrame* frame, BrowserView* browser_view);
Since you're passing in the browserview can we nuke the is_popup parameter? Same
thing with the BrowserFrame arg. Although maybe you need that because the frame
might not have been set on the browserview yet?

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:42: gfx::Rect*
client_view_bounds() { return &client_view_bounds_; }
Can this be nuked now?

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:45: // will add.
This should document where the height is reserved.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:46: virtual int
GetReservedHeight() const { return 0; }
don't inline virtual methods.

[bryeung, 2010-12-10 23:58:59]

PTAL

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/browser_frame_chromeos.cc (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/browser_frame_chromeos.cc:34: bool is_popup =
browser_view()->IsBrowserTypePanel();
On 2010/12/10 18:18:20, sky wrote:
> This doesn't match the old code. Does it matter?

Good catch!  Silly copy-paste error.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:74: // If we have not
created the keyboard yet, then we can safely skip
On 2010/12/10 18:18:20, sky wrote:
> Wouldn't it be better to have this method call InitVirtualKeyboard if needed?

If we try to InitVirtualKeyboard too early, it will crash because the extensions
system isn't ready.  I think it is better to ignore all the notifications
throughout startup, until the keyboard has been shown.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:96:
UpdateKeyboardAndLayout(*Details<const bool>(details).ptr());
On 2010/12/10 18:18:20, sky wrote:
> You're going to support multiple windows, right? You should only show the
> keyboard if focus changed in this window.

TOUCH_UI will not support multiple windows.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_non_client_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_non_client_frame_view.h:39: bool is_popup,
BrowserFrame* frame, BrowserView* browser_view);
On 2010/12/10 18:18:20, sky wrote:
> Since you're passing in the browserview can we nuke the is_popup parameter?
Same
> thing with the BrowserFrame arg. Although maybe you need that because the
frame
> might not have been set on the browserview yet?

Yes, I think you're right about is_popup.

For the frame: I assume there is a good reason that OpaqueBrowserFrameView takes
both parameters in the constructor, so I'd prefer to leave both the frame and
the view.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.h (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:42: gfx::Rect*
client_view_bounds() { return &client_view_bounds_; }
On 2010/12/10 18:18:20, sky wrote:
> Can this be nuked now?

Done.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:45: // will add.
On 2010/12/10 18:18:20, sky wrote:
> This should document where the height is reserved.

Done.

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.h:46: virtual int
GetReservedHeight() const { return 0; }
On 2010/12/10 18:18:20, sky wrote:
> don't inline virtual methods.

Done.

[sky, 2010-12-13 17:37:37]

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:74: // If we have not
created the keyboard yet, then we can safely skip
On 2010/12/10 23:58:59, bryeung wrote:
> On 2010/12/10 18:18:20, sky wrote:
> > Wouldn't it be better to have this method call InitVirtualKeyboard if
needed?
> 
> If we try to InitVirtualKeyboard too early, it will crash because the
extensions
> system isn't ready.  I think it is better to ignore all the notifications
> throughout startup, until the keyboard has been shown.
> 

My comment wasn't very good. I'm suggesting you make this invoke
InitVirtualKeyboard if should_show_keyboard is true. That way other places that
might need to show the keyboard only invoke this method and don't have to worry
about also needing to invoke InitVirtualKeyboard (as you do @ line 95).

http://codereview.chromium.org/5747001/diff/11001/chrome/browser/ui/touch/fra...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:96:
UpdateKeyboardAndLayout(*Details<const bool>(details).ptr());
On 2010/12/10 23:58:59, bryeung wrote:
> On 2010/12/10 18:18:20, sky wrote:
> > You're going to support multiple windows, right? You should only show the
> > keyboard if focus changed in this window.
> 
> TOUCH_UI will not support multiple windows.

Remember that panels/popups are also browser windows.
Also, it's a lot easier to add the check now rather than at some future point
when you know folks will change their mind (perhaps incognito) and you'll have
to go back and patch places like this.

[bryeung, 2010-12-13 19:21:27]

Sorry for the confusion about your comments.

PTAL

[sky, 2010-12-13 20:19:21]

LGTM