Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516803002:
[Mac] Tests for FullscreenToolbarController (Closed)
Description[Mac] Tests for FullscreenToolbarController
Also fixed and reenabled the fullscreen test in BrowserWindowControllerBrowserTest
BUG=599119, 643418
Committed: https://crrev.com/e852d5242adc506bbe9d9dfeb714681e32ae42ef
Cr-Commit-Position: refs/heads/master@{#434709}
Patch Set 1 #Patch Set 2 : Reenabled a test for BrowserWindowController #Patch Set 3 : Fixed the flake #
Total comments: 7
Patch Set 4 : Fix for erikchen #
Total comments: 9
Patch Set 5 : Fix for rsesek #
Total comments: 2
Patch Set 6 : Fix for rsesek #
Messages
Total messages: 48 (32 generated)
The CQ bit was checked by spqchan@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 ========== [Mac] Test for FullscreenToolbarController BUG= ========== to ========== [Mac] Tests for FullscreenToolbarController Also fixed the fullscreen test in BrowserWindowControllerBrowserTest BUG=643418 ==========
Description was changed from ========== [Mac] Tests for FullscreenToolbarController Also fixed the fullscreen test in BrowserWindowControllerBrowserTest BUG=643418 ========== to ========== [Mac] Tests for FullscreenToolbarController Also fixed the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by spqchan@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.
Description was changed from ========== [Mac] Tests for FullscreenToolbarController Also fixed the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 ========== to ========== [Mac] Tests for FullscreenToolbarController Also fixed and reenabled the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 ==========
spqchan@chromium.org changed reviewers: + erikchen@chromium.org
PTAL
this is awesome. thanks. lgtm with nits. https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:92: const CGFloat kToolbarVerticalOffset = -22; maybe we should expose this as well to avoid duplication of random constants? https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:101: id bwc = [OCMockObject mockForClass:[BrowserWindowController class]]; should we retain bwc for the duration of this test? I think the current code is assuming that the autoreleasepool doesn't get flushed. https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:112: menubar_tracker_ = [[MockFullscreenMenubarTracker alloc] should we be using scoped_nsobject here?
The CQ bit was checked by spqchan@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.
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
thanks! +rsesek for ownership https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:92: const CGFloat kToolbarVerticalOffset = -22; On 2016/11/21 18:45:04, erikchen wrote: > maybe we should expose this as well to avoid duplication of random constants? Done. https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:101: id bwc = [OCMockObject mockForClass:[BrowserWindowController class]]; On 2016/11/21 18:45:05, erikchen wrote: > should we retain bwc for the duration of this test? I think the current code is > assuming that the autoreleasepool doesn't get flushed. Done. https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:112: menubar_tracker_ = [[MockFullscreenMenubarTracker alloc] On 2016/11/21 18:45:04, erikchen wrote: > should we be using scoped_nsobject here? No, FullscreenToolbarController will be holding it in a scoped_nsobject
Awesome to see tests for this code!! https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2516803002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:112: menubar_tracker_ = [[MockFullscreenMenubarTracker alloc] On 2016/11/22 20:18:42, spqchan wrote: > On 2016/11/21 18:45:04, erikchen wrote: > > should we be using scoped_nsobject here? > > No, FullscreenToolbarController will be holding it in a scoped_nsobject I think it's a little atypical for a setter to transfer ownership. I'd expect the setter to retain the argument before putting it in scoped_nsobject, and then this reference would be owned locally. https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:732: [[controller() fullscreenToolbarController] computeLayout].toolbarStyle, nit: gtest writes error messages expecting EXPECT_EQ(expected, actual) https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:92: FullscreenToolbarControllerTest() {} You can omit this. https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:129: DCHECK_EQ(layout.toolbarFraction, toolbarFraction); nit: EXPECT_ ordering https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:192: DCHECK([locks isToolbarVisibilityLocked]); No DCHECKs in test. Use EXPECT_TRUE() instead.
The CQ bit was checked by spqchan@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.
PTAL https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:732: [[controller() fullscreenToolbarController] computeLayout].toolbarStyle, On 2016/11/22 21:02:44, Robert Sesek wrote: > nit: gtest writes error messages expecting EXPECT_EQ(expected, actual) Oh snap, I wish I knew this earlier. Thanks for the heads up! https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:92: FullscreenToolbarControllerTest() {} On 2016/11/22 21:02:44, Robert Sesek wrote: > You can omit this. I removed this line and got an compilation error. It says that the test must explicitly initialize the base class since it does not have a default constructor. https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:129: DCHECK_EQ(layout.toolbarFraction, toolbarFraction); On 2016/11/22 21:02:44, Robert Sesek wrote: > nit: EXPECT_ ordering Done. https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:192: DCHECK([locks isToolbarVisibilityLocked]); On 2016/11/22 21:02:44, Robert Sesek wrote: > No DCHECKs in test. Use EXPECT_TRUE() instead. Done.
LGTM w/ nit https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm (right): https://codereview.chromium.org/2516803002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller_unittest.mm:92: FullscreenToolbarControllerTest() {} On 2016/11/22 23:04:56, spqchan wrote: > On 2016/11/22 21:02:44, Robert Sesek wrote: > > You can omit this. > > I removed this line and got an compilation error. It says that the test must > explicitly initialize the base class since it does not have a default > constructor. Huh, that's new... https://codereview.chromium.org/2516803002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/2516803002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:740: [[controller() fullscreenToolbarController] computeLayout].toolbarStyle, Flip these EXPECT_ too.
thanks! https://codereview.chromium.org/2516803002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/2516803002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:740: [[controller() fullscreenToolbarController] computeLayout].toolbarStyle, On 2016/11/23 18:11:36, Robert Sesek wrote: > Flip these EXPECT_ too. Done.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2516803002/#ps100001 (title: "Fix for rsesek")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by spqchan@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by spqchan@chromium.org
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": 100001, "attempt_start_ts": 1480354166502760,
"parent_rev": "f06cc8b8319c3858b03fb8b068fc5172f27b3bae", "commit_rev":
"c8351feca9eb48134db39e10137d313b140d975e"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Tests for FullscreenToolbarController Also fixed and reenabled the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 ========== to ========== [Mac] Tests for FullscreenToolbarController Also fixed and reenabled the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Mac] Tests for FullscreenToolbarController Also fixed and reenabled the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 ========== to ========== [Mac] Tests for FullscreenToolbarController Also fixed and reenabled the fullscreen test in BrowserWindowControllerBrowserTest BUG=599119, 643418 Committed: https://crrev.com/e852d5242adc506bbe9d9dfeb714681e32ae42ef Cr-Commit-Position: refs/heads/master@{#434709} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e852d5242adc506bbe9d9dfeb714681e32ae42ef Cr-Commit-Position: refs/heads/master@{#434709} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
