[ObjC ARC] Converts ios/chrome/browser/ui/toolbar:toolbar to ARC.

[ObjC ARC] Converts ios/chrome/browser/ui/toolbar:toolbar to ARC.

Reland of https://codereview.chromium.org/2808873002/

BUG=624363
TEST=None

Review-Url: https://codereview.chromium.org/2851303006
Cr-Commit-Position: refs/heads/master@{#468976}
Committed: https://chromium.googlesource.com/chromium/src/+/5aa19d360acef358288a23b8e9b69d1292bafc0a

[gambard, 2017-05-02 07:35:18]

gambard@chromium.org changed reviewers:
+ sdefresne@chromium.org, stkhapugin@chromium.org

[gambard, 2017-05-02 07:35:18]

PTAL, reland of https://codereview.chromium.org/2808873002/

[stkhapugin, 2017-05-02 12:12:26]

Thanks, lgtm. As a follow-up, please rename trailing underscore ivars to leading
underscore ivars. :)

https://codereview.chromium.org/2851303006/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm (right):

https://codereview.chromium.org/2851303006/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:121: if (i <
[pathLayers_ count]) {
please use dot notation: pathLayers_.count

https://codereview.chromium.org/2851303006/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:212:
DCHECK([pathLayers_ count] == kNumberOfDots);
ditto, pathLayers_.count or better yet, self.pathLayers.count

[gambard, 2017-05-02 12:58:36]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 12:58:47]

Dry run: CQ is trying da patch.

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

[gambard, 2017-05-02 12:58:58]

Thanks!

https://codereview.chromium.org/2851303006/diff/40001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm (right):

https://codereview.chromium.org/2851303006/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:121: if (i <
[pathLayers_ count]) {
On 2017/05/02 12:12:25, stkhapugin wrote:
> please use dot notation: pathLayers_.count

Done.

https://codereview.chromium.org/2851303006/diff/40001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:212:
DCHECK([pathLayers_ count] == kNumberOfDots);
On 2017/05/02 12:12:25, stkhapugin wrote:
> ditto, pathLayers_.count or better yet, self.pathLayers.count

Done.

[sdefresne, 2017-05-02 14:19:29]

I think the conversion is introducing a bug.

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_controller.h (right):

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_controller.h:115: // TODO(blundell):
Figure out how to fix this and have the top-level view be a
nit: change this to the following

// TODO(crbug.com/228469): Figure out how to fix this and have the top-level
// view be a UIView.

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm (right):

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:142: pathLayers_[i] =
pathLayer;
I don't think you can set objects past the NSArray "count".

If I read the documentation of NSMutableArray correctly [1], "pathLayers_[i] =
pathLayer;" is translated to "[pathLayers_ placeObjectAtIndex:i
withObject:pathLayer];". The method documentation [2], states that "The index of
the object to be replaced. This value must not exceed the bounds of the array.".

[1]: https://developer.apple.com/reference/foundation/nsmutablearray
[2]:
https://developer.apple.com/reference/foundation/nsmutablearray/1414510-repla...

If think this method should be slightly changed to work with an NSMutableArray.
I would recommend iterating twice, first time to call -removeFromSuperlayer on
all objects, then adding all new values. Something like this:

- (void)initializeShapeLayers {
  for (NSUInteger index = 0; index < pathLayers_.count; ++index) {
    [pathLayers_[index] removeFromSuperlayer];
  }
  pathLayers_ = [[NSMutableArray alloc] initWithCapacity:kNumberOfDots];
  for (NSUInteger index = 0; index < kNumberOfDots; ++index) {
    ...
    [pathLayers_ addObject:pathLayer];
  }
}

[commit-bot: I haz the power, 2017-05-02 14:20:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 14:20:44]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-05-02 15:19:02]

Thanks, PTAL.

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_controller.h (right):

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_controller.h:115: // TODO(blundell):
Figure out how to fix this and have the top-level view be a
On 2017/05/02 14:19:28, sdefresne wrote:
> nit: change this to the following
> 
> // TODO(crbug.com/228469): Figure out how to fix this and have the top-level
> // view be a UIView.

