Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165693002:
Fix tests in when MD is set to EXPERIMENTAL by default (Closed)
DescriptionFix tests in when MD is set to EXPERIMENTAL by default
Parameterize test fixtures in ash_unittests and unit_tests to run
with the three modes used in Ash material design (non-material,
material stable, and material experimental); see the runtime flag
--ash-md.
BUG=623428, 623426, 623423, 623422, 622914, 622912
Committed: https://crrev.com/70b502aef44c4f6f9db8d1abff3ff8d4cde9eab0
Cr-Commit-Position: refs/heads/master@{#406956}
Patch Set 1 #
Total comments: 21
Patch Set 2 : address comments #
Total comments: 16
Patch Set 3 : address comments #
Total comments: 1
Messages
Total messages: 33 (19 generated)
Description was changed from ========== fix tests BUG= ========== to ========== Fix tests in when MD is set to EXPERIMENTAL by default Parameterize test fixtures in ash_unittests and unit_tests to run with the three modes used in Ash material design (non-material, material stable, and material experimental); see the runtime flag --ash-md. BUG=623428, 623426, 623423, 623422, 622914, 622912 ==========
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
On 2016/07/19 15:40:54, yiyix wrote: > mailto:yiyix@chromium.org changed reviewers: > + mailto:tdanderson@chromium.org @tdanderson, Could you please take a look? Thank you very much
The CQ bit was checked by yiyix@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...
Nice! LGTM (with nits) https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/test/ash_md_test... File ash/test/ash_md_test_base.cc (right): https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/test/ash_md_test... ash/test/ash_md_test_base.cc:34: const int non_md_auto_hide_shelf_size = I think you should be able to: * Move line 34 to be after line 23, * Move line 39 to be after line 27, * Delete line 32, and * Delete line 37. If so, then omit the comments on lines 20 and 31. https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/wm/workspace/wor... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/wm/workspace/wor... ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved for auto hide shelf height in MD. are -> is https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/wm/workspace/wor... ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved for auto hide shelf height in MD. maybe reword to "for the auto-hidden shelf height" https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/wm/workspace_con... File ash/wm/workspace_controller_unittest.cc (right): https://chromiumcodereview.appspot.com/2165693002/diff/1/ash/wm/workspace_con... ash/wm/workspace_controller_unittest.cc:451: // height is 600. I would omit this comment entirely; the documentation you have given for GetMdAutoHiddenShelfHeightOffset() explains things. https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc (right): https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:194: // as Full screen, i.e., no light bar, windows starts from position 0; in nits: Full -> full "... no light bar and having an origin of (0,0). In non-MD, ..." https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:203: EXPECT_FALSE(frame_view->caption_button_container_->visible()); We probably want to perform this check in both non-MD and MD (both should be false), so move this to just after line 196. https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc (right): https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:130: // In MD, tab strip and tool bar are both hidden in immersive fullscreen mode; Suggested rewording of entire comment: "For MD, the browser's top chrome is completely hidden in immersive fullscreen mode." https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:137: EXPECT_TRUE(tabstrip->IsImmersiveStyle()); I'd move this to line 131 assuming it is true in both non-MD and MD. https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:145: // mode, the web connents should extend to the edge of screen. In non-MD, the connents->contents https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:147: if (ash::MaterialDesignController::IsShelfMaterial()) { nit: consider using a local variable const bool md_shelf = ...::IsShelfMaterial() to avoid the need to call this multiple times https://chromiumcodereview.appspot.com/2165693002/diff/1/chrome/browser/ui/vi... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:256: // ShouldHideTabIndicators() always return true. Omit this comment, and instead replace with expectations, e.g., for line 260: if (is shelf material) EXPECT_TRUE(ShouldHideTabIndicators()); else EXPECT_FALSE(ShouldHideTabIndicators()); Similarly for the other three locations.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2165693002/diff/1/ash/test/ash_md_test_base.cc File ash/test/ash_md_test_base.cc (right): https://codereview.chromium.org/2165693002/diff/1/ash/test/ash_md_test_base.c... ash/test/ash_md_test_base.cc:34: const int non_md_auto_hide_shelf_size = On 2016/07/19 21:10:23, tdanderson wrote: > I think you should be able to: > > * Move line 34 to be after line 23, > * Move line 39 to be after line 27, > * Delete line 32, and > * Delete line 37. > > If so, then omit the comments on lines 20 and 31. It works. We need to reset material_design_state less often this way. https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved for auto hide shelf height in MD. On 2016/07/19 21:10:23, tdanderson wrote: > maybe reword to "for the auto-hidden shelf height" Done. https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved for auto hide shelf height in MD. On 2016/07/19 21:10:23, tdanderson wrote: > are -> is Done. https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace_controller... File ash/wm/workspace_controller_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace_controller... ash/wm/workspace_controller_unittest.cc:451: // height is 600. On 2016/07/19 21:10:23, tdanderson wrote: > I would omit this comment entirely; the documentation you have given for > GetMdAutoHiddenShelfHeightOffset() explains things. Done. https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc (right): https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:194: // as Full screen, i.e., no light bar, windows starts from position 0; in On 2016/07/19 21:10:23, tdanderson wrote: > nits: > > Full -> full > > "... no light bar and having an origin of (0,0). In non-MD, ..." (0,0) is more descriptive than 0. Thanks https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:203: EXPECT_FALSE(frame_view->caption_button_container_->visible()); On 2016/07/19 21:10:23, tdanderson wrote: > We probably want to perform this check in both non-MD and MD (both should be > false), so move this to just after line 196. The caption_button_container visibility is updated in caption_button_container_->SetVisible(!UseImmersiveLightbarHeaderStyle()); then UseImmersiveLightbarHeaderStyle uses browser_view()->IsTabStripVisible() which always return false because we removed the tab strip for immersive mode. So the visibility of caption_button_container is incorrect, this will be corrected after we fully removing boolean for tabstrip. However, since ShouldPaint() returns false, so nothing is painted to the screen. https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:130: // In MD, tab strip and tool bar are both hidden in immersive fullscreen mode; On 2016/07/19 21:10:24, tdanderson wrote: > Suggested rewording of entire comment: "For MD, the browser's top chrome is > completely hidden in immersive fullscreen mode." Done. https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:137: EXPECT_TRUE(tabstrip->IsImmersiveStyle()); On 2016/07/19 21:10:24, tdanderson wrote: > I'd move this to line 131 assuming it is true in both non-MD and MD. Done. https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:145: // mode, the web connents should extend to the edge of screen. In non-MD, the On 2016/07/19 21:10:24, tdanderson wrote: > connents->contents I was careless. Thanks. https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:147: if (ash::MaterialDesignController::IsShelfMaterial()) { On 2016/07/19 21:10:23, tdanderson wrote: > nit: consider using a local variable const bool md_shelf = > ...::IsShelfMaterial() to avoid the need to call this multiple times Done.
On 2016/07/20 06:03:08, yiyix wrote: > https://codereview.chromium.org/2165693002/diff/1/ash/test/ash_md_test_base.cc > File ash/test/ash_md_test_base.cc (right): > > https://codereview.chromium.org/2165693002/diff/1/ash/test/ash_md_test_base.c... > ash/test/ash_md_test_base.cc:34: const int non_md_auto_hide_shelf_size = > On 2016/07/19 21:10:23, tdanderson wrote: > > I think you should be able to: > > > > * Move line 34 to be after line 23, > > * Move line 39 to be after line 27, > > * Delete line 32, and > > * Delete line 37. > > > > If so, then omit the comments on lines 20 and 31. > > It works. We need to reset material_design_state less often this way. > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... > File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... > ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved for > auto hide shelf height in MD. > On 2016/07/19 21:10:23, tdanderson wrote: > > maybe reword to "for the auto-hidden shelf height" > > Done. > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... > ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved for > auto hide shelf height in MD. > On 2016/07/19 21:10:23, tdanderson wrote: > > are -> is > > Done. > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace_controller... > File ash/wm/workspace_controller_unittest.cc (right): > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace_controller... > ash/wm/workspace_controller_unittest.cc:451: // height is 600. > On 2016/07/19 21:10:23, tdanderson wrote: > > I would omit this comment entirely; the documentation you have given for > > GetMdAutoHiddenShelfHeightOffset() explains things. > > Done. > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > File > chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc > (right): > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:194: > // as Full screen, i.e., no light bar, windows starts from position 0; in > On 2016/07/19 21:10:23, tdanderson wrote: > > nits: > > > > Full -> full > > > > "... no light bar and having an origin of (0,0). In non-MD, ..." > > (0,0) is more descriptive than 0. Thanks > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:203: > EXPECT_FALSE(frame_view->caption_button_container_->visible()); > On 2016/07/19 21:10:23, tdanderson wrote: > > We probably want to perform this check in both non-MD and MD (both should be > > false), so move this to just after line 196. > > The caption_button_container visibility is updated in > caption_button_container_->SetVisible(!UseImmersiveLightbarHeaderStyle()); then > UseImmersiveLightbarHeaderStyle uses browser_view()->IsTabStripVisible() which > always return false because we removed the tab strip for immersive mode. So the > visibility of caption_button_container is incorrect, this will be corrected > after we fully removing boolean for tabstrip. > > However, since ShouldPaint() returns false, so nothing is painted to the screen. > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc > (right): > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:130: // > In MD, tab strip and tool bar are both hidden in immersive fullscreen mode; > On 2016/07/19 21:10:24, tdanderson wrote: > > Suggested rewording of entire comment: "For MD, the browser's top chrome is > > completely hidden in immersive fullscreen mode." > > Done. > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:137: > EXPECT_TRUE(tabstrip->IsImmersiveStyle()); > On 2016/07/19 21:10:24, tdanderson wrote: > > I'd move this to line 131 assuming it is true in both non-MD and MD. > > Done. > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:145: // > mode, the web connents should extend to the edge of screen. In non-MD, the > On 2016/07/19 21:10:24, tdanderson wrote: > > connents->contents > > I was careless. Thanks. > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:147: if > (ash::MaterialDesignController::IsShelfMaterial()) { > On 2016/07/19 21:10:23, tdanderson wrote: > > nit: consider using a local variable const bool md_shelf = > > ...::IsShelfMaterial() to avoid the need to call this multiple times > > Done. @tdanderson, Could you take another look?
On 2016/07/20 17:53:33, yiyix wrote: > On 2016/07/20 06:03:08, yiyix wrote: > > https://codereview.chromium.org/2165693002/diff/1/ash/test/ash_md_test_base.cc > > File ash/test/ash_md_test_base.cc (right): > > > > > https://codereview.chromium.org/2165693002/diff/1/ash/test/ash_md_test_base.c... > > ash/test/ash_md_test_base.cc:34: const int non_md_auto_hide_shelf_size = > > On 2016/07/19 21:10:23, tdanderson wrote: > > > I think you should be able to: > > > > > > * Move line 34 to be after line 23, > > > * Move line 39 to be after line 27, > > > * Delete line 32, and > > > * Delete line 37. > > > > > > If so, then omit the comments on lines 20 and 31. > > > > It works. We need to reset material_design_state less often this way. > > > > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... > > File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): > > > > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... > > ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved > for > > auto hide shelf height in MD. > > On 2016/07/19 21:10:23, tdanderson wrote: > > > maybe reword to "for the auto-hidden shelf height" > > > > Done. > > > > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace/workspace_... > > ash/wm/workspace/workspace_window_resizer_unittest.cc:1034: // are reserved > for > > auto hide shelf height in MD. > > On 2016/07/19 21:10:23, tdanderson wrote: > > > are -> is > > > > Done. > > > > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace_controller... > > File ash/wm/workspace_controller_unittest.cc (right): > > > > > https://codereview.chromium.org/2165693002/diff/1/ash/wm/workspace_controller... > > ash/wm/workspace_controller_unittest.cc:451: // height is 600. > > On 2016/07/19 21:10:23, tdanderson wrote: > > > I would omit this comment entirely; the documentation you have given for > > > GetMdAutoHiddenShelfHeightOffset() explains things. > > > > Done. > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > File > > chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc > > (right): > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > > chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:194: > > // as Full screen, i.e., no light bar, windows starts from position 0; in > > On 2016/07/19 21:10:23, tdanderson wrote: > > > nits: > > > > > > Full -> full > > > > > > "... no light bar and having an origin of (0,0). In non-MD, ..." > > > > (0,0) is more descriptive than 0. Thanks > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > > chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:203: > > EXPECT_FALSE(frame_view->caption_button_container_->visible()); > > On 2016/07/19 21:10:23, tdanderson wrote: > > > We probably want to perform this check in both non-MD and MD (both should be > > > false), so move this to just after line 196. > > > > The caption_button_container visibility is updated in > > caption_button_container_->SetVisible(!UseImmersiveLightbarHeaderStyle()); > then > > UseImmersiveLightbarHeaderStyle uses browser_view()->IsTabStripVisible() > which > > always return false because we removed the tab strip for immersive mode. So > the > > visibility of caption_button_container is incorrect, this will be corrected > > after we fully removing boolean for tabstrip. > > > > However, since ShouldPaint() returns false, so nothing is painted to the > screen. > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:130: > // > > In MD, tab strip and tool bar are both hidden in immersive fullscreen mode; > > On 2016/07/19 21:10:24, tdanderson wrote: > > > Suggested rewording of entire comment: "For MD, the browser's top chrome is > > > completely hidden in immersive fullscreen mode." > > > > Done. > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:137: > > EXPECT_TRUE(tabstrip->IsImmersiveStyle()); > > On 2016/07/19 21:10:24, tdanderson wrote: > > > I'd move this to line 131 assuming it is true in both non-MD and MD. > > > > Done. > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:145: > // > > mode, the web connents should extend to the edge of screen. In non-MD, the > > On 2016/07/19 21:10:24, tdanderson wrote: > > > connents->contents > > > > I was careless. Thanks. > > > > > https://codereview.chromium.org/2165693002/diff/1/chrome/browser/ui/views/fra... > > chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:147: > if > > (ash::MaterialDesignController::IsShelfMaterial()) { > > On 2016/07/19 21:10:23, tdanderson wrote: > > > nit: consider using a local variable const bool md_shelf = > > > ...::IsShelfMaterial() to avoid the need to call this multiple times > > > > Done. > > @tdanderson, Could you take another look? still lgtm
yiyix@chromium.org changed reviewers: + jamescook@chromium.org, msw@chromium.org
@jamescook, Could you review changes in ash/ ? @msw, could you review changes in chrome/ ? Thank you very much
The CQ bit was checked by yiyix@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...
ash/* LGTM with an optional idea https://codereview.chromium.org/2165693002/diff/20001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer_unittest.cc:1041: 600 - 160 - 112 - auto_hidden_shelf_height - 7), optional: It's not your fault, but this is really ugly. Could the magic number "600 - 160 - 112" be pulled into a constant? It looks like it's screen height minus window height minus... something. If you can think of a good name for it, it would shorten these lines and might be more reasonable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:250: // TODO(yiyix): Update caption_button_container_'s visibility when Chrome OS q: Could you add MD-specific behavior now and avoid the TODO? https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:250: // TODO(yiyix): Update caption_button_container_'s visibility when Chrome OS nit: |caption_button_container_|'s https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc (right): https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:197: if (ash::MaterialDesignController::IsShelfMaterial()) { Should this check caption_button_container_ visibility in MD? https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:134: if (is_using_material_design) { Should this check tabstrip->IsImmersiveStyle in MD? https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:144: // In MD, since tab strip and tool bar are both hidden in immersive fullscreen nit: 'the tab strip' https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:169: if (ash::MaterialDesignController::IsShelfMaterial()) { nit: use |is_using_material_design| here too. (or just inline ash::MaterialDesignController::IsShelfMaterial() everywhere and remove |is_using_material_design|) https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:255: // Note that tab indicators hints are removed from MD, so function nit: "indicators are", "so ShouldHideTabIndicators()", "returns true."
@jamescook, I tried to improve the readability of that test case, could you take another look? @msw, could you review the code change in chrome/ ? I made some comments regarding your comments too. Thank you very much. https://codereview.chromium.org/2165693002/diff/20001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/20001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer_unittest.cc:1041: 600 - 160 - 112 - auto_hidden_shelf_height - 7), On 2016/07/20 19:21:52, James Cook wrote: > optional: It's not your fault, but this is really ugly. Could the magic number > "600 - 160 - 112" be pulled into a constant? It looks like it's screen height > minus window height minus... something. If you can think of a good name for it, > it would shorten these lines and might be more reasonable. I tried to rename "600 - 160 - 112" and "800 - 320 - 96". https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:250: // TODO(yiyix): Update caption_button_container_'s visibility when Chrome OS On 2016/07/20 20:06:50, msw wrote: > nit: |caption_button_container_|'s Done. https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:250: // TODO(yiyix): Update caption_button_container_'s visibility when Chrome OS On 2016/07/20 20:06:50, msw wrote: > q: Could you add MD-specific behavior now and avoid the TODO? As we have discussed offline, there is no behavior to be added because caption_button_container_ does not paint to screen in immersive full screen mode. https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc (right): https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:197: if (ash::MaterialDesignController::IsShelfMaterial()) { On 2016/07/20 20:06:50, msw wrote: > Should this check caption_button_container_ visibility in MD? since ShouldPaint() returns false, caption_button_container_ will not be painted to screen. Due to the other TODO in browser_non_client_frame_view_ash.cc, the visibility is not updated correctly for the moment. https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:134: if (is_using_material_design) { On 2016/07/20 20:06:51, msw wrote: > Should this check tabstrip->IsImmersiveStyle in MD? As we stopped showing tab indicator hints in MD in immersive full screen mode, the tab strip will never be using immersive style. The function IsImmersiveStyle will be deprecated as we turn on the MD by default. So I choose to ignore this check in this case. Moreover, when tabstrip->visible() returns false, tabstrip will not be shown on screen in any forms (i.e., top chrome form or immersive style form). It is important to check the style when the tabstrip is revealed in immersive mode, which is covered in this test case. https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:144: // In MD, since tab strip and tool bar are both hidden in immersive fullscreen On 2016/07/20 20:06:51, msw wrote: > nit: 'the tab strip' Done. https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:169: if (ash::MaterialDesignController::IsShelfMaterial()) { On 2016/07/20 20:06:51, msw wrote: > nit: use |is_using_material_design| here too. (or just inline > ash::MaterialDesignController::IsShelfMaterial() everywhere and remove > |is_using_material_design|) Done. https://codereview.chromium.org/2165693002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:255: // Note that tab indicators hints are removed from MD, so function On 2016/07/20 20:06:50, msw wrote: > nit: "indicators are", "so ShouldHideTabIndicators()", "returns true." Done. Thank you for carefully reading my comments.
The CQ bit was checked by yiyix@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...
still lgtm https://codereview.chromium.org/2165693002/diff/40001/ash/wm/workspace/worksp... File ash/wm/workspace/workspace_window_resizer_unittest.cc (right): https://codereview.chromium.org/2165693002/diff/40001/ash/wm/workspace/worksp... ash/wm/workspace/workspace_window_resizer_unittest.cc:1008: int distance_to_right = Oh, this is much better. Thanks for doing it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yiyix@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/2165693002/#ps40001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix tests in when MD is set to EXPERIMENTAL by default Parameterize test fixtures in ash_unittests and unit_tests to run with the three modes used in Ash material design (non-material, material stable, and material experimental); see the runtime flag --ash-md. BUG=623428, 623426, 623423, 623422, 622914, 622912 ========== to ========== Fix tests in when MD is set to EXPERIMENTAL by default Parameterize test fixtures in ash_unittests and unit_tests to run with the three modes used in Ash material design (non-material, material stable, and material experimental); see the runtime flag --ash-md. BUG=623428, 623426, 623423, 623422, 622914, 622912 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix tests in when MD is set to EXPERIMENTAL by default Parameterize test fixtures in ash_unittests and unit_tests to run with the three modes used in Ash material design (non-material, material stable, and material experimental); see the runtime flag --ash-md. BUG=623428, 623426, 623423, 623422, 622914, 622912 ========== to ========== Fix tests in when MD is set to EXPERIMENTAL by default Parameterize test fixtures in ash_unittests and unit_tests to run with the three modes used in Ash material design (non-material, material stable, and material experimental); see the runtime flag --ash-md. BUG=623428, 623426, 623423, 623422, 622914, 622912 Committed: https://crrev.com/70b502aef44c4f6f9db8d1abff3ff8d4cde9eab0 Cr-Commit-Position: refs/heads/master@{#406956} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/70b502aef44c4f6f9db8d1abff3ff8d4cde9eab0 Cr-Commit-Position: refs/heads/master@{#406956} |
