Adding search widget UI and functionality.

Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
To be added: localisation, animation.
Screenshots, compact: https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/view?usp=sharing
expanded: https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/view?usp=sharing

BUG=622743
Review-Url: https://codereview.chromium.org/2858253002
Cr-Commit-Position: refs/heads/master@{#470556}
Committed: https://chromium.googlesource.com/chromium/src/+/4f11b22aea4ef0b8a2e7b0ef9d4a4d18591b7fa8

[lody, 2017-05-04 11:56:10]

Description was changed from

==========
Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
Screenshots, compact:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/v...
expanded:https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/view?usp=sharing

BUG=622743
==========

to

==========
Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
Screenshots, compact:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/v...
expanded:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/v...

BUG=622743
==========

[lody, 2017-05-04 12:04:23]

lod@chromium.org changed reviewers:
+ stkhapugin@chromium.org

[lody, 2017-05-04 12:04:24]

Please review, thanks!

to come next: correct handling of ios9, animation.

[stkhapugin, 2017-05-04 13:12:10]

Good job with all that constraint juggling, and thanks for good documentation
for it!

Here's the first round of comments. Mostly it's ideological things for cocoa
development (code style, method names, responsibility distribution). It looks
like a lot, but it should be find-and-replace and cut-paste almost everywhere.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view.h (right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.h:27: // View for the
search widget. Shows a blinking cursor for a fake omnibox and
This is no longer accurate.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.h:45: -
(void)updateCopiedURL:(NSString*)copiedURL;
Should this be a setter on a property |copiedURL| instead?

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view.mm (right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:24: __weak
id<SearchWidgetViewActionTarget> _target;
nit: Since this is your only ivar, convert it to a property! :)

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:27: // The copied URL
value.
This comment is confusing to someone who's reading this code for the first time.
Please be more descriptive. 

Also, since this is a string, it needs to be named ...String, so
copiedURLString.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:53: // Creates the view
for the action buttons.
Depending on your class comment's descriptiveness, you might need to make this
more descriptive.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:54: -
(UIView*)createActionsView:
This should be:

- (UIView*)newActionsView;

(you shouldn't pass constraints, see below)

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:58: -
(void)createOpenCopiedURLView:(NSLayoutAnchor*)topAnchor
This method must return a UIView. Also, if you need to pass down the anchor, you
need to rename your method like so:
-
(UIView*)newOpenCopiedURLViewWithAnchorThatINeedForReasons:(NSLayoutAnchor*)anchor

as explained below, please don't pass the constraint array down.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:77: -
(void)setCopiedURLVisible:(BOOL)copiedURLVisible {
This should be below initWith...

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:105:
NSMutableArray<NSLayoutConstraint*>* constraintArray = [NSMutableArray array];
This potentially gives you a small performance bump, but makes the code much
less readable as you're passing this array around everywhere. It's also not the
best idea to pass a mutable array around. I strongly suggest rewriting in a way
where you never pass around constraint arrays. Instead:

- in this method, set up all constraints between |self| and its immediate
subviews
- inside the helper methods, set the constraints related to the created subview

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:108: @[
self.primaryEffectView, self.secondaryEffectView ]) {
Please make this array a local variable for improved readability, because as is
the identation is ugly.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:112: [constraintArray
addObjectsFromArray:@[
There's a helper for this.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:193: -
(UIView*)createActionView:(NSString*)title
This name should be:

newActionViewWithTitle: imageName: action:(SEL)selector // and no constraints
passed down

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:208: labelView.text =
title;
l10n?

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:209:
labelView.numberOfLines = 2;
also l10n?

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:211: labelView.font =
[UIFont systemFontOfSize:UIFont.smallSystemFontSize];
Does this work with gigantic dynamic type?

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:219: stack.spacing = 5;
make a constant

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:221: [self
addTapAction:actionSelector toView:stack];
Create a transparent UIButton and constrain to |stack|'s sides instead.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:246:
self.copiedButtonView = [[UIView alloc] initWithFrame:CGRectZero];
Just make this a UIButton to avoid having a gesture recognizer that you handle
manually

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:267: // 
NSLayoutConstraint* hairlineTopAnchor =
remove

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:313: -
(void)addTapAction:(SEL)action toView:(UIView*)view {
With two comments above, you'll no longer need this helper.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:320: // If the
copiedURL section is not visible, hide all the copiedURL section
All of this logic belongs to controller layer, not the view. You should instead
expose a set of APIs that tells the view to:

1. show or hide the copied URL section
2. tell this view to use certain strings for openCopiedURLTitleLabel and
copiedURLLabel

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:345:
self.openCopiedURLTitleLabel.text = @"Open Copied Link";
localization?

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:352:
self.copiedURLLabel.text = @"Links you copy will appear here.";
l10n?

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:354:
self.openCopiedURLTitleLabel.text = @"No Copied Link";
l10n

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view_controller.mm
(right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view_controller.mm:97:
[NSLayoutConstraint activateConstraints:@[
I think you can replace this code with a helper that pins all 4 sides of a view
to another view. Forgot what it's called though.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view_controller.mm:138: CGSize
fittingSize = [self.widgetView
Please add a newline and a comment for line 138 and below

[lody, 2017-05-04 15:19:59]

Description was changed from

==========
Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
Screenshots, compact:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/v...
expanded:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/v...

BUG=622743
==========

to

==========
Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
To be added: localisation, animation.
Screenshots, compact:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/v...
expanded:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/v...

BUG=622743
==========

[lpromero, 2017-05-05 14:01:59]

lpromero@chromium.org changed reviewers:
+ lpromero@chromium.org

[lpromero, 2017-05-05 14:02:00]

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view.h (right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.h:45: -
(void)updateCopiedURL:(NSString*)copiedURL;
On 2017/05/04 13:12:08, stkhapugin wrote:
> Should this be a setter on a property |copiedURL| instead?

Some people don't like non-trivial setters.

[lody, 2017-05-05 16:54:36]

Patchset #3 (id:40001) has been deleted

[lody, 2017-05-05 16:54:47]

Patchset #2 (id:20001) has been deleted

[lody, 2017-05-05 17:03:54]

Patchset #2 (id:60001) has been deleted

[lody, 2017-05-05 17:05:44]

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view.h (right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.h:27: // View for the
search widget. Shows a blinking cursor for a fake omnibox and
On 2017/05/04 13:12:08, stkhapugin wrote:
> This is no longer accurate.

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.h:45: -
(void)updateCopiedURL:(NSString*)copiedURL;
On 2017/05/04 13:12:08, stkhapugin wrote:
> Should this be a setter on a property |copiedURL| instead?

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.h:45: -
(void)updateCopiedURL:(NSString*)copiedURL;
On 2017/05/05 14:02:00, lpromero wrote:
> On 2017/05/04 13:12:08, stkhapugin wrote:
> > Should this be a setter on a property |copiedURL| instead?
> 
> Some people don't like non-trivial setters.

Seeing as how I already had a non-trivial setter for copiedURLVisible, I think
consistency is at least better.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view.mm (right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:24: __weak
id<SearchWidgetViewActionTarget> _target;
On 2017/05/04 13:12:08, stkhapugin wrote:
> nit: Since this is your only ivar, convert it to a property! :)

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:27: // The copied URL
value.
On 2017/05/04 13:12:08, stkhapugin wrote:
> This comment is confusing to someone who's reading this code for the first
time.
> Please be more descriptive. 
> 
> Also, since this is a string, it needs to be named ...String, so
> copiedURLString. 

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:53: // Creates the view
for the action buttons.
On 2017/05/04 13:12:09, stkhapugin wrote:
> Depending on your class comment's descriptiveness, you might need to make this
> more descriptive. 

Acknowledged.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:54: -
(UIView*)createActionsView:
On 2017/05/04 13:12:08, stkhapugin wrote:
> This should be:
> 
> - (UIView*)newActionsView;
> 
> (you shouldn't pass constraints, see below)

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:58: -
(void)createOpenCopiedURLView:(NSLayoutAnchor*)topAnchor
On 2017/05/04 13:12:09, stkhapugin wrote:
> This method must return a UIView. Also, if you need to pass down the anchor,
you
> need to rename your method like so:
> -
>
(UIView*)newOpenCopiedURLViewWithAnchorThatINeedForReasons:(NSLayoutAnchor*)anchor
> 
> as explained below, please don't pass the constraint array down.

This does not create a single view, so returning one would make no sense. I
changed the name to be more accurate (using section rather than view).

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:77: -
(void)setCopiedURLVisible:(BOOL)copiedURLVisible {
On 2017/05/04 13:12:09, stkhapugin wrote:
> This should be below initWith...

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:105:
NSMutableArray<NSLayoutConstraint*>* constraintArray = [NSMutableArray array];
On 2017/05/04 13:12:08, stkhapugin wrote:
> This potentially gives you a small performance bump, but makes the code much
> less readable as you're passing this array around everywhere. It's also not
the
> best idea to pass a mutable array around. I strongly suggest rewriting in a
way
> where you never pass around constraint arrays. Instead:
> 
> - in this method, set up all constraints between |self| and its immediate
> subviews
> - inside the helper methods, set the constraints related to the created
subview

As discussed, the passing around of constraints is somewhat necessary for icons.
I tried a method that we discussed, please check what you think of it.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:108: @[
self.primaryEffectView, self.secondaryEffectView ]) {
On 2017/05/04 13:12:09, stkhapugin wrote:
> Please make this array a local variable for improved readability, because as
is
> the identation is ugly.

(indentation is good, eyeballs are less good as seen offline ;) )

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:112: [constraintArray
addObjectsFromArray:@[
On 2017/05/04 13:12:08, stkhapugin wrote:
> There's a helper for this.

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:193: -
(UIView*)createActionView:(NSString*)title
On 2017/05/04 13:12:09, stkhapugin wrote:
> This name should be:
> 
> newActionViewWithTitle: imageName: action:(SEL)selector // and no constraints
> passed down

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:208: labelView.text =
title;
On 2017/05/04 13:12:09, stkhapugin wrote:
> l10n? 

Acknowledged.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:209:
labelView.numberOfLines = 2;
On 2017/05/04 13:12:09, stkhapugin wrote:
> also l10n?

Acknowledged.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:211: labelView.font =
[UIFont systemFontOfSize:UIFont.smallSystemFontSize];
On 2017/05/04 13:12:09, stkhapugin wrote:
> Does this work with gigantic dynamic type?

whoops, used the wrong api. It looks good now, I think.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:219: stack.spacing = 5;
On 2017/05/04 13:12:09, stkhapugin wrote:
> make a constant

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:221: [self
addTapAction:actionSelector toView:stack];
On 2017/05/04 13:12:08, stkhapugin wrote:
> Create a transparent UIButton and constrain to |stack|'s sides instead.

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:246:
self.copiedButtonView = [[UIView alloc] initWithFrame:CGRectZero];
On 2017/05/04 13:12:08, stkhapugin wrote:
> Just make this a UIButton to avoid having a gesture recognizer that you handle
> manually

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:267: // 
NSLayoutConstraint* hairlineTopAnchor =
On 2017/05/04 13:12:09, stkhapugin wrote:
> remove

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:313: -
(void)addTapAction:(SEL)action toView:(UIView*)view {
On 2017/05/04 13:12:08, stkhapugin wrote:
> With two comments above, you'll no longer need this helper. 

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:320: // If the
copiedURL section is not visible, hide all the copiedURL section
On 2017/05/04 13:12:09, stkhapugin wrote:
> All of this logic belongs to controller layer, not the view. You should
instead
> expose a set of APIs that tells the view to:
> 
> 1. show or hide the copied URL section
> 2. tell this view to use certain strings for openCopiedURLTitleLabel and
> copiedURLLabel

As seen offline, this is tbd.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:345:
self.openCopiedURLTitleLabel.text = @"Open Copied Link";
On 2017/05/04 13:12:08, stkhapugin wrote:
> localization?

I knew something was missing from the list of "things to do next". I'll update
the desc.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:352:
self.copiedURLLabel.text = @"Links you copy will appear here.";
On 2017/05/04 13:12:09, stkhapugin wrote:
> l10n?

Acknowledged.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view.mm:354:
self.openCopiedURLTitleLabel.text = @"No Copied Link";
On 2017/05/04 13:12:09, stkhapugin wrote:
> l10n

Acknowledged.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
File ios/chrome/search_widget_extension/search_widget_view_controller.mm
(right):

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view_controller.mm:97:
[NSLayoutConstraint activateConstraints:@[
On 2017/05/04 13:12:09, stkhapugin wrote:
> I think you can replace this code with a helper that pins all 4 sides of a
view
> to another view. Forgot what it's called though. 

Done.

https://codereview.chromium.org/2858253002/diff/1/ios/chrome/search_widget_ex...
ios/chrome/search_widget_extension/search_widget_view_controller.mm:138: CGSize
fittingSize = [self.widgetView
On 2017/05/04 13:12:09, stkhapugin wrote:
> Please add a newline and a comment for line 138 and below

Done.

[lody, 2017-05-09 13:57:00]

lod@chromium.org changed reviewers:
+ marq@chromium.org

[lody, 2017-05-09 13:57:00]

Marq, as stk and lpromero are both out this week (oops), could you please take
over reviewing this CL? Thank you.

[marq (ping after 24h), 2017-05-09 15:35:06]

On 2017/05/09 13:57:00, lody wrote:
> Marq, as stk and lpromero are both out this week (oops), could you please take
> over reviewing this CL? Thank you.

Yup, may not get to it until tomorrow though.

[marq (ping after 24h), 2017-05-09 16:01:43]

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
File ios/chrome/search_widget_extension/search_widget_view.mm (right):

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
ios/chrome/search_widget_extension/search_widget_view.mm:122: // This is made
easier by putting all the constraints into an array and
Activating constraints all at once is *somewhat* more efficient than activating
them one at a time, but not enough to warrant collecting them into a single
array like this. I would say that unless there is some functional reason to do
this, it's fine for each subroutine call to activate its own constraints.

[lody, 2017-05-09 16:17:57]

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
File ios/chrome/search_widget_extension/search_widget_view.mm (right):

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
ios/chrome/search_widget_extension/search_widget_view.mm:122: // This is made
easier by putting all the constraints into an array and
On 2017/05/09 16:01:43, marq (ping after 24h) wrote:
> Activating constraints all at once is *somewhat* more efficient than
activating
> them one at a time, but not enough to warrant collecting them into a single
> array like this. I would say that unless there is some functional reason to do
> this, it's fine for each subroutine call to activate its own constraints.

As seen offline, this is functionally necessary. Please tell me if this comment
should be improved.

[marq (ping after 24h), 2017-05-10 10:48:42]

LGTM. I still don't have a significantly better approach to constructing the
view hierarchy than this, so overall I'm OK with this approach.

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
File ios/chrome/search_widget_extension/search_widget_view.mm (right):

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
ios/chrome/search_widget_extension/search_widget_view.mm:122: // This is made
easier by putting all the constraints into an array and
On 2017/05/09 16:17:57, lody wrote:
> On 2017/05/09 16:01:43, marq (ping after 24h) wrote:
> > Activating constraints all at once is *somewhat* more efficient than
> activating
> > them one at a time, but not enough to warrant collecting them into a single
> > array like this. I would say that unless there is some functional reason to
do
> > this, it's fine for each subroutine call to activate its own constraints.
> 
> As seen offline, this is functionally necessary. Please tell me if this
comment
> should be improved.

My rough suggestion:

"The vibrancy effects require that the icons and circular buttons be added to
different parent views. This means that the constraints that position them
cannot be activated until all of the buttons are added to the stack view that
contains them, and that stack view itself is added to the view hierarchy. In
order to manage this, |constraints| is passed into each invocation of the button
creation method, so the constraints can be collected for activation once the
view hierarchy is complete."

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
ios/chrome/search_widget_extension/search_widget_view.mm:124:
NSMutableArray<NSLayoutConstraint*>* constraintArray = [NSMutableArray array];
Rename to 'constraints' (prefer not to include types in variable names).

[lody, 2017-05-10 12:54:02]

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
File ios/chrome/search_widget_extension/search_widget_view.mm (right):

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
ios/chrome/search_widget_extension/search_widget_view.mm:122: // This is made
easier by putting all the constraints into an array and
On 2017/05/10 10:48:42, marq (ping after 24h) wrote:
> On 2017/05/09 16:17:57, lody wrote:
> > On 2017/05/09 16:01:43, marq (ping after 24h) wrote:
> > > Activating constraints all at once is *somewhat* more efficient than
> > activating
> > > them one at a time, but not enough to warrant collecting them into a
single
> > > array like this. I would say that unless there is some functional reason
to
> do
> > > this, it's fine for each subroutine call to activate its own constraints.
> > 
> > As seen offline, this is functionally necessary. Please tell me if this
> comment
> > should be improved.
> 
> My rough suggestion:
> 
> "The vibrancy effects require that the icons and circular buttons be added to
> different parent views. This means that the constraints that position them
> cannot be activated until all of the buttons are added to the stack view that
> contains them, and that stack view itself is added to the view hierarchy. In
> order to manage this, |constraints| is passed into each invocation of the
button
> creation method, so the constraints can be collected for activation once the
> view hierarchy is complete."

perfect! thanks.

https://codereview.chromium.org/2858253002/diff/80001/ios/chrome/search_widge...
ios/chrome/search_widget_extension/search_widget_view.mm:124:
NSMutableArray<NSLayoutConstraint*>* constraintArray = [NSMutableArray array];
On 2017/05/10 10:48:42, marq (ping after 24h) wrote:
> Rename to 'constraints' (prefer not to include types in variable names).

Done.

[lody, 2017-05-10 12:54:06]

The CQ bit was checked by lod@chromium.org

[lody, 2017-05-10 12:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2858253002/#ps100001
(title: "comment and naming")

[commit-bot: I haz the power, 2017-05-10 12:54:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 14:01:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494420846838780,
"parent_rev": "20abc56a850ed875b6be6d5ed2f8eeafeca5371a", "commit_rev":
"4f11b22aea4ef0b8a2e7b0ef9d4a4d18591b7fa8"}

[commit-bot: I haz the power, 2017-05-10 14:01:38]

Description was changed from

==========
Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
To be added: localisation, animation.
Screenshots, compact:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/v...
expanded:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/v...

BUG=622743
==========

to

==========
Adding search widget UI and functionality.

Apologies for the large CL. Mostly a battle of wills with autolayout.
To be added: localisation, animation.
Screenshots, compact:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqdFEwWVFPTklfM2c/v...
expanded:
https://drive.google.com/a/chromium.org/file/d/0BxrfNSu8yqJqbG5VaUJFR1dOMDg/v...

BUG=622743

Review-Url: https://codereview.chromium.org/2858253002
Cr-Commit-Position: refs/heads/master@{#470556}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f11b22aea4ef0b8a2e7b0ef9d4a...
==========

[commit-bot: I haz the power, 2017-05-10 14:01:39]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4f11b22aea4ef0b8a2e7b0ef9d4a...