Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517833003:
Allow to create ARC++ windows minimized on startup. (Closed)
DescriptionAllow to create ARC++ windows minimized on startup.
TEST=Tested with OneNote badge.
BUG=667608
Committed: https://crrev.com/43961ed73ccb8a76e6621fcdbe0a930aa88a42b3
Cr-Commit-Position: refs/heads/master@{#433824}
Patch Set 1 #Patch Set 2 : Added a test. #
Total comments: 4
Patch Set 3 : Cleaned up the test. #
Total comments: 4
Messages
Total messages: 33 (20 generated)
The CQ bit was checked by mtomasz@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...
ARC side here: ag/1644756
mtomasz@chromium.org changed reviewers: + reveman@chromium.org, skuhne@chromium.org
@reveman, @skuhne: PTAL. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think this should be fine - but I leave the final lgtm to reveman here
Description was changed from ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=b/31012717 ========== to ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=b/31012717 ==========
mtomasz@chromium.org changed reviewers: + oshima@chromium.org
@oshima: PTAL.
Description was changed from ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=b/31012717 ========== to ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=667608 ==========
The CQ bit was checked by mtomasz@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm after minor improvements to the test https://codereview.chromium.org/2517833003/diff/20001/components/exo/shell_su... File components/exo/shell_surface_unittest.cc (right): https://codereview.chromium.org/2517833003/diff/20001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:135: TEST_F(ShellSurfaceTest, Minimize_BeforeSurfaceCommit) { nit: s/Minimize_BeforeSurfaceCommit/InitiallyMinimized/ or alternatively fold it into the "Minimize" test above https://codereview.chromium.org/2517833003/diff/20001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:137: std::unique_ptr<Buffer> buffer( Can you attach this buffer and commit after calling minimize to ensure that this doesn't reset the state?
https://codereview.chromium.org/2517833003/diff/20001/components/exo/shell_su... File components/exo/shell_surface_unittest.cc (right): https://codereview.chromium.org/2517833003/diff/20001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:135: TEST_F(ShellSurfaceTest, Minimize_BeforeSurfaceCommit) { On 2016/11/22 08:26:49, reveman wrote: > nit: s/Minimize_BeforeSurfaceCommit/InitiallyMinimized/ or alternatively fold it > into the "Minimize" test above Done. https://codereview.chromium.org/2517833003/diff/20001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:137: std::unique_ptr<Buffer> buffer( On 2016/11/22 08:26:48, reveman wrote: > Can you attach this buffer and commit after calling minimize to ensure that this > doesn't reset the state? Done.
The CQ bit was checked by mtomasz@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...
lgtm
The CQ bit was unchecked by mtomasz@chromium.org
The CQ bit was checked by mtomasz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2517833003/#ps40001 (title: "Cleaned up the test.")
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": 1479806569227470,
"parent_rev": "77bd101cd233af2a17c1a5e94c0d8dbfd276c2ee", "commit_rev":
"d4f3ac0e8fea75aa2f1e0514c8c3f5fbcb27b721"}
Message was sent while issue was closed.
Description was changed from ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=667608 ========== to ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=667608 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=667608 ========== to ========== Allow to create ARC++ windows minimized on startup. TEST=Tested with OneNote badge. BUG=667608 Committed: https://crrev.com/43961ed73ccb8a76e6621fcdbe0a930aa88a42b3 Cr-Commit-Position: refs/heads/master@{#433824} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/43961ed73ccb8a76e6621fcdbe0a930aa88a42b3 Cr-Commit-Position: refs/heads/master@{#433824}
Message was sent while issue was closed.
Some unit test improvements that would be nice to fix in a follow up. https://codereview.chromium.org/2517833003/diff/40001/components/exo/shell_su... File components/exo/shell_surface_unittest.cc (right): https://codereview.chromium.org/2517833003/diff/40001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:135: surface->Commit(); Would be nice to check that shell surface is still minimized after this Commit() call but before the next Minimize call. https://codereview.chromium.org/2517833003/diff/40001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:136: shell_surface->Minimize(); Not sure it makes sense to test calling this now that we expect the shell surface to already be minimized. Can you instead restore it, verify that it restored correctly, then minimize it again and verify that it worked?
Message was sent while issue was closed.
https://codereview.chromium.org/2517833003/diff/40001/components/exo/shell_su... File components/exo/shell_surface_unittest.cc (right): https://codereview.chromium.org/2517833003/diff/40001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:135: surface->Commit(); On 2016/11/22 09:29:11, reveman wrote: > Would be nice to check that shell surface is still minimized after this Commit() > call but before the next Minimize call. Acknowledged. https://codereview.chromium.org/2517833003/diff/40001/components/exo/shell_su... components/exo/shell_surface_unittest.cc:136: shell_surface->Minimize(); On 2016/11/22 09:29:11, reveman wrote: > Not sure it makes sense to test calling this now that we expect the shell > surface to already be minimized. Can you instead restore it, verify that it > restored correctly, then minimize it again and verify that it worked? Oops, minimizing here completely doesn't make sense. That's what happens when in rush... My intention was to confirm that after committing it would still be minimized, so just removing this line (without restoring). Is it OK? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
