Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2933963002:
[ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs. (Closed)
|
Created:
3 years, 6 months ago by justincohen Modified:
3 years, 6 months ago Reviewers:
rohitrao (ping after 24h) CC:
chromium-reviews, marq+watch_chromium.org, jdonnelly+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs.
BUG=730461, 733650
Review-Url: https://codereview.chromium.org/2933963002
Cr-Commit-Position: refs/heads/master@{#479708}
Committed: https://chromium.googlesource.com/chromium/src/+/9895b1f7e22db3bcfe0743ad100383216c1d5b37
Patch Set 1 #
Total comments: 1
Patch Set 2 : more omnibox fixes #
Total comments: 8
Patch Set 3 : comments #
Total comments: 3
Patch Set 4 : Wrap iOS11 checks #
Total comments: 2
Patch Set 5 : Add crbug #
Messages
Total messages: 22 (11 generated)
The CQ bit was checked by justincohen@chromium.org to run a CQ dry run
justincohen@chromium.org changed reviewers: + rohitrao@chromium.org
I'm sure this won't break everything. PTAL
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2933963002/diff/1/ios/chrome/browser/ui/omnib... File ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm (right): https://codereview.chromium.org/2933963002/diff/1/ios/chrome/browser/ui/omnib... ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:557: NSTextAlignment alignment = [self bestAlignmentForText:[self text]]; Why is it ok to always use [self text] here? Is this better or worse than updating this function to properly handle Natural alignments?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm going to extract the ios_util changes and commit those separately. https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm (right): https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:566: [self.tableView reloadData]; I don't understand this code at all. Why do we need to update rows after calling reloadData? https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:568: [UIView performWithoutAnimation:^{ Please add a comment explaining what's going on here and why we need performWithoutAnimation. https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:662: if (!scrollView.dragging) Nice, this feels much cleaner. https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm (right): https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:561: // TODO(crbug.com/730461): Remove this entire block once it's been tested Can you please update the bug with a detailed description of why you think this code is no longer necessary? The goal is to document this for later, and also to convince me that we don't need this code =)
https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm (right): https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:566: [self.tableView reloadData]; On 2017/06/14 14:54:11, rohitrao (ping after 24h) wrote: > I don't understand this code at all. Why do we need to update rows after > calling reloadData? material design! we lay out the rows ourselves below. https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:568: [UIView performWithoutAnimation:^{ On 2017/06/14 14:54:11, rohitrao (ping after 24h) wrote: > Please add a comment explaining what's going on here and why we need > performWithoutAnimation. Done. https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:662: if (!scrollView.dragging) On 2017/06/14 14:54:11, rohitrao (ping after 24h) wrote: > Nice, this feels much cleaner. good, not planning on putting this or above behind iOS11 flags. https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm (right): https://codereview.chromium.org/2933963002/diff/20001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm:561: // TODO(crbug.com/730461): Remove this entire block once it's been tested On 2017/06/14 14:54:11, rohitrao (ping after 24h) wrote: > Can you please update the bug with a detailed description of why you think this > code is no longer necessary? The goal is to document this for later, and also > to convince me that we don't need this code =) Done.
lgtm but I'm going back and forth on the iOS11 checks. Since this is omnibox code, we should probably be conservative use an iOS11 check for everything that we want to merge to 60. What bug is triggered by the update animations? What happens if you remove the part of the CL that puts code behind performWithoutAnimation? https://codereview.chromium.org/2933963002/diff/40001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm (right): https://codereview.chromium.org/2933963002/diff/40001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:38: const CGFloat kTopAndBottomPadding = 8.0; Why do we need to inset with this padding? If you remember why, this could use a comment. https://codereview.chromium.org/2933963002/diff/40001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:569: // doens't try to apply it's own animation. doesn't and its. https://codereview.chromium.org/2933963002/diff/40001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:664: if (!scrollView.dragging) Please add a comment here explaining why we need this check. Seems like it's because we programmatically set insets, which triggers a call to didScroll that we want to ignore. We only want to inform the machinery of a scroll if we know that the user is directly scrolling.
Ok, per my paranoia about breaking M60, I'd like to split the performWithoutAnimation change into a separate CL and wrap the .dragging change in an iOS11 check. We can merge the two iOS11-only changes to M60 and then remove the version checks on trunk. This should fix the two worst issues in the current app, and I don't think the row animation is so terrible that we need a fix in 60.
Description was changed from ========== [ios] Fix baseWritingDirection refactor bug. BUG= ========== to ========== [ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs. BUG=730461 ==========
ptal
lgtm https://codereview.chromium.org/2933963002/diff/60001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm (right): https://codereview.chromium.org/2933963002/diff/60001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:659: // Default to the dragging check once it's been tested on trunk. Is there a bug for this?
Description was changed from ========== [ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs. BUG=730461 ========== to ========== [ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs. BUG=730461, 733650 ==========
https://codereview.chromium.org/2933963002/diff/60001/ios/chrome/browser/ui/o... File ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm (right): https://codereview.chromium.org/2933963002/diff/60001/ios/chrome/browser/ui/o... ios/chrome/browser/ui/omnibox/omnibox_popup_material_view_controller.mm:659: // Default to the dragging check once it's been tested on trunk. On 2017/06/15 14:06:18, rohitrao (ping after 24h) wrote: > Is there a bug for this? Done.
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2933963002/#ps80001 (title: "Add crbug")
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": 80001, "attempt_start_ts": 1497538982056590,
"parent_rev": "146952e1e31a110acf80a53e1cb5510cb24b3d24", "commit_rev":
"9895b1f7e22db3bcfe0743ad100383216c1d5b37"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs. BUG=730461, 733650 ========== to ========== [ios] Fix omnibox baseWritingDirection and dismissal iOS11 bugs. BUG=730461, 733650 Review-Url: https://codereview.chromium.org/2933963002 Cr-Commit-Position: refs/heads/master@{#479708} Committed: https://chromium.googlesource.com/chromium/src/+/9895b1f7e22db3bcfe0743ad1003... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9895b1f7e22db3bcfe0743ad1003... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
