|
|
Created:
3 years, 10 months ago by stkhapugin Modified:
3 years, 8 months ago Reviewers:
lpromero CC:
chromium-reviews, marq+watch_chromium.org, tfarina, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ObjC ARC] Converts ios/chrome/browser/ui/bookmarks/bars:bars to ARC.
Automatically generated ARCMigrate commit
Notable issues:None
BUG=624363
TEST=None
Review-Url: https://codereview.chromium.org/2677443002
Cr-Commit-Position: refs/heads/master@{#463713}
Committed: https://chromium.googlesource.com/chromium/src/+/d914e02c97cb940507d6662b1b9a744d39b69a96
Patch Set 1 #
Total comments: 2
Patch Set 2 : asdf #
Total comments: 4
Patch Set 3 : comments #Patch Set 4 : fix typo #Patch Set 5 : I'm dumb #
Messages
Total messages: 35 (22 generated)
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
stkhapugin@chromium.org changed reviewers: + noyau@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2677443002/diff/1/ios/chrome/browser/ui/bookm... File ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm (right): https://codereview.chromium.org/2677443002/diff/1/ios/chrome/browser/ui/bookm... ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm:12: @property(nonatomic, strong) UIView* contentView; The only place the content view is assigned is in init. Is the redefinition of the property really needed? I would assign the instance variable directly.
ptal https://codereview.chromium.org/2677443002/diff/1/ios/chrome/browser/ui/bookm... File ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm (right): https://codereview.chromium.org/2677443002/diff/1/ios/chrome/browser/ui/bookm... ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm:12: @property(nonatomic, strong) UIView* contentView; On 2017/02/03 12:24:24, noyau wrote: > The only place the content view is assigned is in init. Is the redefinition of > the property really needed? I would assign the instance variable directly. Done.
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping
Description was changed from ========== [ObjC ARC] Converts ios/chrome/browser/ui/bookmarks/bars:bars to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/chrome/browser/ui/bookmarks/bars:bars to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ==========
stkhapugin@chromium.org changed reviewers: + lpromero@chromium.org - noyau@chromium.org
ptal
lgtm https://codereview.chromium.org/2677443002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm (right): https://codereview.chromium.org/2677443002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm:24: UIView* contentView = [[UIView alloc] init]; Use _contentView here. See below. https://codereview.chromium.org/2677443002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm:25: self.contentView.backgroundColor = [UIColor clearColor]; This is a bug, as self.contentView is nil here. You might also nuke the line as we seem to have lived without its effect for quite some time…
Thanks! https://codereview.chromium.org/2677443002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm (right): https://codereview.chromium.org/2677443002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm:24: UIView* contentView = [[UIView alloc] init]; On 2017/03/10 15:45:14, lpromero wrote: > Use _contentView here. See below. Done. https://codereview.chromium.org/2677443002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bars/bookmark_top_bar.mm:25: self.contentView.backgroundColor = [UIColor clearColor]; On 2017/03/10 15:45:15, lpromero wrote: > This is a bug, as self.contentView is nil here. You might also nuke the line as > we seem to have lived without its effect for quite some time… Done.
The CQ bit was checked by stkhapugin@chromium.org
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2677443002/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2677443002/#ps60001 (title: "fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2677443002/#ps80001 (title: "I'm dumb")
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": 1491928046662720, "parent_rev": "e6bb4131843342dbfede940f89a918cc29a399f9", "commit_rev": "d914e02c97cb940507d6662b1b9a744d39b69a96"}
Message was sent while issue was closed.
Description was changed from ========== [ObjC ARC] Converts ios/chrome/browser/ui/bookmarks/bars:bars to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None ========== to ========== [ObjC ARC] Converts ios/chrome/browser/ui/bookmarks/bars:bars to ARC. Automatically generated ARCMigrate commit Notable issues:None BUG=624363 TEST=None Review-Url: https://codereview.chromium.org/2677443002 Cr-Commit-Position: refs/heads/master@{#463713} Committed: https://chromium.googlesource.com/chromium/src/+/d914e02c97cb940507d6662b1b9a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d914e02c97cb940507d6662b1b9a... |