Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1071723002:
[Find in Page] Prevent scrolling past the page edge. (Closed)
|
Created:
5 years, 8 months ago by justincohen Modified:
5 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Find in Page] Prevent scrolling past the page edge.
Sometimes the find in page js returns a text position that is too far down.
Correct all |points| sent to -setContentOffset for overscroll.
BUG=472091
Committed: https://crrev.com/87c443e596cac6351b5b24cca5a8c84ce85a935f
Cr-Commit-Position: refs/heads/master@{#330518}
Patch Set 1 #
Total comments: 10
Patch Set 2 : sdefresne comments #Patch Set 3 : typo #
Total comments: 1
Patch Set 4 : Resurrected from the dead #Patch Set 5 : Ascii sort cmath #
Total comments: 2
Patch Set 6 : Remove static cast #Messages
Total messages: 16 (4 generated)
justincohen@chromium.org changed reviewers: + shreyasv@chromium.org
ptal
justincohen@chromium.org changed reviewers: + sdefresne@chromium.org
shreyasv@ for review, sdefresne@ for OWNERS
not lgtm until the self/strongSelf are fixed (I care less about std::max or API change) https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:61: // Prevent overscroll. API recommendation: since each invocation follow this pattern: point = [self limitOverscroll:someView atPoint:point]; [someView setContentOffset:point animated:YES]; I would recommend to instead have a method -[setContentOffset:point forView:someView animated:YES limitOverScroll:YES] instead. https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:143: CGFloat maxScroll = fmax(0, contentHeight - frameHeight); nit: please use std::max() from <cmath> https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:219: [self limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; point = [strongSelf limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:233: point = [self limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; point = [strongSelf limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:248: point = [self limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; point = [strongSelf limitOverscroll:[strongSelf webViewScrollView] atPoint:point];
Oops, big catch on the strongSelf, thanks. PTAL https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:61: // Prevent overscroll. On 2015/04/08 20:02:19, sdefresne wrote: > API recommendation: since each invocation follow this pattern: > > point = [self limitOverscroll:someView atPoint:point]; > [someView setContentOffset:point animated:YES]; > > I would recommend to instead have a method -[setContentOffset:point > forView:someView animated:YES limitOverScroll:YES] instead. messaged offline about this. https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:143: CGFloat maxScroll = fmax(0, contentHeight - frameHeight); On 2015/04/08 20:02:19, sdefresne wrote: > nit: please use std::max() from <cmath> Done. https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:219: [self limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; On 2015/04/08 20:02:18, sdefresne wrote: > point = [strongSelf limitOverscroll:[strongSelf webViewScrollView] > atPoint:point]; Done. https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:233: point = [self limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; On 2015/04/08 20:02:18, sdefresne wrote: > point = [strongSelf limitOverscroll:[strongSelf webViewScrollView] > atPoint:point]; Done. https://codereview.chromium.org/1071723002/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:248: point = [self limitOverscroll:[strongSelf webViewScrollView] atPoint:point]; On 2015/04/08 20:02:18, sdefresne wrote: > point = [strongSelf limitOverscroll:[strongSelf webViewScrollView] > atPoint:point]; Done.
https://codereview.chromium.org/1071723002/diff/40001/ios/chrome/browser/find... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/1071723002/diff/40001/ios/chrome/browser/find... ios/chrome/browser/find_in_page/find_in_page_controller.mm:140: - (CGPoint)limitoverscroll:(CRWWebViewScrollViewProxy*)scrollViewProxy Declaration does not match definition.
hold off on reviewing this, I spoke to sdefresne offline and will take a different approach.
Sorry for the ancient CL, but it's back. PTAL!
lgtm with nits https://codereview.chromium.org/1071723002/diff/80001/ios/chrome/browser/find... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/1071723002/diff/80001/ios/chrome/browser/find... ios/chrome/browser/find_in_page/find_in_page_controller.mm:144: CGFloat maxScroll = std::max((CGFloat)0, contentHeight - frameHeight); C-style cast are forbidden (and sadly casting constructor like "CGFloat(0)" or "CGFloat{0}"), I would recommend using "std::max<CGFloat>(0, contentHeight - frameHeight)" instead.
https://codereview.chromium.org/1071723002/diff/80001/ios/chrome/browser/find... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/1071723002/diff/80001/ios/chrome/browser/find... ios/chrome/browser/find_in_page/find_in_page_controller.mm:144: CGFloat maxScroll = std::max((CGFloat)0, contentHeight - frameHeight); On 2015/05/19 13:30:27, sdefresne wrote: > C-style cast are forbidden (and sadly casting constructor like "CGFloat(0)" or > "CGFloat{0}"), I would recommend using "std::max<CGFloat>(0, contentHeight - > frameHeight)" instead. Done.
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1071723002/#ps100001 (title: "Remove static cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1071723002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/87c443e596cac6351b5b24cca5a8c84ce85a935f Cr-Commit-Position: refs/heads/master@{#330518} |
