Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
301613002:
Enable Tab Dragging Tests with Linux and Ash (Closed)
|
Created:
6 years, 7 months ago by jonross Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina, dcheng Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionEnable Tab Dragging Tests with Linux and Ash
Enable a Shell for tests that exist in Linux with Ash.
TEST=TabDragControllerInteractiveUiTest
BUG=330429
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274311
Patch Set 1 #Patch Set 2 : Update Defines and Disabled Tests #
Total comments: 10
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Rebase #Messages
Total messages: 13 (0 generated)
- I ran the tests locally and not all of the tests that you have reenabled pass. (which is ok, but they should not be reenabled yet) - The goal is to get the Linux path to be as similar as possible to the Windows path. Replacing "#if defined(USE_ASH) && !defined(OS_WIN)" with "#if defined(OS_CHROMEOS)" is the correct thing to do. - I do not mind if some of the todos get killed. To my knowledge, there has not been an effort of running browser_tests or interactive_ui_tests on Win Ash. I have yet to see http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20Aura%20Ash... green. - Please remove any unneeded includes from your CL too I am kind of weird but I like getting a message asking for a review. For instance, for this CL I am unsure whether you are still working on the CL or wanted a review.
I've updated the tests to account for the ones that you saw failing. Changing the defines to use OS_CHROMEOS led to me updating some disabled tests. They are now always off, as they only compile in on ChromeOS. We could almost look at this entire section to see if it is still useful. Some small cpplint items updated. When I added you it was more to get an opinion of if this was the correct way to approach the update. However I forgot to specify that in the initial message. I'll make sure to be more specific in the future. I do feel now that this is at a state to begin review. https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (left): https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:51: #include "ash/wm/window_state.h" cpplint https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:7: #include <algorithm> Cpplint https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:340: // tab_strip->tab_at(1)->OnGestureEvent(&gesture_begin); cpplint https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1712: work_area.Inset(20, 20, 20, 60); cpplint
https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:340: // tab_strip->tab_at(1)->OnGestureEvent(&gesture_begin); Can't we just get rid of this line? https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:369: #if defined(OS_CHROMEOS) Can this new code be removed? https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2264: #elif defined(USE_ASH) // TODO(linux_ash) Remove TODO
https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:340: // tab_strip->tab_at(1)->OnGestureEvent(&gesture_begin); On 2014/05/28 00:32:03, pkotwicz wrote: > Can't we just get rid of this line? Done. https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:369: #if defined(OS_CHROMEOS) On 2014/05/28 00:32:03, pkotwicz wrote: > Can this new code be removed? With this now restricted to chrome os, yes. Done. https://codereview.chromium.org/301613002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2264: #elif defined(USE_ASH) // TODO(linux_ash) On 2014/05/28 00:32:03, pkotwicz wrote: > Remove TODO Done.
LGTM with nit https://codereview.chromium.org/301613002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/301613002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:376: } Can this new code be removed?
Hi, Could you please review these changes to interactive tests for Linux? https://codereview.chromium.org/301613002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/301613002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:376: } On 2014/05/28 15:00:33, pkotwicz wrote: > Can this new code be removed? Done.
lgtm
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/301613002/60001
The CQ bit was unchecked by jonross@chromium.org
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jonross@chromium.org/301613002/70001
Message was sent while issue was closed.
Change committed as 274311 |
