Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1809933003:
Port BubbleDelegate tests to BubbleDialogDelegate (Closed)
|
Created:
4 years, 9 months ago by Evan Stade Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPort BubbleDelegate tests to BubbleDialogDelegate
BUG=588877
TBR=sky@chromium.org
Committed: https://crrev.com/1b8cd416b2eb6eadc565b9b3a3be13de8a424c87
Cr-Commit-Position: refs/heads/master@{#382045}
Patch Set 1 #
Total comments: 16
Patch Set 2 : msw review #
Messages
Total messages: 35 (16 generated)
estade@chromium.org changed reviewers: + msw@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809933003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809933003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msw@chromium.org changed reviewers: + groby@chromium.org, hcarmona@chromium.org
+groby and hcarmona for CloseReason changes.
https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (left): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:284: EXPECT_EQ(BubbleDelegateView::CloseReason::DEACTIVATION, Are close reasons no longer important? Rename this test? https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (right): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:45: using BubbleDialogDelegateView::GetBubbleFrameView; This is redundant with GetBubbleFrameViewForTest; can you make them consistent? If you go with 'using', please do the same for SetAnchor[Rect|View]ForTest(). https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:59: Widget* CreateTestWidget() { nit: this could be a static local function, then we don't need the subclass. https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:71: /* You probably meant to remove this. https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:86: scoped_ptr<Widget> anchor_widget(CreateTestWidget()); Every test does this same pattern (anchor widget, new TestBubbleBlahBlah), maybe make that part of a Setup override or ctor for the test subclass? https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:305: scoped_ptr<Widget> anchor_widget(CreateTestWidget()); These should probably be three separate tests... https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:143: // Allow dialogs to show the system menu and be dragged. nit: "allow non-bubble dialogs"? https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.h:95: FRIEND_TEST_ALL_PREFIXES(BubbleDialogDelegateTest, CloseReasons); Can you expose more functions in the Test* subclasses to avoid friend tests?
On 2016/03/17 17:05:08, msw wrote: > +groby and hcarmona for CloseReason changes. lgtm for CloseReason changes. Doesn't look like that code is used outside this class.
indeed, the closereason code is just copy-pasted from BubbleDelegateView and is as yet unused in BubbleDialogDelegateView. I can add it back one day if necessary. https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (left): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:284: EXPECT_EQ(BubbleDelegateView::CloseReason::DEACTIVATION, On 2016/03/17 17:28:26, msw wrote: > Are close reasons no longer important? Rename this test? Done. https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (right): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:45: using BubbleDialogDelegateView::GetBubbleFrameView; On 2016/03/17 17:28:26, msw wrote: > This is redundant with GetBubbleFrameViewForTest; can you make them consistent? > If you go with 'using', please do the same for SetAnchor[Rect|View]ForTest(). Done. https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:59: Widget* CreateTestWidget() { On 2016/03/17 17:28:26, msw wrote: > nit: this could be a static local function, then we don't need the subclass. CreateParams is a (non-static) ViewsTestBase method https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:71: /* On 2016/03/17 17:28:26, msw wrote: > You probably meant to remove this. right you are https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:86: scoped_ptr<Widget> anchor_widget(CreateTestWidget()); On 2016/03/17 17:28:26, msw wrote: > Every test does this same pattern (anchor widget, new TestBubbleBlahBlah), maybe > make that part of a Setup override or ctor for the test subclass? that seems tricky because most tests do something to the bubble delegate before creating the bubble widget (the common pattern has a step in the middle that's unique to each test). https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_dial... ui/views/bubble/bubble_dialog_delegate_unittest.cc:305: scoped_ptr<Widget> anchor_widget(CreateTestWidget()); On 2016/03/17 17:28:26, msw wrote: > These should probably be three separate tests... I've seen both styles used. I'm sort of ambivalent. I'm happy to keep logically related test cases in the same test. https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:143: // Allow dialogs to show the system menu and be dragged. On 2016/03/17 17:28:26, msw wrote: > nit: "allow non-bubble dialogs"? I think the code is the best documentation in this case (should be fairly easy to read). https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/1809933003/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.h:95: FRIEND_TEST_ALL_PREFIXES(BubbleDialogDelegateTest, CloseReasons); On 2016/03/17 17:28:26, msw wrote: > Can you expose more functions in the Test* subclasses to avoid friend tests? this is just for access to close_, and there's no test bubble frame view to expose close_ to. If it makes you feel better the BubbleDelegateTest one will go away one day.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809933003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809933003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You'll need CloseReason again if/when you convert ExtensionMessageBubbleView or ToolbarActionsBarBubbleViews (etc.) it might be nice for both of us to avoid that churn, but whatever. lgtm
On 2016/03/18 00:49:42, msw wrote: > You'll need CloseReason again if/when you convert ExtensionMessageBubbleView or > ToolbarActionsBarBubbleViews (etc.) it might be nice for both of us to avoid > that churn, but whatever. lgtm IIRC these are BubbleDelegateView not BubbleDialogDelegateView, so probably OK. LGTM
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/1809933003/#ps20001 (title: "msw review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809933003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809933003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estade@chromium.org changed reviewers: + sky@chromium.org
+sky TBR for ui/views/views.gyp
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809933003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809933003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Port BubbleDelegate tests to BubbleDialogDelegate BUG=588877 ========== to ========== Port BubbleDelegate tests to BubbleDialogDelegate BUG=588877 TBR=sky@chromium.org ==========
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809933003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809933003/20001
Message was sent while issue was closed.
Description was changed from ========== Port BubbleDelegate tests to BubbleDialogDelegate BUG=588877 TBR=sky@chromium.org ========== to ========== Port BubbleDelegate tests to BubbleDialogDelegate BUG=588877 TBR=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Port BubbleDelegate tests to BubbleDialogDelegate BUG=588877 TBR=sky@chromium.org ========== to ========== Port BubbleDelegate tests to BubbleDialogDelegate BUG=588877 TBR=sky@chromium.org Committed: https://crrev.com/1b8cd416b2eb6eadc565b9b3a3be13de8a424c87 Cr-Commit-Position: refs/heads/master@{#382045} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1b8cd416b2eb6eadc565b9b3a3be13de8a424c87 Cr-Commit-Position: refs/heads/master@{#382045} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