Done.

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm (right):

https://codereview.chromium.org/2851303006/diff/60001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:142: pathLayers_[i] =
pathLayer;
On 2017/05/02 14:19:28, sdefresne wrote:
> I don't think you can set objects past the NSArray "count".
> 
> If I read the documentation of NSMutableArray correctly [1], "pathLayers_[i] =
> pathLayer;" is translated to "[pathLayers_ placeObjectAtIndex:i
> withObject:pathLayer];". The method documentation [2], states that "The index
of
> the object to be replaced. This value must not exceed the bounds of the
array.".
> 
> [1]: https://developer.apple.com/reference/foundation/nsmutablearray
> [2]:
>
https://developer.apple.com/reference/foundation/nsmutablearray/1414510-repla...
> 
> If think this method should be slightly changed to work with an
NSMutableArray.
> I would recommend iterating twice, first time to call -removeFromSuperlayer on
> all objects, then adding all new values. Something like this:
> 
> - (void)initializeShapeLayers {
>   for (NSUInteger index = 0; index < pathLayers_.count; ++index) {
>     [pathLayers_[index] removeFromSuperlayer];
>   }
>   pathLayers_ = [[NSMutableArray alloc] initWithCapacity:kNumberOfDots];
>   for (NSUInteger index = 0; index < kNumberOfDots; ++index) {
>     ...
>     [pathLayers_ addObject:pathLayer];
>   }
> }

It seemed to work but it makes more sense to do it this way.

[sdefresne, 2017-05-03 12:34:16]

LG but I think there is some code incorrectly indented. Please fix.

https://codereview.chromium.org/2851303006/diff/80001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm (right):

https://codereview.chromium.org/2851303006/diff/80001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:122: }
Indentation appears to be incorrect here. I would expect this line and all the
following lines (up to line 143) to be indented by 2 or 4 spaces, not 4 or 6.
Please fix.

[gambard, 2017-05-03 14:21:32]

Thanks, PTAL.

https://codereview.chromium.org/2851303006/diff/80001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm (right):

https://codereview.chromium.org/2851303006/diff/80001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_tools_menu_button.mm:122: }
On 2017/05/03 12:34:16, sdefresne wrote:
> Indentation appears to be incorrect here. I would expect this line and all the
> following lines (up to line 143) to be indented by 2 or 4 spaces, not 4 or 6.
> Please fix.

Done.

[gambard, 2017-05-03 14:21:38]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 14:21:56]

Dry run: CQ is trying da patch.

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

[sdefresne, 2017-05-03 14:25:46]

lgtm

[gambard, 2017-05-03 14:26:40]

The CQ bit was unchecked by gambard@chromium.org

[gambard, 2017-05-03 14:26:40]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-05-03 14:26:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org
Link to the patchset: https://codereview.chromium.org/2851303006/#ps120001
(title: "Fix DCHECK")

[commit-bot: I haz the power, 2017-05-03 14:26:47]

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-03 15:21:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493821600659200,
"parent_rev": "478229abf628ef29be1e14518ac991654e256605", "commit_rev":
"5aa19d360acef358288a23b8e9b69d1292bafc0a"}

[commit-bot: I haz the power, 2017-05-03 15:21:57]

Description was changed from

==========
[ObjC ARC] Converts ios/chrome/browser/ui/toolbar:toolbar to ARC.

Reland of https://codereview.chromium.org/2808873002/

BUG=624363
TEST=None
==========

to

==========
[ObjC ARC] Converts ios/chrome/browser/ui/toolbar:toolbar to ARC.

Reland of https://codereview.chromium.org/2808873002/

BUG=624363
TEST=None

Review-Url: https://codereview.chromium.org/2851303006
Cr-Commit-Position: refs/heads/master@{#468976}
Committed:
https://chromium.googlesource.com/chromium/src/+/5aa19d360acef358288a23b8e9b6...
==========

[commit-bot: I haz the power, 2017-05-03 15:21:58]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5aa19d360acef358288a23b8e9b6...