Resize BrowserFrameAsh and not RenderWidgetHostViewAura when keyboard bounds change.

Resize BrowserFrameAsh and not RenderWidgetHostViewAura when keyboard bounds change.

TextInputClient returns a BrowserFrameAsh is the omnibar is clicked, and RenderWidgetHostViewAura if a textfield on the page is clicked. We always want to resize BrowserFrameAsh.


TEST=WorkspaceLayoutManagerKeyboardTest.ChildWindowFocused
BUG=407094
Committed: https://crrev.com/00bff42f5738f985d04a888d64a64e802b65977d
Cr-Commit-Position: refs/heads/master@{#292004}

[rsadam, 2014-08-25 14:17:42]

Hi Rob, PTAL!

[rsadam, 2014-08-25 14:19:59]

Indent was off.

[flackr, 2014-08-25 14:26:06]

Can you add a unittest for this?

https://codereview.chromium.org/504793003/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/504793003/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:146: wm::WindowState*
toplevel_window_state = wm::GetWindowState(window);
nit: s/toplevel_window_state/window_state
Since it is the state of window now.

[rsadam, 2014-08-25 15:10:52]

Added a unittest!

https://codereview.chromium.org/504793003/diff/20001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/504793003/diff/20001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager.cc:146: wm::WindowState*
toplevel_window_state = wm::GetWindowState(window);
On 2014/08/25 14:26:06, flackr wrote:
> nit: s/toplevel_window_state/window_state
> Since it is the state of window now.

Done.

[flackr, 2014-08-25 15:25:30]

https://codereview.chromium.org/504793003/diff/40001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/504793003/diff/40001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1093:
EXPECT_EQ(occluded_window_bound.ToString(), window->bounds().ToString());
This test looks like it would pass on master as the text input client window is
the toplevel window. Don't you need to create a child window of a toplevel
window and set it as the focused text input client?

I recommend a new test for this btw.

[rsadam, 2014-08-25 18:26:31]

You're right! Created a separate test, and set the textinputclient to return the
child window. Verified that this fails without my patch.

[flackr, 2014-08-25 20:06:52]

Looks good, just a couple nits.

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1037:
TEST_F(WorkspaceLayoutManagerKeyboardTest, ChildWindowFocused) {
nit: Add a description, i.e.:
Tests that when a child window gains focus the top level window containing it is
resized to fit the remaining workspace area.

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1064: }
Can you make methods in WorkspaceLayoutManagerKeyboardTest for
Focus(TextInputClient or Window) and Blur() (lines 1079-1083) to avoid
duplicating this code in both tests?

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1070: gfx::Rect
small_window_bound(50, 50, 100, 500);
 nit: s/small_window_bound/initial_window_bounds

[rsadam, 2014-08-26 14:36:49]

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1037:
TEST_F(WorkspaceLayoutManagerKeyboardTest, ChildWindowFocused) {
On 2014/08/25 20:06:51, flackr wrote:
> nit: Add a description, i.e.:
> Tests that when a child window gains focus the top level window containing it
is
> resized to fit the remaining workspace area.

Done.

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1064: }
On 2014/08/25 20:06:51, flackr wrote:
> Can you make methods in WorkspaceLayoutManagerKeyboardTest for
> Focus(TextInputClient or Window) and Blur() (lines 1079-1083) to avoid
> duplicating this code in both tests?

Done.

https://codereview.chromium.org/504793003/diff/80001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1070: gfx::Rect
small_window_bound(50, 50, 100, 500);
On 2014/08/25 20:06:51, flackr wrote:
>  nit: s/small_window_bound/initial_window_bounds

Done.

[flackr, 2014-08-26 15:42:40]

LGTM with nits

https://codereview.chromium.org/504793003/diff/120001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/504793003/diff/120001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1018:
root_window->GetProperty(aura::client::kRootWindowInputMethodKey);
nit: move 1015 and 1017 to 1023 where used, similar to the blur method.

https://codereview.chromium.org/504793003/diff/120001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1090: gfx::Rect
initial_window_bound(50, 50, 100, 500);
nit: s/bound/bounds

[rsadam, 2014-08-26 15:45:53]

Fixed nits.

[rsadam, 2014-08-26 15:46:18]

https://codereview.chromium.org/504793003/diff/120001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/504793003/diff/120001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1018:
root_window->GetProperty(aura::client::kRootWindowInputMethodKey);
On 2014/08/26 15:42:40, flackr wrote:
> nit: move 1015 and 1017 to 1023 where used, similar to the blur method.

Done.

https://codereview.chromium.org/504793003/diff/120001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1090: gfx::Rect
initial_window_bound(50, 50, 100, 500);
On 2014/08/26 15:42:40, flackr wrote:
> nit: s/bound/bounds

Done.

[rsadam, 2014-08-26 15:46:26]

The CQ bit was checked by rsadam@chromium.org

[commit-bot: I haz the power, 2014-08-26 15:46:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsadam@chromium.org/504793003/140001

[commit-bot: I haz the power, 2014-08-26 22:34:17]

Committed patchset #8 (140001) as c88dbbed5e5dd82eaf64c04b8a1a5928d32841fa

[commit-bot: I haz the power, 2014-09-10 02:46:29]

Patchset 8 (id:??) landed as
https://crrev.com/00bff42f5738f985d04a888d64a64e802b65977d
Cr-Commit-Position: refs/heads/master@{#292004}

[chromium-reviews, 2014-09-10 05:10:48]

I'm not sure why I got this message - this patch landed a while ago, is
this a bug with CQ?

On Tue, Sep 9, 2014 at 10:46 PM, <commit-bot@chromium.org> wrote:

> Patchset 8 (id:??) landed as
> https://crrev.com/00bff42f5738f985d04a888d64a64e802b65977d
> Cr-Commit-Position: refs/heads/master@{#292004}
>
> https://codereview.chromium.org/504793003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.