Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838373002:
Adding Sign-in promo for bookmark view. (Closed)
|
Created:
3 years, 8 months ago by jlebel Modified:
3 years, 7 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, tfarina, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, srahim+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding Sign-in promo for bookmark view.
The dismiss button is not implemented yet, see crrev.com/2844253003.
Before:
https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k
https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8
After with cold state:
https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM
https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs
After with warm state:
https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U
https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM
BUG=661794
Review-Url: https://codereview.chromium.org/2838373002
Cr-Commit-Position: refs/heads/master@{#468946}
Committed: https://chromium.googlesource.com/chromium/src/+/4d2d106422e18753c3fa4e52e089b465c6437bf2
Patch Set 1 #
Total comments: 10
Patch Set 2 : Remove consumer and comments #
Total comments: 17
Patch Set 3 : . #
Total comments: 6
Patch Set 4 : . #
Total comments: 6
Patch Set 5 : . #Patch Set 6 : From CGFLOAT_MAX to 1000 #
Dependent Patchsets: Messages
Total messages: 37 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Adding Sign-in promo for bookmark view. The cross to remove the promo is not implemented yet, and the rest of the UI has not be updated yet to match perfectly the preview. BUG=661794 ========== to ========== Adding Sign-in promo for bookmark view. The cross to remove the promo is not implemented yet, and the rest of the UI has not be updated yet to match perfectly the preview. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 ==========
Patchset #1 (id:60001) has been deleted
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Mihai, Can you review this patch? Thanks,
jlebel@chromium.org changed reviewers: + lpromero@chromium.org
Hello Louis, Can you review this patch when you will have time? Thanks,
Description was changed from ========== Adding Sign-in promo for bookmark view. The cross to remove the promo is not implemented yet, and the rest of the UI has not be updated yet to match perfectly the preview. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 ========== to ========== Adding Sign-in promo for bookmark view. The dismiss button is not implemented yet and will come a following path soon. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 ==========
https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm:692: DCHECK(![self isPromoSection:indexPath.section]); Why is it the case? https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:362: UICollectionViewCell* cell = Is it possible to encapsulate this logic somewhere else than in this already big controller? https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:503: _signinPromoViewMediator = nil; You might want to unset as consumer too. https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:12: @interface BookmarkSigninPromoCell : UICollectionViewCell Comments everywhere.
Patchset #2 (id:100001) has been deleted
Thanks for your late review. https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_collection_view.mm:692: DCHECK(![self isPromoSection:indexPath.section]); On 2017/04/26 19:03:50, lpromero wrote: > Why is it the case? I moved it in the subclass so we do the cell size in one place only. For the new promo cell, I definitely need to move it in the subclass. But if I don't move the previous promo cell, then the code to compute the size would be duplicated. So I chose to move both cell, therefore I added the DCHECK to be clear. https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:362: UICollectionViewCell* cell = On 2017/04/26 19:03:50, lpromero wrote: > Is it possible to encapsulate this logic somewhere else than in this already big > controller? I could copy this code into BookmarkSigninPromoCell and BookmarkPromoCell. This would make the code implemented twice. What do you think? https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:503: _signinPromoViewMediator = nil; On 2017/04/26 19:03:50, lpromero wrote: > You might want to unset as consumer too. Ok, but I'm the owner of this class. https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:12: @interface BookmarkSigninPromoCell : UICollectionViewCell On 2017/04/26 19:03:51, lpromero wrote: > Comments everywhere. Done.
https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:364: CGRect estimatedFrame = I think this is not the estimated frame, but rather a frame that has fixed size 0, fixed width and very large height. How about we rename it to cellFrameToComputeHeightThat or fixedWidthMaxHeightFrame. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:366: CGPoint originalPosition = CGPointZero; Optional nit: s/originalPosition/initialCellOrigin https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:375: if (experimental_flags::IsSigninPromoEnabled()) { I think this code would be easier to read if we have a function in anonymous namespace that is called, that has a similar implementation to the one in https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/collection_view/ce...): (CGFloat)PreferredHeightForWidth(UICollectionViewCell cell, width) https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:386: cell.frame = estimatedFrame; Nit: Move this inside the if(cell) as on the else case the cell has the size set to estimatedFrame https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:12: // Sign-in promo cell based on SigninPromoView. This cell invite the user to s/invite/invites https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:13: // login without typing their password (based on "CONTINUE AS"). Remove "(based on "CONTINUE AS")" (I do not understand what that means). https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:19: // SigninPromoView view. This comment is useless (it is the same as the name of the property). Remove it. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:46: // +[MDCCollectionViewCell cr_preferredHeightForWidth:forItem:]. This comment is not correct - this is not a MDCCollectionViewCell and [MDCCollectionViewCell cr_preferredHeightForWidth:forItem:] is never called for this cell. However this implementation is required as the prefered height of this cell is computed using a similar algorithm.
https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:364: CGRect estimatedFrame = On 2017/04/27 11:58:24, msarda wrote: > I think this is not the estimated frame, but rather a frame that has fixed size > 0, fixed width and very large height. > How about we rename it to cellFrameToComputeHeightThat or > fixedWidthMaxHeightFrame. Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:366: CGPoint originalPosition = CGPointZero; On 2017/04/27 11:58:24, msarda wrote: > Optional nit: > s/originalPosition/initialCellOrigin Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:375: if (experimental_flags::IsSigninPromoEnabled()) { On 2017/04/27 11:58:24, msarda wrote: > I think this code would be easier to read if we have a function in anonymous > namespace that is called, that has a similar implementation to the one in > https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/collection_view/ce...): > (CGFloat)PreferredHeightForWidth(UICollectionViewCell cell, width) Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:386: cell.frame = estimatedFrame; On 2017/04/27 11:58:24, msarda wrote: > Nit: Move this inside the if(cell) as on the else case the cell has the size set > to estimatedFrame Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:12: // Sign-in promo cell based on SigninPromoView. This cell invite the user to On 2017/04/27 11:58:24, msarda wrote: > s/invite/invites Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:13: // login without typing their password (based on "CONTINUE AS"). On 2017/04/27 11:58:24, msarda wrote: > Remove "(based on "CONTINUE AS")" (I do not understand what that means). Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:19: // SigninPromoView view. On 2017/04/27 11:58:24, msarda wrote: > This comment is useless (it is the same as the name of the property). Remove it. Done. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:46: // +[MDCCollectionViewCell cr_preferredHeightForWidth:forItem:]. On 2017/04/27 11:58:24, msarda wrote: > This comment is not correct - this is not a MDCCollectionViewCell and > [MDCCollectionViewCell cr_preferredHeightForWidth:forItem:] is never called for > this cell. > > However this implementation is required as the prefered height of this cell is > computed using a similar algorithm. Done.
The CQ bit was checked by jlebel@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...
lgtm https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:362: UICollectionViewCell* cell = On 2017/04/27 09:54:54, jlebel wrote: > On 2017/04/26 19:03:50, lpromero wrote: > > Is it possible to encapsulate this logic somewhere else than in this already > big > > controller? > > I could copy this code into BookmarkSigninPromoCell and BookmarkPromoCell. This > would make the code implemented twice. What do you think? At some point, the old cell will be deprecated and removed, and I guess that day we won't move this code back to the new cell. Maybe add a TODO here to move this code to the new cell when the old is removed? https://codereview.chromium.org/2838373002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:503: _signinPromoViewMediator = nil; On 2017/04/27 09:54:54, jlebel wrote: > On 2017/04/26 19:03:50, lpromero wrote: > > You might want to unset as consumer too. > > Ok, but I'm the owner of this class. You can't know if you are the only owner though :) I am fine either way. https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h (right): https://codereview.chromium.org/2838373002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.h:19: // SigninPromoView view. On 2017/04/27 14:01:28, jlebel wrote: > On 2017/04/27 11:58:24, msarda wrote: > > This comment is useless (it is the same as the name of the property). Remove > it. > > Done. Maybe: The sign-in promo view embedded in this cell. https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:28: CGSize PreferredCellSizeForWidth(UICollectionViewCell* cell, CGFloat width) { Please add a comment. https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:550: static_cast<BookmarkSigninPromoCell*>(cell); Replace with: BookmarkSigninPromoCell* signinPromoCell = base::mac::ObjCCast<BookmarkSigninPromoCell*>( [self.collectionView cellForItemAtIndexPath:indexPath]); https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right): https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:53: // height. Don't you need two calls to layoutSubviews?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In -[BookmarkFolderCollectionView cellSizeForIndexPath:] I changed: [[BookmarkSigninPromoCell alloc] init]; by: [[BookmarkSigninPromoCell alloc] initWithFrame:CGRectMake(0, 0, 1000, 1000)]; To avoid contraint warnings. https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:28: CGSize PreferredCellSizeForWidth(UICollectionViewCell* cell, CGFloat width) { On 2017/04/27 15:49:12, lpromero wrote: > Please add a comment. Done. https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:550: static_cast<BookmarkSigninPromoCell*>(cell); On 2017/04/27 15:49:12, lpromero wrote: > Replace with: > > BookmarkSigninPromoCell* signinPromoCell = > base::mac::ObjCCast<BookmarkSigninPromoCell*>( > [self.collectionView cellForItemAtIndexPath:indexPath]); Done. https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right): https://codereview.chromium.org/2838373002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:53: // height. On 2017/04/27 15:49:12, lpromero wrote: > Don't you need two calls to layoutSubviews? No. I tried it with rotation too, it is not useful.
Description was changed from ========== Adding Sign-in promo for bookmark view. The dismiss button is not implemented yet and will come a following path soon. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 ========== to ========== Adding Sign-in promo for bookmark view. The dismiss button is not implemented yet, see crrev.com/2844253003. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 ==========
Thank you for figuring out this layout, Jerome. LGTM with a personal preference and a nit. https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:29: CGSize PreferredCellSizeForWidth(UICollectionViewCell* cell, CGFloat width) { Personal preference (I find that you need less local variables): How about? CGFrame cellFrame = cell.frame; cellFrame.size.width = width; cell.frame = cellFrame; [cell setNeedsLayout]; [cell layoutIfNeeded]; CGSize result = ... cellFrame.size = result; cell.frame = cellFrame; return result; https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:393: initWithFrame:CGRectMake(0, 0, 1000, 1000)]; I would prefer CGFLOAT_MAX, CGFLOAT_MAX instead of 1000 (1000 seems very arbitrary to me).
https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:29: CGSize PreferredCellSizeForWidth(UICollectionViewCell* cell, CGFloat width) { On 2017/04/28 11:53:14, msarda wrote: > Personal preference (I find that you need less local variables): How about? > CGFrame cellFrame = cell.frame; > cellFrame.size.width = width; > cell.frame = cellFrame; > [cell setNeedsLayout]; > [cell layoutIfNeeded]; > CGSize result = ... > cellFrame.size = result; > cell.frame = cellFrame; > return result; Done. https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:393: initWithFrame:CGRectMake(0, 0, 1000, 1000)]; On 2017/04/28 11:53:14, msarda wrote: > I would prefer CGFLOAT_MAX, CGFLOAT_MAX instead of 1000 (1000 seems very > arbitrary to me). It is totally arbitrary. But it seems more reasonable than CGFLOAT_MAX.
LGTM.
The CQ bit was checked by jlebel@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/2838373002/#ps180001 (title: ".")
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 jlebel@chromium.org
https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:393: initWithFrame:CGRectMake(0, 0, 1000, 1000)]; On 2017/04/28 13:03:17, jlebel wrote: > On 2017/04/28 11:53:14, msarda wrote: > > I would prefer CGFLOAT_MAX, CGFLOAT_MAX instead of 1000 (1000 seems very > > arbitrary to me). > > It is totally arbitrary. But it seems more reasonable than CGFLOAT_MAX. CGFLOAT_MAX*CGFLOAT_MAX would have a large (huge!) memory impact :p
https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right): https://codereview.chromium.org/2838373002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:393: initWithFrame:CGRectMake(0, 0, 1000, 1000)]; On 2017/05/02 15:25:20, lpromero wrote: > On 2017/04/28 13:03:17, jlebel wrote: > > On 2017/04/28 11:53:14, msarda wrote: > > > I would prefer CGFLOAT_MAX, CGFLOAT_MAX instead of 1000 (1000 seems very > > > arbitrary to me). > > > > It is totally arbitrary. But it seems more reasonable than CGFLOAT_MAX. > > CGFLOAT_MAX*CGFLOAT_MAX would have a large (huge!) memory impact :p Done.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2838373002/#ps200001 (title: "From CGFLOAT_MAX to 1000")
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": 200001, "attempt_start_ts": 1493809363834140,
"parent_rev": "95ea1df0596b4ee4ac745405b321e20b68f05d42", "commit_rev":
"4d2d106422e18753c3fa4e52e089b465c6437bf2"}
Message was sent while issue was closed.
Description was changed from ========== Adding Sign-in promo for bookmark view. The dismiss button is not implemented yet, see crrev.com/2844253003. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 ========== to ========== Adding Sign-in promo for bookmark view. The dismiss button is not implemented yet, see crrev.com/2844253003. Before: https://drive.google.com/open?id=0ByXziH_JVCGJNW9pV1RLSVBLc1k https://drive.google.com/open?id=0ByXziH_JVCGJaFUzNW0yMHhROW8 After with cold state: https://drive.google.com/open?id=0ByXziH_JVCGJR1U1QWdiNzlMSHM https://drive.google.com/open?id=0ByXziH_JVCGJSUp1VFNKRVBfSGs After with warm state: https://drive.google.com/open?id=0ByXziH_JVCGJWWxvd1hnNXJxS0U https://drive.google.com/open?id=0ByXziH_JVCGJX3R3el9RRGJFTXM BUG=661794 Review-Url: https://codereview.chromium.org/2838373002 Cr-Commit-Position: refs/heads/master@{#468946} Committed: https://chromium.googlesource.com/chromium/src/+/4d2d106422e18753c3fa4e52e089... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/chromium/src/+/4d2d106422e18753c3fa4e52e089... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
