New issue
Advanced search Search tips

Issue 826093 link

Starred by 1 user
Status: Fixed
Owner:
kkhorimoto@chromium.org
Closed: Apr 2018
Cc:
rohitrao@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: 2018-04-13
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

[iOS] BrowserContainerView should be a no-op if called with the currently displayed content view.

Project Member Reported by kkhorimoto@chromium.org, Mar 27 2018 
The current implementation will remove the previous content view from the hierarchy regardless of whether |_contentView| == |contentView|.  Any NamedGuides added to the BVC that are constrained to a subview of the |_contentView| will have its constraints deactivated if the content view is removed unecessarily.

Because this is a central point in UI setup for the app, this should be done just after M67 branch point to minimize any unintended side effects of this change.

Old implementation:

- (void)displayContentView:(UIView*)contentView {
  DCHECK(![_contentView superview] || [_contentView superview] == self);
  [_contentView removeFromSuperview];
  _contentView = contentView;

  if (contentView) {
    [self addSubview:contentView];
  }
}
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4afa8b5fa4a3e09a8ee3bca5e0d2cbd57bb0e39

commit e4afa8b5fa4a3e09a8ee3bca5e0d2cbd57bb0e39
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue Mar 27 20:44:31 2018

[iOS] Don't remove the old content view if re-displaying it.

BrowserContainerView's |-displayContentView:| should have no effect if
called twice with the same argument, but its current implementation
changes the view hierarchy in this case.

This CL is a partial solution to fix a specific issue with the voice
search button NamedGuide to minimize risk.

Bug:  826093 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ia2d58883df057df2ad430ecdbc7ccdf28f02a4cd
Reviewed-on: https://chromium-review.googlesource.com/981609
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546245}
[modify] https://crrev.com/e4afa8b5fa4a3e09a8ee3bca5e0d2cbd57bb0e39/ios/chrome/browser/ui/browser_container_view.mm

Comment 2 by monor...@bugs.chromium.org, Apr 13 2018 

The NextAction date has arrived: 2018-04-13

Comment 3 by kkhorimoto@chromium.org, Apr 20 2018

Status: Fixed (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 20 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d565f8638922e78e289ad7090b5bf6d7c7869360

commit d565f8638922e78e289ad7090b5bf6d7c7869360
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Apr 20 18:13:40 2018

[iOS] Fix BrowserContainerViewController's |-displayContentView|.

This function should be a no-op if called with the currently-displayed
content view.

Bug:  826093 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I5283f1723d4f2240c017e4561644d134d989869d
Reviewed-on: https://chromium-review.googlesource.com/1015979
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552391}
[modify] https://crrev.com/d565f8638922e78e289ad7090b5bf6d7c7869360/ios/chrome/browser/ui/browser_container_view_controller.mm
►

Sign in to add a comment