Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966343002:
Port NotificationViewMD unit tests from NotificationViewTest. (Closed)
DescriptionPort NotificationViewMD unit tests from NotificationViewTest.
Ported unit tests from NotificationViewTest and created
NotificationViewMDTest. Also fixed bugs and check failures found during
porting the tests.
* Fix RemoveChildView null checking. (CHECK fails on debug builds.)
* Fix action button hover state inheritance.
Following tests are not ported from NotificationViewTest, mostly
because the feature is unimplemented or the test is not applicable.
* CreateOrUpdateTestSettingsButton
* TestLineLimits
* TestImageSizing
* SettingsButtonTest
* ViewOrderingTest
* FormatContextMessageTest
BUG=733948
TEST=out/Debug/message_center_unittests
Review-Url: https://codereview.chromium.org/2966343002
Cr-Commit-Position: refs/heads/master@{#484851}
Committed: https://chromium.googlesource.com/chromium/src/+/9a4a1121fae26a1727728888a41975bc15e3e62f
Patch Set 1 #Patch Set 2 : Added Slide tests. #
Total comments: 10
Patch Set 3 : Fix nits. #
Total comments: 12
Patch Set 4 : Resolve nits. #Patch Set 5 : Rebased. #
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by tetsui@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 ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. BUG=733948 TEST=out/Debug/message_center_unittests ========== to ========== WIP: Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. BUG=733948 TEST=out/Debug/message_center_unittests ==========
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 tetsui@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 ========== WIP: Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. BUG=733948 TEST=out/Debug/message_center_unittests ========== to ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. BUG=733948 TEST=out/Debug/message_center_unittests ==========
Description was changed from ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. BUG=733948 TEST=out/Debug/message_center_unittests ========== to ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. * Fix RemoveChildView null checking. (CHECK fails on debug builds.) * Fix action button hover state inheritance. BUG=733948 TEST=out/Debug/message_center_unittests ==========
Description was changed from ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. * Fix RemoveChildView null checking. (CHECK fails on debug builds.) * Fix action button hover state inheritance. BUG=733948 TEST=out/Debug/message_center_unittests ========== to ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. * Fix RemoveChildView null checking. (CHECK fails on debug builds.) * Fix action button hover state inheritance. Following tests are not ported from NotificationViewTest, mostly because the feature is unimplemented or the test is not applicable. * CreateOrUpdateTestSettingsButton * TestLineLimits * TestImageSizing * SettingsButtonTest * ViewOrderingTest * FormatContextMessageTest BUG=733948 TEST=out/Debug/message_center_unittests ==========
tetsui@chromium.org changed reviewers: + fukino@chromium.org, yoshiki@chromium.org
Please take a look. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:497: if (title_view_ != nullptr) nit: "if (title_view_)" is sufficient and consistent with the rest. https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:553: if (compact_title_message_view_ != nullptr) ditto https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:571: if (progress_bar_view_ != nullptr) ditto https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:620: if (icon_view_ != nullptr) ditto https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:717: if (widget != nullptr) { ditto
The CQ bit was checked by tetsui@chromium.org to run a CQ dry run
fukino@: Thanks! yoshiki@: PTAL for OWNERS review. https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... File ui/message_center/views/notification_view_md.cc (right): https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:497: if (title_view_ != nullptr) On 2017/07/06 12:44:10, fukino wrote: > nit: "if (title_view_)" is sufficient and consistent with the rest. Done. https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:553: if (compact_title_message_view_ != nullptr) On 2017/07/06 12:44:11, fukino wrote: > ditto Done. https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:571: if (progress_bar_view_ != nullptr) On 2017/07/06 12:44:10, fukino wrote: > ditto Done. https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:620: if (icon_view_ != nullptr) On 2017/07/06 12:44:11, fukino wrote: > ditto Done. https://codereview.chromium.org/2966343002/diff/20001/ui/message_center/views... ui/message_center/views/notification_view_md.cc:717: if (widget != nullptr) { On 2017/07/06 12:44:11, fukino wrote: > ditto Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm nits. I'm ooo today, so please feel free to submit it after addressing these comments. Thanks. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_view_md_unittest.cc (right): https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:55: Notification* notification() { return notification_.get(); } nit: const https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:56: views::Widget* widget() { return notification_view()->GetWidget(); } ditto https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:56: views::Widget* widget() { return notification_view()->GetWidget(); } How about storing an instance of widget when we create in SetUp() method and having a reference in this class, instead of referring the notification view's widget? Referring indirectly should be difficult to read. If you want to ensure the equality between the one which Setup() creates and the one in the notification view, please use DCHECK_EQ(..., ...). https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:60: static const SkColor kBitmapColor = SK_ColorGREEN; nit: constants are usually placed in a global anonymous namespace. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:106: notification_view_->SetIsNested(); // TODO(tetsui): ? nit: could you add detailed information about TODO? https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:272: EXPECT_TRUE(nullptr != notification_view()->title_view_); EXPECT_NE is better. and same below. I know the original code is wrong as well, and we need to fix it in sometime later. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:278: notification()->set_title(base::ASCIIToUTF16("")); nit: Just base::string16() or base::EmptyString16()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yoshiki@: Thanks! https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... File ui/message_center/views/notification_view_md_unittest.cc (right): https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:55: Notification* notification() { return notification_.get(); } On 2017/07/07 02:48:30, yoshiki wrote: > nit: const Done. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:56: views::Widget* widget() { return notification_view()->GetWidget(); } On 2017/07/07 02:48:30, yoshiki wrote: > How about storing an instance of widget when we create in SetUp() method and > having a reference in this class, instead of referring the notification view's > widget? Referring indirectly should be difficult to read. > > If you want to ensure the equality between the one which Setup() creates and the > one in the notification view, please use DCHECK_EQ(..., ...). Done. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:60: static const SkColor kBitmapColor = SK_ColorGREEN; On 2017/07/07 02:48:30, yoshiki wrote: > nit: constants are usually placed in a global anonymous namespace. Done. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:272: EXPECT_TRUE(nullptr != notification_view()->title_view_); On 2017/07/07 02:48:30, yoshiki wrote: > EXPECT_NE is better. and same below. > > I know the original code is wrong as well, and we need to fix it in sometime > later. Done. https://codereview.chromium.org/2966343002/diff/40001/ui/message_center/views... ui/message_center/views/notification_view_md_unittest.cc:278: notification()->set_title(base::ASCIIToUTF16("")); On 2017/07/07 02:48:30, yoshiki wrote: > nit: Just base::string16() or base::EmptyString16() Done.
The CQ bit was checked by tetsui@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org, yoshiki@chromium.org Link to the patchset: https://codereview.chromium.org/2966343002/#ps80001 (title: "Rebased.")
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": 80001, "attempt_start_ts": 1499407806974490,
"parent_rev": "c2908de779d1f322c368bba7eab39766e7b52a7a", "commit_rev":
"9a4a1121fae26a1727728888a41975bc15e3e62f"}
Message was sent while issue was closed.
Description was changed from ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. * Fix RemoveChildView null checking. (CHECK fails on debug builds.) * Fix action button hover state inheritance. Following tests are not ported from NotificationViewTest, mostly because the feature is unimplemented or the test is not applicable. * CreateOrUpdateTestSettingsButton * TestLineLimits * TestImageSizing * SettingsButtonTest * ViewOrderingTest * FormatContextMessageTest BUG=733948 TEST=out/Debug/message_center_unittests ========== to ========== Port NotificationViewMD unit tests from NotificationViewTest. Ported unit tests from NotificationViewTest and created NotificationViewMDTest. Also fixed bugs and check failures found during porting the tests. * Fix RemoveChildView null checking. (CHECK fails on debug builds.) * Fix action button hover state inheritance. Following tests are not ported from NotificationViewTest, mostly because the feature is unimplemented or the test is not applicable. * CreateOrUpdateTestSettingsButton * TestLineLimits * TestImageSizing * SettingsButtonTest * ViewOrderingTest * FormatContextMessageTest BUG=733948 TEST=out/Debug/message_center_unittests Review-Url: https://codereview.chromium.org/2966343002 Cr-Commit-Position: refs/heads/master@{#484851} Committed: https://chromium.googlesource.com/chromium/src/+/9a4a1121fae26a1727728888a419... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9a4a1121fae26a1727728888a419...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2965353002/ by mastiz@chromium.org. The reason for reverting is: message_center_unittests failing on chromium.memory/Linux Chromium OS ASan LSan Tests (1) BUG=739356.
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 484851 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
