Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950913002:
Added new features tests to AppListPresenterDelegateUnittests. (Closed)
DescriptionAdded new feature tests to AppListPresenterDelegateUnittests.
BUG=734743
Review-Url: https://codereview.chromium.org/2950913002
Cr-Commit-Position: refs/heads/master@{#482323}
Committed: https://chromium.googlesource.com/chromium/src/+/ab3dacbb34125806290d54f9e02d6ffaf2f8ff3c
Patch Set 1 : Added new features tests to AppListPresenterDelegateUnittests. #Patch Set 2 : Added new features tests to AppListPresenterDelegateUnittests. Added early test returns for Config:… #
Total comments: 4
Patch Set 3 : Added new feature tests to AppListPresenterDelegateUnittests. Added early test returns for Config::… #
Total comments: 4
Patch Set 4 : Added new feature tests to AppListPresenterDelegateUnittests. Added early test returns for Config::… #
Total comments: 4
Patch Set 5 : Added new feature tests to AppListPresenterDelegateUnittests. Added early test returns for Config::… #
Total comments: 2
Patch Set 6 : Added new feature tests to AppListPresenterDelegateUnittests. Added early test returns for Config::… #Patch Set 7 : Updated method to GetRootWindow after shellport code changed. #
Total comments: 2
Patch Set 8 : Added new feature tests to AppListPresenterDelegateUnittests. Added early test returns for Config::… #Patch Set 9 : Added new feature tests to AppListPresenterDelegateUnittests. Added early test returns for Config::… #Patch Set 10 : fixed patch error. #Patch Set 11 : patched on top of rebase. #Patch Set 12 : Fixed function call that was not namespaced after rebase. #Patch Set 13 : Added failing tests to the mash filter. #Patch Set 14 : Added missing test to filter, alphabetized filtered tests. #
Messages
Total messages: 76 (58 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Added new features tests to AppListPresenterDelegateUnittests. BUG=734743 ========== to ========== Added new feature tests to AppListPresenterDelegateUnittests. BUG=734743 ==========
The CQ bit was checked by newcomer@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: 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_...)
newcomer@chromium.org changed reviewers: + vadimt@chromium.org
Vadim, PTAL! -Alex
newcomer@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... ui/events/gesture_event_details.h:100: DCHECK(ET_SCROLL_FLING_START == type_ || ET_GESTURE_SCROLL_UPDATE == type_); Where do we set the velocity for SCROLL_UPDATE?
Responded to sadrul@ https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... ui/events/gesture_event_details.h:100: DCHECK(ET_SCROLL_FLING_START == type_ || ET_GESTURE_SCROLL_UPDATE == type_); On 2017/06/20 20:43:38, sadrul wrote: > Where do we set the velocity for SCROLL_UPDATE? It seems like it is not set: https://cs.chromium.org/chromium/src/ui/events/gesture_event_details.cc?dr=CS... but this code works as intended here: https://cs.chromium.org/chromium/src/ui/app_list/views/app_list_view.cc?q=app... So maybe it's working by accident somehow?
https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... ui/events/gesture_event_details.h:100: DCHECK(ET_SCROLL_FLING_START == type_ || ET_GESTURE_SCROLL_UPDATE == type_); On 2017/06/20 21:23:34, newcomer wrote: > On 2017/06/20 20:43:38, sadrul wrote: > > Where do we set the velocity for SCROLL_UPDATE? > > It seems like it is not set: > https://cs.chromium.org/chromium/src/ui/events/gesture_event_details.cc?dr=CS... > > but this code works as intended here: > https://cs.chromium.org/chromium/src/ui/app_list/views/app_list_view.cc?q=app... > > So maybe it's working by accident somehow? I think that code wants scroll_y(), and it happens to work because data_ is a union, and things happen to line up. You should keep the DCHECK_EQ, and update app-list-view to use the right thing.
Thanks Sadrul@! vadimt@: ready for review, PTAL. -Alex https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2950913002/diff/60001/ui/events/gesture_event... ui/events/gesture_event_details.h:100: DCHECK(ET_SCROLL_FLING_START == type_ || ET_GESTURE_SCROLL_UPDATE == type_); On 2017/06/20 21:39:00, sadrul wrote: > On 2017/06/20 21:23:34, newcomer wrote: > > On 2017/06/20 20:43:38, sadrul wrote: > > > Where do we set the velocity for SCROLL_UPDATE? > > > > It seems like it is not set: > > > https://cs.chromium.org/chromium/src/ui/events/gesture_event_details.cc?dr=CS... > > > > but this code works as intended here: > > > https://cs.chromium.org/chromium/src/ui/app_list/views/app_list_view.cc?q=app... > > > > So maybe it's working by accident somehow? > > I think that code wants scroll_y(), and it happens to work because data_ is a > union, and things happen to line up. You should keep the DCHECK_EQ, and update > app-list-view to use the right thing. Oh, interesting. Done! Thanks for your help.
https://codereview.chromium.org/2950913002/diff/80001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate_unittest.cc:215: SetShelfAlignment(ShelfAlignment::SHELF_ALIGNMENT_LEFT); Will this alignment stick and influence other tests? Or it doesn't matter? https://codereview.chromium.org/2950913002/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate_unittest.cc:244: return; Empty line below.
newcomer@chromium.org changed reviewers: - sadrul@chromium.org
Thanks vadimt@! Responded to your comments. https://codereview.chromium.org/2950913002/diff/80001/ash/app_list/app_list_p... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate_unittest.cc:215: SetShelfAlignment(ShelfAlignment::SHELF_ALIGNMENT_LEFT); On 2017/06/21 00:05:59, vadimt wrote: > Will this alignment stick and influence other tests? Or it doesn't matter? I just tested this, it does not influence other tests. https://codereview.chromium.org/2950913002/diff/80001/ash/app_list/app_list_p... ash/app_list/app_list_presenter_delegate_unittest.cc:244: return; On 2017/06/21 00:05:59, vadimt wrote: > Empty line below. Done.
lgtm
newcomer@chromium.org changed reviewers: + sky@chromium.org, xiyuan@chromium.org
newcomer@chromium.org changed reviewers: + oshima@chromium.org - sky@chromium.org
The promised tests for AppListView and one bug fix. PTAL! -Alex
https://codereview.chromium.org/2950913002/diff/100001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/100001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:210: // Todo(Newcomer): Investigate mash failures crbug.com/726838 nit: Todo(Newcomer) -> TODO(newcomer), here and other places https://codereview.chromium.org/2950913002/diff/100001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2950913002/diff/100001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:859: GetWidget()->Close(); Why do we need to do this? delegate_->Dismiss() down below should close app list eventually.
Thanks for the comments xiyuan@! PTAL. -Alex https://codereview.chromium.org/2950913002/diff/100001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/100001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:210: // Todo(Newcomer): Investigate mash failures crbug.com/726838 On 2017/06/21 18:16:18, xiyuan wrote: > nit: Todo(Newcomer) -> TODO(newcomer), here and other places done! https://codereview.chromium.org/2950913002/diff/100001/ui/app_list/views/app_... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/2950913002/diff/100001/ui/app_list/views/app_... ui/app_list/views/app_list_view.cc:859: GetWidget()->Close(); On 2017/06/21 18:16:18, xiyuan wrote: > Why do we need to do this? delegate_->Dismiss() down below should close app list > eventually. I changed it so I could test it more easily. I agree, and I modified the tests to check for this.app_list_state() == CLOSED instead of forcing the widget to close.
lgtm https://codereview.chromium.org/2950913002/diff/120001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/120001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:210: // TODO(Newcomer): Investigate mash failures crbug.com/726838 Newcomer -> newcomer, as this should be your @chromium.org account id.
TY! On Wed, Jun 21, 2017 at 1:40 PM, <xiyuan@chromium.org> wrote: > lgtm > > > > > https://codereview.chromium.org/2950913002/diff/120001/ > ash/app_list/app_list_presenter_delegate_unittest.cc > File ash/app_list/app_list_presenter_delegate_unittest.cc (right): > > https://codereview.chromium.org/2950913002/diff/120001/ > ash/app_list/app_list_presenter_delegate_unittest.cc#newcode210 > ash/app_list/app_list_presenter_delegate_unittest.cc:210: // > TODO(Newcomer): Investigate mash failures crbug.com/726838 > Newcomer -> newcomer, as this should be your @chromium.org account id. > > https://codereview.chromium.org/2950913002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by newcomer@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: 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_...)
The CQ bit was checked by newcomer@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.
nice tests! just one request. https://codereview.chromium.org/2950913002/diff/160001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/160001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:212: return; can you use filter instead? mustash team prefer that way now. testing/buildbot/filters/ash_unittests_mus.filter
Responded to comments! -Alex https://codereview.chromium.org/2950913002/diff/120001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/120001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:210: // TODO(Newcomer): Investigate mash failures crbug.com/726838 On 2017/06/21 20:40:52, xiyuan wrote: > Newcomer -> newcomer, as this should be your @chromium.org account id. Done! Thanks. https://codereview.chromium.org/2950913002/diff/160001/ash/app_list/app_list_... File ash/app_list/app_list_presenter_delegate_unittest.cc (right): https://codereview.chromium.org/2950913002/diff/160001/ash/app_list/app_list_... ash/app_list/app_list_presenter_delegate_unittest.cc:212: return; On 2017/06/22 00:06:25, oshima wrote: > can you use filter instead? mustash team prefer that way now. > > testing/buildbot/filters/ash_unittests_mus.filter Done.
lgtm
The CQ bit was checked by newcomer@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: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by newcomer@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #9 (id:200001) has been deleted
Patchset #8 (id:180001) has been deleted
The CQ bit was checked by newcomer@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 checked by newcomer@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 checked by newcomer@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 newcomer@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: 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 newcomer@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: 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_...)
The CQ bit was checked by newcomer@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: 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_...)
The CQ bit was checked by newcomer@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.
The CQ bit was checked by newcomer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org, xiyuan@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2950913002/#ps340001 (title: "Added missing test to filter, alphabetized filtered tests.")
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": 340001, "attempt_start_ts": 1498500585072580,
"parent_rev": "1004a7a4a4f6aca1422caa28aa9e26741e6760f9", "commit_rev":
"ab3dacbb34125806290d54f9e02d6ffaf2f8ff3c"}
Message was sent while issue was closed.
Description was changed from ========== Added new feature tests to AppListPresenterDelegateUnittests. BUG=734743 ========== to ========== Added new feature tests to AppListPresenterDelegateUnittests. BUG=734743 Review-Url: https://codereview.chromium.org/2950913002 Cr-Commit-Position: refs/heads/master@{#482323} Committed: https://chromium.googlesource.com/chromium/src/+/ab3dacbb34125806290d54f9e02d... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:340001) as https://chromium.googlesource.com/chromium/src/+/ab3dacbb34125806290d54f9e02d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
