Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683033002:
Remove non-MD test coverage from ShelfLayoutManagerTest (Closed)
DescriptionRemove non-MD test coverage from ShelfLayoutManagerTest
Removed checks that verify some non-MD immersive mode layout.
BUG=685837
TEST=ShelfLayoutManagerTest.* in ash_unittests
Review-Url: https://codereview.chromium.org/2683033002
Cr-Commit-Position: refs/heads/master@{#449520}
Committed: https://chromium.googlesource.com/chromium/src/+/84069f265a506df8b66ab1109aa0a576f086b971
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added back and updated wrongly removed check #Patch Set 3 : Removed unnecessary check #Messages
Total messages: 26 (18 generated)
The CQ bit was checked by mohsen@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...
mohsen@chromium.org changed reviewers: + tdanderson@chromium.org
Please take a look...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tdanderson@chromium.org changed reviewers: + jamescook@chromium.org
One comment below, otherwise LGTM. Though I'm not an owner so I'm adding jamescook@. https://codereview.chromium.org/2683033002/diff/1/ash/shelf/shelf_layout_mana... File ash/shelf/shelf_layout_manager_unittest.cc (left): https://codereview.chromium.org/2683033002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager_unittest.cc:179: auto_hidden_shelf_widget_bounds_.height()); Instead of removing this altogether can you replace it with an EXPECT_EQ(height, 0) as per the comment on line 173?
LGTM if tdanderson's comment addressed
The CQ bit was checked by mohsen@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...
https://codereview.chromium.org/2683033002/diff/1/ash/shelf/shelf_layout_mana... File ash/shelf/shelf_layout_manager_unittest.cc (left): https://codereview.chromium.org/2683033002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager_unittest.cc:179: auto_hidden_shelf_widget_bounds_.height()); On 2017/02/08 at 22:36:53, tdanderson wrote: > Instead of removing this altogether can you replace it with an EXPECT_EQ(height, 0) as per the comment on line 173? I think I mistakenly removed this check entirely. I should have only removed the material mode check. Fixed. I also check width in case of vertical shelf. I don't think a check similar to what you suggested would work however. Per my tests, shelf height/width is not always zero in for an auto hidden shelf while a drag gesture is in progress. Also, |auto_hidden_shelf_widget_bounds_.height()| is 3 even in MD mode. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mohsen@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...
https://codereview.chromium.org/2683033002/diff/1/ash/shelf/shelf_layout_mana... File ash/shelf/shelf_layout_manager_unittest.cc (left): https://codereview.chromium.org/2683033002/diff/1/ash/shelf/shelf_layout_mana... ash/shelf/shelf_layout_manager_unittest.cc:179: auto_hidden_shelf_widget_bounds_.height()); On 2017/02/09 at 06:57:02, mohsen wrote: > On 2017/02/08 at 22:36:53, tdanderson wrote: > > Instead of removing this altogether can you replace it with an EXPECT_EQ(height, 0) as per the comment on line 173? > > I think I mistakenly removed this check entirely. I should have only removed the material mode check. Fixed. I also check width in case of vertical shelf. I don't think a check similar to what you suggested would work however. Per my tests, shelf height/width is not always zero in for an auto hidden shelf while a drag gesture is in progress. Also, |auto_hidden_shelf_widget_bounds_.height()| is 3 even in MD mode. WDYT? Per offline discussions with tdanderson@, this check is not useful on MD. |shelf_bounds.height()| can have any non-negative value. So, removed!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm
The CQ bit was checked by mohsen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2683033002/#ps40001 (title: "Removed unnecessary check")
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": 1486692920040280,
"parent_rev": "e8f8ea9d2076848d998001dea1e7b90d7beb83a8", "commit_rev":
"84069f265a506df8b66ab1109aa0a576f086b971"}
Message was sent while issue was closed.
Description was changed from ========== Remove non-MD test coverage from ShelfLayoutManagerTest Removed checks that verify some non-MD immersive mode layout. BUG=685837 TEST=ShelfLayoutManagerTest.* in ash_unittests ========== to ========== Remove non-MD test coverage from ShelfLayoutManagerTest Removed checks that verify some non-MD immersive mode layout. BUG=685837 TEST=ShelfLayoutManagerTest.* in ash_unittests Review-Url: https://codereview.chromium.org/2683033002 Cr-Commit-Position: refs/heads/master@{#449520} Committed: https://chromium.googlesource.com/chromium/src/+/84069f265a506df8b66ab1109aa0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/84069f265a506df8b66ab1109aa0... |
