Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
282143002:
mac: Add browser tests for history swiping. (Closed)
|
Created:
6 years, 7 months ago by erikchen Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionmac: Add browser tests for history swiping.
One of the tests replays recorded events from a real swipe. The others
simulate realistic conditions and test edge cases. The tests discovered two
bugs:
- History swiping cannot be triggered if the very first touch event consists of
only vertical motion.
- History swiping blocks vertical part of diagonal swipes.
BUG=375514, 375512
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272334
Patch Set 1 : Fix ordering of includes in gypi #Patch Set 2 : More tests! #Patch Set 3 : Disable tests on 10.6 #Patch Set 4 : Add crbug links. #Patch Set 5 : Add forward declarations for 10.6 #
Total comments: 20
Patch Set 6 : Address style comments from asvitkine. #
Total comments: 10
Patch Set 7 : More style comments from asvitkine. #
Messages
Total messages: 17 (0 generated)
thakis: More tests will follow once this one is submitted. This is in preparation for upcoming changes I plan on making to history swiping.
thakis: ping
asvitkine: Let me know if you have time to review this CL, as thakis is pretty swamped right now.
I haven't looked at the logic yet, but here's some style nits for you for now. Thanks for working on this, by the way! https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm (right): https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:23: namespace { Nit: Add an empty line after this. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:28: // 'touches{Began,Moved,Ended}WithEvent:' Nit: Make order of these consistent with the enums. Alternatively, just make these line-comments above the relevant enums, which makes it clearer what is referring to what without cross-referencing with this comment. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:33: DeploymentTouchesBegan, Nit: I think generally Chromium prefers FOO_BAR style for enums. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:39: }; Nit: Add an empty line after this. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:75: event_queue_ = nil; Nit: please initialize these via initializer list in this ctor and also set the other primitives (e.g. touchX_ and touchY_) or some memory tools may yell at you. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:112: int GetScrollTop() { return GetScriptIntValue("document.body.scrollTop"); } Nit: put body on separate line. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:137: NSEventPhase momentumPhase, Nit: Use hacker_style naming convention in C++ methods (please fix throughout). https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:142: [(NSEvent*)[[event stub] andReturnValue:OCMOCK_VALUE(phase)] phase]; Nit: Can you cache (NSEvent*)[[event stub] in a local var? https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:337: CGFloat touchX_; Nit: touch_x_ and touch_y_ or better use use a CGPoint. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:605: respondsToSelector:@selector(isSwipeTrackingFromScrollEventsEnabled)]) Nit: Can you make a small wrapper function around this at the top of the file instead of duplicating this in every test? e.g. IsHistorySwipingSupported()
asvitkine: Thanks for the fast first pass! PTAL https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm (right): https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:23: namespace { On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: Add an empty line after this. Done. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:28: // 'touches{Began,Moved,Ended}WithEvent:' On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: Make order of these consistent with the enums. > > Alternatively, just make these line-comments above the relevant enums, which > makes it clearer what is referring to what without cross-referencing with this > comment. I added line-comments above each of the enum values. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:33: DeploymentTouchesBegan, On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: I think generally Chromium prefers FOO_BAR style for enums. huh, okay, done! https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:39: }; On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: Add an empty line after this. Done. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:75: event_queue_ = nil; On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: please initialize these via initializer list in this ctor and also set the > other primitives (e.g. touchX_ and touchY_) or some memory tools may yell at > you. I've moved the trivial initializations to the initializer list. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:112: int GetScrollTop() { return GetScriptIntValue("document.body.scrollTop"); } On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: put body on separate line. Done. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:137: NSEventPhase momentumPhase, On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: Use hacker_style naming convention in C++ methods (please fix throughout). ack. been wriring too much ObjC. done. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:142: [(NSEvent*)[[event stub] andReturnValue:OCMOCK_VALUE(phase)] phase]; On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: Can you cache (NSEvent*)[[event stub] in a local var? Unfortunately not. Each ObjC method invocation on this line has a non-idempotent effect. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:337: CGFloat touchX_; On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: touch_x_ and touch_y_ or better use use a CGPoint. Converted to CGPoint. https://codereview.chromium.org/282143002/diff/120001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:605: respondsToSelector:@selector(isSwipeTrackingFromScrollEventsEnabled)]) On 2014/05/21 06:29:10, Alexei Svitkine wrote: > Nit: Can you make a small wrapper function around this at the top of the file > instead of duplicating this in every test? e.g. IsHistorySwipingSupported() Done.
LGTM with a few more nits https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm (right): https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:94: [event_queue_ release]; Nit: Use a scoped_nsobject instead and .release() here. https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:179: QueuedEvent* queued_event = [[[QueuedEvent alloc] init] autorelease]; Nit: Instead of repeating these 4 lines in a lot of these methods, how about having a helper QueueEvent(event, deployment, run_message_loop); method? https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:210: float y, Nit: CGFloat? https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:302: BOOL runLoop = queued_event.runMessageLoop; Nit: run_loop https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:316: Nit: Either consistently have an empty line between cases or consistently don't.
https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm (right): https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:94: [event_queue_ release]; On 2014/05/22 13:30:47, Alexei Svitkine wrote: > Nit: Use a scoped_nsobject instead and .release() here. I used a scoped_nsobject and .reset(), which doesn't have WARN_UNUSED_RESULT. https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:179: QueuedEvent* queued_event = [[[QueuedEvent alloc] init] autorelease]; On 2014/05/22 13:30:47, Alexei Svitkine wrote: > Nit: Instead of repeating these 4 lines in a lot of these methods, how about > having a helper QueueEvent(event, deployment, run_message_loop); method? Sure! https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:210: float y, On 2014/05/22 13:30:47, Alexei Svitkine wrote: > Nit: CGFloat? Done. https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:302: BOOL runLoop = queued_event.runMessageLoop; On 2014/05/22 13:30:47, Alexei Svitkine wrote: > Nit: run_loop Done. https://codereview.chromium.org/282143002/diff/140001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper_browsertest.mm:316: On 2014/05/22 13:30:47, Alexei Svitkine wrote: > Nit: Either consistently have an empty line between cases or consistently don't. I removed the empty lines to be consistent.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/282143002/160001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
thakis: looking for an owner LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
rslgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/282143002/160001
Message was sent while issue was closed.
Change committed as 272334 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
