Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
97013002:
[Input View] Makes the input view window support window.resizeTo() and w3c visibility API its web c… (Closed)
|
Created:
7 years ago by Shu Chen Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Input View] Makes the input view window support window.resizeTo() and w3c visibility API in its web contents.
BUG=chromium:316524
TEST=Locally verified in sandbox.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240323
Patch Set 1 #
Total comments: 6
Patch Set 2 : Refacto'ed: removed KeyboardResizer, using static variables. And consider using variable height in … #Patch Set 3 : Added some comments. #Patch Set 4 : Notifies keyboard observer for keyboard resizing. #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : Fixed a bug. #Patch Set 8 : . #Patch Set 9 : #
Total comments: 14
Patch Set 10 : Modified per review comments. #Patch Set 11 : #
Total comments: 6
Patch Set 12 : Modified per comments. #Patch Set 13 : Fix unit tests failure. #Patch Set 14 : . #
Total comments: 2
Patch Set 15 : Fixed ash_unittests break. #Patch Set 16 : . #
Total comments: 22
Patch Set 17 : Modified per comments. #Patch Set 18 : Add an unit test in keyboard_unittests. #
Total comments: 15
Patch Set 19 : Modified per comments. #Patch Set 20 : . #
Total comments: 14
Patch Set 21 : . #Patch Set 22 : rebased. #
Total comments: 20
Patch Set 23 : . #Messages
Total messages: 31 (0 generated)
Added Sadrul as a fellow reviewer, as he is more familiar with the keyboard window code and Ash in particular. https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller.cc:169: container_->SetLayoutManager(layout_manager); It looks a bit odd passing a raw pointer to 2 separate objects. Appears to be safe in this case only because the container and proxy have the same lifetime. Perhaps a comment is warranted. https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller_proxy.cc:161: GetKeyboardWindow()->Show(); Is it necessary to show/hide the keyboard window, or is it sufficient to show/hide the container? https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller_proxy.h:32: virtual void SetKeyboardHeight(int height) = 0; Please document the method including units for the height. I believe that the height will be in logical (device independent) pixels rather that physical pixels. Makes a difference on high-dpi devices.
Kevin/Sadrul, I've refactor'ed my change. Please take a new look. Thanks. https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller.cc:169: container_->SetLayoutManager(layout_manager); On 2013/11/29 20:26:05, kevers wrote: > It looks a bit odd passing a raw pointer to 2 separate objects. Appears to be > safe in this case only because the container and proxy have the same lifetime. > Perhaps a comment is warranted. Done. I've refactored the changes and removed the keyboard resizer. https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller_proxy.cc:161: GetKeyboardWindow()->Show(); On 2013/11/29 20:26:05, kevers wrote: > Is it necessary to show/hide the keyboard window, or is it sufficient to > show/hide the container? It is necessary, otherwise it won't trigger the visibility change event which can be handled by document.addEventListener('visibilitychange', ...) in content JS. https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/97013002/diff/1/ui/keyboard/keyboard_controll... ui/keyboard/keyboard_controller_proxy.h:32: virtual void SetKeyboardHeight(int height) = 0; On 2013/11/29 20:26:05, kevers wrote: > Please document the method including units for the height. I believe that the > height will be in logical (device independent) pixels rather that physical > pixels. Makes a difference on high-dpi devices. I've refactored the changes and removed the keyboard resizer. And I've added comments as appropriate in the new patch set.
https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:76: gfx::Rect keyboard_bounds = KeyboardBoundsFromWindowBounds(bounds_); Won't bounds_ be properly updated by OnBoundsChanged? If there is a synchronization problem here, the we should investigate why OnBoundsChanged is not getting called. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:126: const gfx::Rect& requested_bounds) OVERRIDE { Chatted with Sadrul, and the main reason we had this as a no-op initially was to safeguard against outside sources resizing the contents. In this case, we want to allow resizing from Javascript so it makes sense to flip a flag in the proxy to indicate that resizing is allowed on the first call to resize the web contents. Sadrul's suggested that an auto-reset of the switch was likely not required, though we could reset on a web content reload. The controller has access to the proxy, so we shouldn't need a static variable. OnWindowResized should also check the state of the flag in the proxy to avoid a double update. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.h:67: const gfx::Rect& new_bounds, bool force = false); Default argument values are not allowed in the style guide, except in limited cases which don't apply here. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:79: content::WebContents* source, const gfx::Rect& pos) { OVERRIDE https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:89: keyboard::KeyboardControllerProxy::keyboard_resizing_from_contents = true; Can set a non-static member and add a public accessor method. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:91: keyboard::KeyboardControllerProxy::keyboard_resizing_from_contents = false; Resetting might not be strictly required. See other notes. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.h:69: static int keyboard_height; Please avoid using public static data members. KeyboardLayoutManager already has access to the KeyboardController and thus indirect access to the ControllerProxy. In addition, I don't think we should be caching persistent information in a proxy.
Thanks for review. Please see my changes in the latest patch set. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:76: gfx::Rect keyboard_bounds = KeyboardBoundsFromWindowBounds(bounds_); On 2013/12/03 00:08:12, kevers wrote: > Won't bounds_ be properly updated by OnBoundsChanged? If there is a > synchronization problem here, the we should investigate why OnBoundsChanged is > not getting called. Actually with proxy_, bounds_ is not needed. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:126: const gfx::Rect& requested_bounds) OVERRIDE { On 2013/12/03 00:08:12, kevers wrote: > Chatted with Sadrul, and the main reason we had this as a no-op initially was to > safeguard against outside sources resizing the contents. In this case, we want > to allow resizing from Javascript so it makes sense to flip a flag in the proxy > to indicate that resizing is allowed on the first call to resize the web > contents. Sadrul's suggested that an auto-reset of the switch was likely not > required, though we could reset on a web content reload. > > The controller has access to the proxy, so we shouldn't need a static variable. > OnWindowResized should also check the state of the flag in the proxy to avoid a > double update. Done. I've changed it to non-static. What do you mean by "auto-reset of the switch"? I've changed it to that set the flag in KeyboardContentsDelegate::MoveContents() and reset the flag in KeyboardLayoutManager::SetChildBounds(). https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.h:67: const gfx::Rect& new_bounds, bool force = false); On 2013/12/03 00:08:12, kevers wrote: > Default argument values are not allowed in the style guide, except in limited > cases which don't apply here. Done. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:79: content::WebContents* source, const gfx::Rect& pos) { On 2013/12/03 00:08:12, kevers wrote: > OVERRIDE Done. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:89: keyboard::KeyboardControllerProxy::keyboard_resizing_from_contents = true; On 2013/12/03 00:08:12, kevers wrote: > Can set a non-static member and add a public accessor method. Done. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:91: keyboard::KeyboardControllerProxy::keyboard_resizing_from_contents = false; On 2013/12/03 00:08:12, kevers wrote: > Resetting might not be strictly required. See other notes. Done. https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/97013002/diff/150001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.h:69: static int keyboard_height; On 2013/12/03 00:08:12, kevers wrote: > Please avoid using public static data members. KeyboardLayoutManager already > has access to the KeyboardController and thus indirect access to the > ControllerProxy. > > In addition, I don't think we should be caching persistent information in a > proxy. Done.
https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:109: controller_->NotifyKeyboardBoundsChanged(keyboard_bounds); Is NotifyKeyboardBoundsChanged going to fire twice for the same resize event: once in SetChildBounds and once here? Possible that this notification and the call to SetChildBoundsDirect should only be called if no resizing from contents. https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:186: NotifyKeyboardBoundsChanged(gfx::Rect()); Will we now get extra notifications: here and when we pick up the size change? There is also a verb tense change that is confusing. We call notify changed, but this dispatches to the observers that the size is changing. The notifications here, were firing before that actual size change fires. We're also calling the same notifications after the size change takes place. If we need notifications after the change, we should be calling a separate method on the observer. https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:248: NotifyKeyboardBoundsChanged(container_->children()[0]->bounds()); See previous comment.
Cl updated per comments. https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:109: controller_->NotifyKeyboardBoundsChanged(keyboard_bounds); On 2013/12/03 02:53:07, kevers wrote: > Is NotifyKeyboardBoundsChanged going to fire twice for the same resize event: > once in SetChildBounds and once here? Possible that this notification and the > call to SetChildBoundsDirect should only be called if no resizing from contents. Done. I've removed this line and add comments. https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:186: NotifyKeyboardBoundsChanged(gfx::Rect()); On 2013/12/03 02:53:07, kevers wrote: > Will we now get extra notifications: here and when we pick up the size change? > There is also a verb tense change that is confusing. We call notify changed, > but this dispatches to the observers that the size is changing. The > notifications here, were firing before that actual size change fires. We're also > calling the same notifications after the size change takes place. If we need > notifications after the change, we should be calling a separate method on the > observer. Done. I've renamed the method to NotifyKeyboardBoundsChanging. Let's do a separated cl to address your comments about supporting both BoundsChanging and BoundsChanged calls in KeyboardObserver. https://codereview.chromium.org/97013002/diff/180001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:248: NotifyKeyboardBoundsChanged(container_->children()[0]->bounds()); On 2013/12/03 02:53:07, kevers wrote: > See previous comment. Done.
This patch breaks VirtualKeyboardRootWindowControllerTest.ClickVirtualKeyboardInBlockedWindow, which is part of ash_unittests. I'll do some digging to see if I can pin down the problem. Possible that we're not initializing the keyboard size correctly. https://codereview.chromium.org/97013002/diff/230001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/230001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:74: virtual bool IsPopupOrPanel(const content::WebContents* source) const { OVERRIDE required for clang compiler
I've modified the cl per comments and also fixed the test breaks. The test fix is kind of a hack, and I've added some comments for it. Please let me know if you're ok with it. Thanks, Shu https://codereview.chromium.org/97013002/diff/230001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/230001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:74: virtual bool IsPopupOrPanel(const content::WebContents* source) const { On 2013/12/03 22:11:34, kevers wrote: > OVERRIDE required for clang compiler Done.
lgtm with nits. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:110: // changed here. Nit: Find this comment more confusing than helpful. Consider removing. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:87: // same keyboard bounds as the hit test mask. Nit: Obsolete comment. Please remove.
You should add some tests to keyboard_unittests https://codereview.chromium.org/97013002/diff/270001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/270001/ash/root_window_controll... ash/root_window_controller_unittest.cc:679: ->SetBounds(keyboard_window->bounds()); It's not clear to me why you need a new window for |keyboard_window|. Can't you do: keyboard_window = ...->proxy()->GetKeyboardWindow(); instead (and install an aura::test::TestEventHandler (or some other handler) on it to detect the mouse-clicks)? https://codereview.chromium.org/97013002/diff/270001/ash/shell/keyboard_contr... File ash/shell/keyboard_controller_proxy_stub.h (right): https://codereview.chromium.org/97013002/diff/270001/ash/shell/keyboard_contr... ash/shell/keyboard_controller_proxy_stub.h:34: aura::Window* keyboard_; Should this be a scoped_ptr<> (since the comment on KeyboardControllerProxy::GetKeyboardWindow() claims the proxy is expected to retain the ownership of the window)? https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:45: KeyboardWindowDelegate(keyboard::KeyboardControllerProxy* proxy) explicit https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:99: KeyboardLayoutManager(KeyboardController* controller) explicit here too https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:110: // changed here. On 2013/12/04 15:18:28, kevers wrote: > Nit: Find this comment more confusing than helpful. Consider removing. +1 https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:111: SetChildBoundsDirect(keyboard_, keyboard_bounds); We should not be doing this when proxy::resizing_from_contents() is set, right? https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:130: controller_->proxy()->set_resizing_from_contents(false); Why reset here? https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:131: } I think you should just do keyboard_->SetBounds() in OnWindowResized(), and call SetChildBoundsDirect() in here, with |requested_bounds| if resizing_from_contents(), and with KeyboardBoundsFromWindowBounds() otherwise. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.h:66: void NotifyKeyboardBoundsChanging(const gfx::Rect& new_bounds); This should be a private/protected method
I've modified cl per comments and added an unit test in keyboard_unittests. https://codereview.chromium.org/97013002/diff/270001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/270001/ash/root_window_controll... ash/root_window_controller_unittest.cc:679: ->SetBounds(keyboard_window->bounds()); On 2013/12/04 17:35:20, sadrul wrote: > It's not clear to me why you need a new window for |keyboard_window|. Can't you > do: > keyboard_window = ...->proxy()->GetKeyboardWindow(); > instead (and install an aura::test::TestEventHandler (or some other handler) on > it to detect the mouse-clicks)? Done. https://codereview.chromium.org/97013002/diff/270001/ash/shell/keyboard_contr... File ash/shell/keyboard_controller_proxy_stub.h (right): https://codereview.chromium.org/97013002/diff/270001/ash/shell/keyboard_contr... ash/shell/keyboard_controller_proxy_stub.h:34: aura::Window* keyboard_; On 2013/12/04 17:35:20, sadrul wrote: > Should this be a scoped_ptr<> (since the comment on > KeyboardControllerProxy::GetKeyboardWindow() claims the proxy is expected to > retain the ownership of the window)? Done. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:45: KeyboardWindowDelegate(keyboard::KeyboardControllerProxy* proxy) On 2013/12/04 17:35:20, sadrul wrote: > explicit Done. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:99: KeyboardLayoutManager(KeyboardController* controller) On 2013/12/04 17:35:20, sadrul wrote: > explicit here too Done. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:110: // changed here. On 2013/12/04 15:18:28, kevers wrote: > Nit: Find this comment more confusing than helpful. Consider removing. Done. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:110: // changed here. On 2013/12/04 17:35:20, sadrul wrote: > On 2013/12/04 15:18:28, kevers wrote: > > Nit: Find this comment more confusing than helpful. Consider removing. > > +1 Done. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:111: SetChildBoundsDirect(keyboard_, keyboard_bounds); On 2013/12/04 17:35:20, sadrul wrote: > We should not be doing this when proxy::resizing_from_contents() is set, right? I think it won't happen because the flag resizing_from_contents() is supposed to be auto-reset from MoveContents() to SetChildBounds(). OnWindowResized() shouldn't be called in the middle. Anyway, I've removed the implementation of OnWindowResized() per your comments in SetChildBounds(). https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:130: controller_->proxy()->set_resizing_from_contents(false); On 2013/12/04 17:35:20, sadrul wrote: > Why reset here? After finishing resizing the web content window, we should reset the flag. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:131: } On 2013/12/04 17:35:20, sadrul wrote: > I think you should just do keyboard_->SetBounds() in OnWindowResized(), and call > SetChildBoundsDirect() in here, with |requested_bounds| if > resizing_from_contents(), and with KeyboardBoundsFromWindowBounds() otherwise. When resizing from contents, the code flow is WebContentsDelegate::MoveContents() -> aura::LayoutManager::SetChildBounds(), which is nothing to do with OnWindowResized() here. OnWindowResized() is called when the keyboard container window is resized (e.g. screen rotate, etc.). I found that when OnWindowResized() happens, SetChildBounds() also happens. Therefore, OnWindowResized() is useless, we only need to implement SetChildBounds(). https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.h:66: void NotifyKeyboardBoundsChanging(const gfx::Rect& new_bounds); On 2013/12/04 17:35:20, sadrul wrote: > This should be a private/protected method This should be public because KeyboardLayoutManager need to call it. https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/270001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:87: // same keyboard bounds as the hit test mask. On 2013/12/04 15:18:28, kevers wrote: > Nit: Obsolete comment. Please remove. Done.
Btw, I cannot verify the latest patch set on chrome sandbox via "ssh -X ...". Kevin, can you please help me verify it with my testing IME extension? Thanks, Shu
https://codereview.chromium.org/97013002/diff/310001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/310001/ash/root_window_controll... ash/root_window_controller_unittest.cc:641: keyboard_window, gfx::Rect()); Why not set the bound directly on keyboard_window? https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} Why did you remove this code? This would still be useful if/when the container window is resized. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:121: controller_->proxy()->set_resizing_from_contents(false); Do not reset the state here. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:155: container_->children()[0] == proxy_->GetKeyboardWindow()) { if (container_->Contains(GetKeyboardWindow())) https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:178: if (proxy_ && proxy_->GetKeyboardWindow()->IsVisible()) { Should this be a CHECK? https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:87: keyboard->SetBounds(bounds); reset the state to false in here instead. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_unittest.cc:344: EXPECT_EQ(180, keyboard_window->bounds().height()); Add some code here to verify that calling keyboard_window->SetBounds() before calling set_resizing_from_contents(true) does not change the size.
Sadrul, thanks for the review. Pls see my latest patch set. For your comments about OnWindowResized() & SetChildBounds(), I couldn't verify it on sandbox because I can only ssh to my workstation and sandbox has problem for "ssh -X". But I recall SetChildBounds() gets called when container is resized. https://codereview.chromium.org/97013002/diff/310001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/310001/ash/root_window_controll... ash/root_window_controller_unittest.cc:641: keyboard_window, gfx::Rect()); On 2013/12/05 07:54:55, sadrul wrote: > Why not set the bound directly on keyboard_window? Done. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} On 2013/12/05 07:54:55, sadrul wrote: > Why did you remove this code? This would still be useful if/when the container > window is resized. As I mentioned, SetChildBounds() also gets called when container gets resized. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:121: controller_->proxy()->set_resizing_from_contents(false); On 2013/12/05 07:54:55, sadrul wrote: > Do not reset the state here. Done. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:155: container_->children()[0] == proxy_->GetKeyboardWindow()) { On 2013/12/05 07:54:55, sadrul wrote: > if (container_->Contains(GetKeyboardWindow())) Done. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:178: if (proxy_ && proxy_->GetKeyboardWindow()->IsVisible()) { On 2013/12/05 07:54:55, sadrul wrote: > Should this be a CHECK? Done. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:87: keyboard->SetBounds(bounds); On 2013/12/05 07:54:55, sadrul wrote: > reset the state to false in here instead. Done. https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_unittest.cc:344: EXPECT_EQ(180, keyboard_window->bounds().height()); On 2013/12/05 07:54:55, sadrul wrote: > Add some code here to verify that calling keyboard_window->SetBounds() before > calling set_resizing_from_contents(true) does not change the size. Done.
https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/310001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:178: if (proxy_ && proxy_->GetKeyboardWindow()->IsVisible()) { On 2013/12/05 08:21:04, Shu Chen wrote: > On 2013/12/05 07:54:55, sadrul wrote: > > Should this be a CHECK? > > Done. After changing this to CHECK, some tests broken. Bounds can be changed when window is hidden, in this case, we should mute the notification instead of crash chrome. Therefore, I changed it back.
On 2013/12/05 07:48:28, Shu Chen wrote:
> Btw, I cannot verify the latest patch set on chrome sandbox via "ssh -X ...".
>
> Kevin, can you please help me verify it with my testing IME extension?
>
> Thanks,
> Shu
I see the following warning message when installing the latest version of the
sample IME extension:
'inputMethodPrivate' is not allowed for specified extension ID.
Having said that, the button labeled 'window.resizeTo()' appears to be working,
toggling between two view sizes.
Thanks Kevin, "inputMethodPrivate" is nothing to do with this cl. Sadrul, thanks for staying so late to do code review. And looking forward to your next round of comments. Thanks, Shu
https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (left): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:109: keyboard_ = child; Why did you remove this code? https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} Did you verify that the keyboard-window gets resized correctly when the container is resized with this change? What triggers that resize? https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:176: if (proxy_ && proxy_->GetKeyboardWindow()->IsVisible()) { DCHECK(proxy_) https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.h:72: // Gets the proxy. This comment is unnecessary
https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} On 2013/12/06 12:30:02, sadrul wrote: > Did you verify that the keyboard-window gets resized correctly when the > container is resized with this change? What triggers that resize? You can test this trivially in a test by changing the bounds of the container, and verifying that the bounds of the keyboard-window also changes when the container-bounds is changed.
just drive-up https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:96: // owner window. It starts to make sense that we move the KeyboardLayoutManager into a new file like ui/keyboard/keyboard_layout_manager.* I will do it once your patch lands. https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} Looks like the keyboard-window size is now set by SetChildBounds below when the bounds of the container changed. The reason SetChildBounds is called is because of the code in root_window_layout_manager here: https://code.google.com/p/chromium/codesearch/#chromium/src/ash/wm/root_windo... When the screen size changed, root_window_layout_manager try to SetBounds of top level containers and second level containers. For virtual keyboard, the second level container is the keyboard window. And it then triggers SetChildBounds being called in this layout manager. I am not sure if we should SetBounds of virtual keyboard window from root_window_layout_manager after screen resized though. On 2013/12/09 22:47:01, sadrul wrote: > On 2013/12/06 12:30:02, sadrul wrote: > > Did you verify that the keyboard-window gets resized correctly when the > > container is resized with this change? What triggers that resize? > > You can test this trivially in a test by changing the bounds of the container, > and verifying that the bounds of the keyboard-window also changes when the > container-bounds is changed.
https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} On 2013/12/10 21:18:08, bshe wrote: > Looks like the keyboard-window size is now set by SetChildBounds below when the > bounds of the container changed. > > The reason SetChildBounds is called is because of the code in > root_window_layout_manager here: > https://code.google.com/p/chromium/codesearch/#chromium/src/ash/wm/root_windo... > > When the screen size changed, root_window_layout_manager try to SetBounds of top > level containers and second level containers. For virtual keyboard, the second > level container is the keyboard window. And it then triggers SetChildBounds > being called in this layout manager. I am not sure if we should SetBounds of > virtual keyboard window from root_window_layout_manager after screen resized > though. We shouldn't depend on outside code (ash) to maintain the internal state (keyboard-window height ratio), when it's trivial to maintain the internal state from here.
https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (left): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:109: keyboard_ = child; On 2013/12/06 12:30:02, sadrul wrote: > Why did you remove this code? Done. https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:96: // owner window. On 2013/12/10 21:18:08, bshe wrote: > It starts to make sense that we move the KeyboardLayoutManager into a new file > like ui/keyboard/keyboard_layout_manager.* > I will do it once your patch lands. Thank you! https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} On 2013/12/09 22:47:01, sadrul wrote: > On 2013/12/06 12:30:02, sadrul wrote: > > Did you verify that the keyboard-window gets resized correctly when the > > container is resized with this change? What triggers that resize? > > You can test this trivially in a test by changing the bounds of the container, > and verifying that the bounds of the keyboard-window also changes when the > container-bounds is changed. Test added. https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:104: virtual void OnWindowResized() OVERRIDE {} On 2013/12/10 21:21:19, sadrul wrote: > On 2013/12/10 21:18:08, bshe wrote: > > Looks like the keyboard-window size is now set by SetChildBounds below when > the > > bounds of the container changed. > > > > The reason SetChildBounds is called is because of the code in > > root_window_layout_manager here: > > > https://code.google.com/p/chromium/codesearch/#chromium/src/ash/wm/root_windo... > > > > When the screen size changed, root_window_layout_manager try to SetBounds of > top > > level containers and second level containers. For virtual keyboard, the second > > level container is the keyboard window. And it then triggers SetChildBounds > > being called in this layout manager. I am not sure if we should SetBounds of > > virtual keyboard window from root_window_layout_manager after screen resized > > though. > > We shouldn't depend on outside code (ash) to maintain the internal state > (keyboard-window height ratio), when it's trivial to maintain the internal state > from here. I've modified the code to let OnWindowResized handle the resizing from container (when the flag is not set). Thanks https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:176: if (proxy_ && proxy_->GetKeyboardWindow()->IsVisible()) { On 2013/12/06 12:30:02, sadrul wrote: > DCHECK(proxy_) Done. https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/97013002/diff/350001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.h:72: // Gets the proxy. On 2013/12/06 12:30:02, sadrul wrote: > This comment is unnecessary Done.
This LGTM. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/97013002/370001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Added oshima@ as reviewer for changes under ui/ash/...
mostly style nits. https://codereview.chromium.org/97013002/diff/390001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/390001/ash/root_window_controll... ash/root_window_controller_unittest.cc:622: ->proxy()->GetKeyboardWindow(); move "->" previous line https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:78: : KeyboardBoundsFromWindowBounds(bounds_); move ":" previous line (line should not start with operator) https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:107: } nuke {} https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:154: if (container_.get()) { you can just do if (container_) { https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:160: } nuke {} https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:181: DCHECK(proxy_); you don't need this as smart pointer checks this. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:74: virtual bool IsPopupOrPanel(const content::WebContents* source) const new line after ( to be consistent with other places. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:80: content::WebContents* source, const gfx::Rect& pos) OVERRIDE { one argument per line for decl/definition. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.h:45: bool resizing_from_contents() { return resizing_from_contents_; } const https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_unittest.cc:340: aura::Window* keyboard_window(proxy()->GetKeyboardWindow()); nit: use = for consistency
oshima@, just modified the cl per your comments. Please take another look. Thanks https://codereview.chromium.org/97013002/diff/390001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/390001/ash/root_window_controll... ash/root_window_controller_unittest.cc:622: ->proxy()->GetKeyboardWindow(); On 2013/12/12 01:53:24, oshima wrote: > move "->" previous line Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:78: : KeyboardBoundsFromWindowBounds(bounds_); On 2013/12/12 01:53:24, oshima wrote: > move ":" previous line (line should not start with operator) Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:107: } On 2013/12/12 01:53:24, oshima wrote: > nuke {} Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:154: if (container_.get()) { On 2013/12/12 01:53:24, oshima wrote: > you can just do > if (container_) { Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:160: } On 2013/12/12 01:53:24, oshima wrote: > nuke {} Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller.cc:181: DCHECK(proxy_); On 2013/12/12 01:53:24, oshima wrote: > you don't need this as smart pointer checks this. Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.cc (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:74: virtual bool IsPopupOrPanel(const content::WebContents* source) const On 2013/12/12 01:53:24, oshima wrote: > new line after ( > to be consistent with other places. Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.cc:80: content::WebContents* source, const gfx::Rect& pos) OVERRIDE { On 2013/12/12 01:53:24, oshima wrote: > one argument per line for decl/definition. Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_proxy.h (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_proxy.h:45: bool resizing_from_contents() { return resizing_from_contents_; } On 2013/12/12 01:53:24, oshima wrote: > const Done. https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/97013002/diff/390001/ui/keyboard/keyboard_con... ui/keyboard/keyboard_controller_unittest.cc:340: aura::Window* keyboard_window(proxy()->GetKeyboardWindow()); On 2013/12/12 01:53:24, oshima wrote: > nit: use = for consistency Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shuchen@chromium.org/97013002/410001
Message was sent while issue was closed.
Change committed as 240323 |
