Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2589793002:
ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated (Closed)
|
Created:
4 years ago by Qiang(Joe) Xu Modified:
4 years ago Reviewers:
James Cook CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated
Changes:
Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated, otherwise it might miss the required bounds update in the case of 673803.
BUG=673803
TEST=emulator test; also add test coverage.
Committed: https://crrev.com/962f4ece9b5b3efa7e2cc3f0dde56cd08d9ad176
Cr-Commit-Position: refs/heads/master@{#439992}
Patch Set 1 #Patch Set 2 : add test coverage #
Total comments: 8
Patch Set 3 : nits #
Messages
Total messages: 25 (16 generated)
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated BUG=673803 TEST=emulator test and automated tests. ========== to ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated. BUG=673803 TEST=emulator test and automated tests. ==========
warx@chromium.org changed reviewers: + jamescook@chromium.org
Hi James, ptal, thanks!
On 2016/12/19 13:38:29, Qiang(Joe) Xu wrote: > Hi James, ptal, thanks! btw, the changes in case WM_EVENT_WORKAREA_BOUNDS_CHANGED fix the reporter's bug. But I feel it also needs to be changed in case WM_EVENT_DISPLAY_BOUNDS_CHANGED.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code looks good, but can you add test coverage? Note: Due to the mustash WmWindow refactoring tests in //ash/common/wm have to be written in terms of the WmWindow / WindowOwner and there aren't too many examples. If it's hard to add coverage there feel free to put it in //ash/wm (the classic ash location), either in its own file or as part of window_manager_unittest.cc
The CQ bit was checked by warx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi James, test coverage is added in ash/common/wm/workspace/workspace_layout_manager_unittest.cc, I see currently most of the layout tests are put there. PTAL, thanks!
Description was changed from ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated. BUG=673803 TEST=emulator test and automated tests. ========== to ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated, otherwise it might miss the required bounds update in the case of 673803. BUG=673803 TEST=emulator test; also add test coverage. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. Nice fix. https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... File ash/common/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:543: TEST_F(WorkspaceLayoutManagerTest, SnappedWindowMayNotUpdateOnWorkAreaChanged) { nit: Maybe call this test what you expect the good result to be? Or add a comment about what it's testing? It's not obvious what "MayNot" means -- either you're testing that it should *change* bounds or *not change* bounds. https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:546: CreateTestWindow(gfx::Rect(40, 40, 40, 40))); tip: It doesn't matter here, but in tests, vary your data. For example, 10, 20, 300, 400 would be better, as it may change issues like width / height being reversed, and failures are more obvious. https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:556: gfx::Rect expected = How about |expected_bounds| or |expected_window_bounds| ? https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:564: // crbug.com/673803 that work area first becomes fullscreen and then return to nit: return -> returns
https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... File ash/common/wm/workspace/workspace_layout_manager_unittest.cc (right): https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:543: TEST_F(WorkspaceLayoutManagerTest, SnappedWindowMayNotUpdateOnWorkAreaChanged) { On 2016/12/20 17:50:31, James Cook (OOO until Jan 4) wrote: > nit: Maybe call this test what you expect the good result to be? Or add a > comment about what it's testing? It's not obvious what "MayNot" means -- either > you're testing that it should *change* bounds or *not change* bounds. done by adding a comment and adjust the title a little bit https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:546: CreateTestWindow(gfx::Rect(40, 40, 40, 40))); On 2016/12/20 17:50:31, James Cook (OOO until Jan 4) wrote: > tip: It doesn't matter here, but in tests, vary your data. For example, 10, 20, > 300, 400 would be better, as it may change issues like width / height being > reversed, and failures are more obvious. Done. https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:556: gfx::Rect expected = On 2016/12/20 17:50:31, James Cook (OOO until Jan 4) wrote: > How about |expected_bounds| or |expected_window_bounds| ? Done. https://codereview.chromium.org/2589793002/diff/20001/ash/common/wm/workspace... ash/common/wm/workspace/workspace_layout_manager_unittest.cc:564: // crbug.com/673803 that work area first becomes fullscreen and then return to On 2016/12/20 17:50:31, James Cook (OOO until Jan 4) wrote: > nit: return -> returns Done.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2589793002/#ps40001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482287880492060,
"parent_rev": "ed13647d055ffe4256bafdede48e99d31b9ebefb", "commit_rev":
"3adf6e528e1da081ffa15703a257679042648576"}
Message was sent while issue was closed.
Description was changed from ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated, otherwise it might miss the required bounds update in the case of 673803. BUG=673803 TEST=emulator test; also add test coverage. ========== to ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated, otherwise it might miss the required bounds update in the case of 673803. BUG=673803 TEST=emulator test; also add test coverage. Review-Url: https://codereview.chromium.org/2589793002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated, otherwise it might miss the required bounds update in the case of 673803. BUG=673803 TEST=emulator test; also add test coverage. Review-Url: https://codereview.chromium.org/2589793002 ========== to ========== ash: Should use GetTargetBounds to check call of SetBoundsDirectAnimated Changes: Because window's layer might be doing animation, in default_state.cc we should use GetTargetBounds to check call of SetBoundsDirectAnimated, otherwise it might miss the required bounds update in the case of 673803. BUG=673803 TEST=emulator test; also add test coverage. Committed: https://crrev.com/962f4ece9b5b3efa7e2cc3f0dde56cd08d9ad176 Cr-Commit-Position: refs/heads/master@{#439992} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/962f4ece9b5b3efa7e2cc3f0dde56cd08d9ad176 Cr-Commit-Position: refs/heads/master@{#439992} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
