Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1755513002:
Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc (Closed)
|
Created:
4 years, 9 months ago by tapted Modified:
4 years, 9 months ago Reviewers:
msw CC:
chromium-reviews, tfarina, dcheng, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc
The tests were instantiated as
#if ash && chromeos
#elif ash
#endif
Since we've droped USE_ASH form most configurations, this means tests
were no longer being instantiated anywhere except ChromeOS.
Committed: https://crrev.com/964dcffcac8d9e599a5f66fd977c309bf03007d7
Cr-Commit-Position: refs/heads/master@{#378602}
Patch Set 1 #Patch Set 2 : Nested #if chromeos #
Total comments: 7
Messages
Total messages: 15 (8 generated)
Description was changed from ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as Since we've droped USE_ASH, this means tests are no longer instantiated anywhere except ChromeOS. ========== to ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && !chromeos #elif ash #endif Since we've droped USE_ASH, this means tests are no longer instantiated anywhere except ChromeOS. ==========
Description was changed from ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && !chromeos #elif ash #endif Since we've droped USE_ASH, this means tests are no longer instantiated anywhere except ChromeOS. ========== to ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && !chromeos #elif ash #endif Since we've droped USE_ASH, this means tests are no longer instantiated anywhere except ChromeOS. ==========
Description was changed from ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && !chromeos #elif ash #endif Since we've droped USE_ASH, this means tests are no longer instantiated anywhere except ChromeOS. ========== to ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && !chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
Hi Michael, please take a look. I've verified the tests are now running. E.g. comparing a trybot run on another cl: https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel... with on here: https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel... (look for "TabDragging/") https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1510: #if defined(OS_CHROMEOS) (balanced on line 2341) https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2341: #endif // OS_CHROMEOS This actually balances the #if defined(OS_CHROMEOS) on line 1510. The one I've deleted on line 2258 was nested inside it.
That file is unwieldy, so many namespaces and ifdefs. lgtm with a cl desc nit and a q; wanna try to enable more? https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (left): https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2347: #if defined(USE_ASH) && defined(OS_CHROMEOS) // TODO(win_ash,linux_ash) You CL desc says "#if ash && !chromeos", but this was "#if ash && chromeos". https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2348: INSTANTIATE_TEST_CASE_P(TabDragging, Did you try any of these with just USE_AURA? (it's fine if they fail or won't compile, but neat if they pass) https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2341: #endif // OS_CHROMEOS On 2016/03/01 08:28:52, tapted wrote: > This actually balances the #if defined(OS_CHROMEOS) on line 1510. The one I've > deleted on line 2258 was nested inside it. Acknowledged.
Description was changed from ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && !chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. ========== to ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. ==========
https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (left): https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2347: #if defined(USE_ASH) && defined(OS_CHROMEOS) // TODO(win_ash,linux_ash) On 2016/03/01 18:08:32, msw wrote: > You CL desc says "#if ash && !chromeos", but this was "#if ash && chromeos". oops - fixed. (I probably had it right the first time, but of course `cl upload` removed the lines that started with a '#' :p) https://codereview.chromium.org/1755513002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2348: INSTANTIATE_TEST_CASE_P(TabDragging, On 2016/03/01 18:08:32, msw wrote: > Did you try any of these with just USE_AURA? > (it's fine if they fail or won't compile, but neat if they pass) Yeah.. so like you say this file is in a sorry state. The extra test harnesses like DetachToBrowserInSeparateDisplayTabDragControllerTest and DifferentDeviceScaleFactorDisplayTabDragControllerTest DetachToBrowserTabDragControllerTestTouch are currently inside `#if chromeos` so they won't instantiate with aura unless that changes. And.. all the test cases inside that block are DISABLED_ in any case :/ I think we can try adding "touch" to DetachToBrowserTabDragControllerTest and see if that works on aura, but perhaps in a follow-up since it hasn't been tried outside of ChromeOS before. First priority is to bring back the lost coverage when USE_ASH went away on Windows. Bringing up more on aura, and re-enabling the DISABLED_ stuff on ChromeOS, seem to be blocked on some harder problems. Right now we're looking at bringing up as much of this as we can (and makes sense to) for Mac, and there could be some opportunity to neaten all this up. E.g. I want to move all the tests that are clearly only meant for ChromeOS to a _chromeos.cc file (those relying on ash::wm, for example).
gotcha; that's totally fair. still lgtm
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1755513002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1755513002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. ========== to ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. ========== to ========== Fix ifdefs around INSTANTIATE_TEST_CASE_P in tab_drag_controller_interactive_uitest.cc The tests were instantiated as #if ash && chromeos #elif ash #endif Since we've droped USE_ASH form most configurations, this means tests were no longer being instantiated anywhere except ChromeOS. Committed: https://crrev.com/964dcffcac8d9e599a5f66fd977c309bf03007d7 Cr-Commit-Position: refs/heads/master@{#378602} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/964dcffcac8d9e599a5f66fd977c309bf03007d7 Cr-Commit-Position: refs/heads/master@{#378602} |
