Fix virtual keyboard on aura

Fix virtual keyboard on aura

Fix compile error
Fix to show virtual keyboard on linux build

BUG=chromium-os:10620
TEST=build success with GYP_DEFINES="use_virtual_keyboard=1"


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

[hashimoto, 2012-01-23 10:21:53]

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
File chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc (right):

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc:312:
SetBounds(bounds);
Replace SetTransform with SetBounds because virtual keyboard cannot be shown
correctly with SetTransform on desktop build.

[sadrul, 2012-01-23 22:47:57]

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
File chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc (right):

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc:312:
SetBounds(bounds);
On 2012/01/23 10:21:53, hashimoto wrote:
> Replace SetTransform with SetBounds because virtual keyboard cannot be shown
> correctly with SetTransform on desktop build.

I think this is a wrong thing to do. Changing the bounds will cause the keyboard
contents to relayout. Doing this for every animation step is going to be very
expensive, and will almost undoubtedly result in a very janky animation.

What is the reason SetTransform does not work in a desktop build?

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/webui/keyboar...
File chrome/browser/ui/webui/keyboard_ui.cc (right):

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/webui/keyboar...
chrome/browser/ui/webui/keyboard_ui.cc:9: #include "base/message_loop.h"
I don't think this is necessary?

[hashimoto, 2012-01-24 11:23:05]

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
File chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc (right):

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc:312:
SetBounds(bounds);
On 2012/01/23 22:47:57, sadrul wrote:
> On 2012/01/23 10:21:53, hashimoto wrote:
> > Replace SetTransform with SetBounds because virtual keyboard cannot be shown
> > correctly with SetTransform on desktop build.
> 
> I think this is a wrong thing to do. Changing the bounds will cause the
keyboard
> contents to relayout. Doing this for every animation step is going to be very
> expensive, and will almost undoubtedly result in a very janky animation.
Unless |keyboard_height_| is changed, the size of KeyboardWidget does not
change.
Simply moving a widget without changing its size looks not costing much and it
actually works smoothly on my Alex.

> 
> What is the reason SetTransform does not work in a desktop build?
I have no idea why the current code does not work but there are some known
limitations when using DomView, so I'm not surprised to see a code which worked
fine with RenderWidgetHostViewViews not working fine with
RenderWidgetHostViewAura.
(
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...
  URL above is a good example of a limitation of DomView, they have to create an
additional Widget to avoid a DomView's bug where adding a child to DomView will
break the displayed content.)

Adding to that, since this code is the only place where SetTransform is used
inside chrome directory (see the result of
http://cs.chromium.org/settransform%20file:^src/chrome), I think SetBounds is a
safer choice than SetTransform.

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/webui/keyboar...
File chrome/browser/ui/webui/keyboard_ui.cc (right):

http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/webui/keyboar...
chrome/browser/ui/webui/keyboard_ui.cc:9: #include "base/message_loop.h"
On 2012/01/23 22:47:57, sadrul wrote:
> I don't think this is necessary?

MessageLoop is used in line 37.

[hashimoto, 2012-01-26 02:33:20]

On 2012/01/24 11:23:05, hashimoto wrote:
>
http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
> File chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc (right):
> 
>
http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/virtual_keybo...
> chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc:312:
> SetBounds(bounds);
> On 2012/01/23 22:47:57, sadrul wrote:
> > On 2012/01/23 10:21:53, hashimoto wrote:
> > > Replace SetTransform with SetBounds because virtual keyboard cannot be
shown
> > > correctly with SetTransform on desktop build.
> > 
> > I think this is a wrong thing to do. Changing the bounds will cause the
> keyboard
> > contents to relayout. Doing this for every animation step is going to be
very
> > expensive, and will almost undoubtedly result in a very janky animation.
> Unless |keyboard_height_| is changed, the size of KeyboardWidget does not
> change.
> Simply moving a widget without changing its size looks not costing much and it
> actually works smoothly on my Alex.
> 
> > 
> > What is the reason SetTransform does not work in a desktop build?
> I have no idea why the current code does not work but there are some known
> limitations when using DomView, so I'm not surprised to see a code which
worked
> fine with RenderWidgetHostViewViews not working fine with
> RenderWidgetHostViewAura.
> (
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...
>   URL above is a good example of a limitation of DomView, they have to create
an
> additional Widget to avoid a DomView's bug where adding a child to DomView
will
> break the displayed content.)
> 
> Adding to that, since this code is the only place where SetTransform is used
> inside chrome directory (see the result of
> http://cs.chromium.org/settransform%2520file:%5Esrc/chrome), I think SetBounds
is a
> safer choice than SetTransform.
> 
>
http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/webui/keyboar...
> File chrome/browser/ui/webui/keyboard_ui.cc (right):
> 
>
http://codereview.chromium.org/9200011/diff/1/chrome/browser/ui/webui/keyboar...
> chrome/browser/ui/webui/keyboard_ui.cc:9: #include "base/message_loop.h"
> On 2012/01/23 22:47:57, sadrul wrote:
> > I don't think this is necessary?
> 
> MessageLoop is used in line 37.

ping?

[hashimoto, 2012-01-30 11:08:41]

sadrul,

Could you review the new patch set?

[hashimoto, 2012-01-30 11:09:27]

On 2012/01/30 11:08:41, hashimoto wrote:
> sadrul,
> 
> Could you review the new patch set?

Sorry, there's no new patch set.
Please see my comments.

[sadrul, 2012-01-31 05:34:08]

lgtm

[commit-bot: I haz the power, 2012-01-31 06:10:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9200011/1

[commit-bot: I haz the power, 2012-01-31 06:10:39]

Can't apply patch for file
chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc
Hunk #1 FAILED at 21.
Hunk #4 succeeded at 225 (offset -1 lines).
Hunk #5 succeeded at 240 (offset -1 lines).
Hunk #6 succeeded at 250 (offset -1 lines).
Hunk #7 succeeded at 306 (offset -1 lines).
1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/ui/virtual_keyboard/virtual_keyboard_manager.cc.rej

[commit-bot: I haz the power, 2012-01-31 09:04:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9200011/15002

[commit-bot: I haz the power, 2012-01-31 09:04:46]

Presubmit check for 9200011-15002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/ui/webui/keyboard_ui.cc

[hashimoto, 2012-01-31 09:20:18]

James,

Could you review keyboard_ui.cc as an OWNER?

[James Hawkins, 2012-01-31 18:18:23]

lgtm

[commit-bot: I haz the power, 2012-02-01 00:40:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/9200011/15002

[commit-bot: I haz the power, 2012-02-01 02:09:58]

Change committed as 120010