Adjust virtual keyboard size base on input method candidates.

Adjust virtual keyboard size base on input method candidates.

Known issue:
Because we use javascript to show/hide candidates and re-size the virtual keyboard container, so it is difficult to make them happen at same time. When we hide the candidates and shrink the keyboard container, you may see two steps: 1. Candidates are hidden and keyboard jumps up. 2. The container is shrank.

Solution:
We plan to make virtual keyboard as a toplevel window instead of a child view. It will overlay the browser. And we will make candidates always visible in html and use SetBounds (0, -height_of_candidates, w, h) to hide it.


BUG=None
TEST=Manually


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

[bryeung, 2011-06-03 19:14:52]

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
File chrome/browser/resources/keyboard/ime.js (right):

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
chrome/browser/resources/keyboard/ime.js:183: var should_visible =
maybe just visible or visible_now?

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
chrome/browser/resources/keyboard/ime.js:194: else {
nit: move to line above

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
File chrome/browser/resources/keyboard/main.js (right):

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
chrome/browser/resources/keyboard/main.js:651: var oldX = 0;
I'm confused why this is necessary.  Won't one of these always have changed when
window.onresize gets called?

http://codereview.chromium.org/7058046/diff/13/chrome/browser/ui/touch/frame/...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7058046/diff/13/chrome/browser/ui/touch/frame/...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:31: const int
kKeyboardHeight = 300;
please rename to kDefaultKeyboardHeight

http://codereview.chromium.org/7058046/diff/13/chrome/browser/ui/touch/frame/...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:295: if (height !=
keyboard_height_) {
we need to do more error checking here, e.g.:
 - don't allow negative heights (set to 0)
 - don't allow heights larger than the view size

We may even want to consider restricting the keyboard to be at most 60% or 70%
of the view height at a maximum.

http://codereview.chromium.org/7058046/diff/13/chrome/common/extensions/api/e...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7058046/diff/13/chrome/common/extensions/api/e...
chrome/common/extensions/api/extension_api.json:2448: "description": "This
function is called when the event processing is completed.",
Does this mean that it is only called when the view is actually resized?  It
seems that would be helpful, but it may by too difficult to implement to be
worthwhile.

[Peng, 2011-06-03 22:22:09]

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
File chrome/browser/resources/keyboard/ime.js (right):

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
chrome/browser/resources/keyboard/ime.js:183: var should_visible =
On 2011/06/03 19:14:52, bryeung wrote:
> maybe just visible or visible_now?

Done.

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
chrome/browser/resources/keyboard/ime.js:194: else {
On 2011/06/03 19:14:52, bryeung wrote:
> nit: move to line above

Done.

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
File chrome/browser/resources/keyboard/main.js (right):

http://codereview.chromium.org/7058046/diff/13/chrome/browser/resources/keybo...
chrome/browser/resources/keyboard/main.js:651: var oldX = 0;
On 2011/06/03 19:14:52, bryeung wrote:
> I'm confused why this is necessary.  Won't one of these always have changed
when
> window.onresize gets called?

oldHeight is not the height of window. It may be the (window_height -
IME_HEIGHT) or (window_height - 0). They could be same in window.onresize().

http://codereview.chromium.org/7058046/diff/13/chrome/browser/ui/touch/frame/...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7058046/diff/13/chrome/browser/ui/touch/frame/...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:31: const int
kKeyboardHeight = 300;
On 2011/06/03 19:14:52, bryeung wrote:
> please rename to kDefaultKeyboardHeight

Done.

http://codereview.chromium.org/7058046/diff/13/chrome/browser/ui/touch/frame/...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:295: if (height !=
keyboard_height_) {
On 2011/06/03 19:14:52, bryeung wrote:
> we need to do more error checking here, e.g.:
>  - don't allow negative heights (set to 0)
>  - don't allow heights larger than the view size
> 
> We may even want to consider restricting the keyboard to be at most 60% or 70%
> of the view height at a maximum.

Done.

http://codereview.chromium.org/7058046/diff/13/chrome/common/extensions/api/e...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7058046/diff/13/chrome/common/extensions/api/e...
chrome/common/extensions/api/extension_api.json:2448: "description": "This
function is called when the event processing is completed.",
On 2011/06/03 19:14:52, bryeung wrote:
> Does this mean that it is only called when the view is actually resized?  It
> seems that would be helpful, but it may by too difficult to implement to be
> worthwhile.

Done.

[asargent_no_longer_on_chrome, 2011-06-04 00:26:07]

extensions parts LGTM

One high level comment for you to consider for this and several other methods I
noticed in your API implementation (no need to hold up this CL) - is there a way
to avoid having the C++ implementation of your extension functions use
Notifications? Is there a way to get a more specific handle to the interested
object and directly invoke a method on it or something?

[bryeung, 2011-06-07 22:45:03]

I agree with asargent@: we should find a way to avoid using notifications, but
that should be a separate CL.

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/resources/key...
File chrome/browser/resources/keyboard/main.js (right):

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard/main.js:660: if (newX != oldX || height !=
oldHeight) {
Sorry: I'm still not understanding.

Are you saying that window.onresize can be called even if the height has not
changed?  Or is that the height could have changed, but it might be offset by
the IME height, so no resize is necessary?  i.e. the extra window height is
exactly taken up by the candidates, so we don't need to resize the keys?

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/ui/touch/fram...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:299: keyboard_height_
= height;
Please do not set the height or call Layout if the height is invalid.

I would suggest putting an if statement immediately after getting the height to
check if it is valid (instead of the DCHECKs).  If it is invalid, you could log
an error.

[Peng, 2011-06-08 14:59:57]

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/resources/key...
File chrome/browser/resources/keyboard/main.js (right):

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/resources/key...
chrome/browser/resources/keyboard/main.js:660: if (newX != oldX || height !=
oldHeight) {
On 2011/06/07 22:45:03, bryeung wrote:
> Sorry: I'm still not understanding.
> 
> Are you saying that window.onresize can be called even if the height has not
> changed?  Or is that the height could have changed, but it might be offset by
> the IME height, so no resize is necessary?  i.e. the extra window height is
> exactly taken up by the candidates, so we don't need to resize the keys?

Yeah. Sometime, the window.onresize is caused by showing or hiding candidates.
In those case, the height of keys should be unchanged. So they do not need
resize.

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/ui/touch/fram...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7058046/diff/7001/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:299: keyboard_height_
= height;
On 2011/06/07 22:45:03, bryeung wrote:
> Please do not set the height or call Layout if the height is invalid.
> 
> I would suggest putting an if statement immediately after getting the height
to
> check if it is valid (instead of the DCHECKs).  If it is invalid, you could
log
> an error.

I added code in extension_input_api.cc to check the height to make sure it is
not less than 0.

For height > View::height(), I think we could keep using current code. In
future, we will refactory the CL to allow 
extension_input_api.cc access virtual keyboard directly. So we could check it
and return an error to javascript.

I added some TODO in code for them.

Done

[bryeung, 2011-06-08 15:06:57]

LGTM

Just two typos/nits.

http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:298: DCHECK_GE(height,
0) << "Height of the keyboar is less than 0.";
s/keyboar/keyboard/

http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:299: DCHECK_LE(height,
View::height()) << "Height of the keyboard is great "
s/great/greater/

[Peng, 2011-06-08 15:13:11]

http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):

http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:298: DCHECK_GE(height,
0) << "Height of the keyboar is less than 0.";
On 2011/06/08 15:06:57, bryeung wrote:
> s/keyboar/keyboard/

Done.

http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:299: DCHECK_LE(height,
View::height()) << "Height of the keyboard is great "
On 2011/06/08 15:06:57, bryeung wrote:
> s/great/greater/

Done.

[bryeung, 2011-06-08 15:16:18]

LGTM

On Wed, Jun 8, 2011 at 11:13 AM,  <penghuang@chromium.org> wrote:
>
>
http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
> File chrome/browser/ui/touch/frame/touch_browser_frame_view.cc (right):
>
>
http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:298:
> DCHECK_GE(height, 0) << "Height of the keyboar is less than 0.";
> On 2011/06/08 15:06:57, bryeung wrote:
>>
>> s/keyboar/keyboard/
>
> Done.
>
>
http://codereview.chromium.org/7058046/diff/8012/chrome/browser/ui/touch/fram...
> chrome/browser/ui/touch/frame/touch_browser_frame_view.cc:299:
> DCHECK_LE(height, View::height()) << "Height of the keyboard is great "
> On 2011/06/08 15:06:57, bryeung wrote:
>>
>> s/great/greater/
>
> Done.
>
> http://codereview.chromium.org/7058046/
>

[commit-bot: I haz the power, 2011-06-09 19:40:02]

Presubmit check for 7058046-9003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/common/notification_type.h

[Peng, 2011-06-09 19:46:48]

Hi John

[Peng, 2011-06-09 19:48:22]

Hi John, Could you please review the change in notification_type.h? Thanks.

[jam, 2011-06-09 21:48:05]

content lgtm

[commit-bot: I haz the power, 2011-06-09 22:56:53]

Change committed as 88605