Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23674004:
[rAC, OSX] Add overlay shield for interstitials/waits. (Closed)
|
Created:
7 years, 3 months ago by groby-ooo-7-16 Modified:
7 years, 3 months ago Reviewers:
sail CC:
chromium-reviews, benquan, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[rAC, OSX] Add overlay shield for interstitials/waits.
requestAutocomplete displays a splash screen as well as an
interstitial while communicating with wallet using the overlay shield.
The current dialog is hidden behind the shield for the duration, and
the shield can contain images as well as text messages.
R=sail@chromium.org
BUG=157274, 260951
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221399
Patch Set 1 #
Total comments: 12
Patch Set 2 : Allow animated overlays. #
Total comments: 46
Patch Set 3 : Review fixes & missing test file. #
Total comments: 4
Patch Set 4 : Review fixes, next round. #
Total comments: 3
Patch Set 5 : Final tweaks. #
Messages
Total messages: 12 (0 generated)
Sailesh: PTAL. Last remaining big chunk for rAC. As splash screen: http://imgur.com/xU0kkH6,v0HoZAb#0 With a message below: http://imgur.com/xU0kkH6,v0HoZAb#1
https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h (right): https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:15: struct DialogOverlayState; Don't need to indent or add a "// namespace ..." comment. Same below. https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:27: @protocol ChromiumAnimationDelegate How about just AnimationDelegate or AnimationDelegateProtocol? https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:36: base::scoped_nsobject<NSBox> view_; // The main view. Instead of keeping a separate reference to this, how about just adding a method that just does a ObjCCastStrict<NSBox>([self view]). https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:53: - (int)getHeightForWidth:(int)width; how about heightForWidth:? https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:159: - (id)init { rsesek pointed out that NSViewController has an designated initializer (initWithNibName:bundle). This should be replaced with that initializer, same with the [super init] call below. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:20: const int kOverlayTextInterlineSpacing = 10; CGFloat?, same below https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:113: CGFloat arrowHalfWidth = kArrowWidth / 2.0; I know this was already there but could we put kArrowWidth (and the other constant in autofill_dialog_constants.h) inside a namespace? Maybe "autofill" namespace?
https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h (right): https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:15: struct DialogOverlayState; On 2013/09/04 20:04:06, sail wrote: > Don't need to indent or add a "// namespace ..." comment. > Same below. Done. https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:27: @protocol ChromiumAnimationDelegate I'm fine with that - I just hope it doesn't clash at some point :) On 2013/09/04 20:04:06, sail wrote: > How about just AnimationDelegate or AnimationDelegateProtocol? https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:36: base::scoped_nsobject<NSBox> view_; // The main view. On 2013/09/04 20:04:06, sail wrote: > Instead of keeping a separate reference to this, how about just adding a method > that just does a ObjCCastStrict<NSBox>([self view]). Done. https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:53: - (int)getHeightForWidth:(int)width; On 2013/09/04 20:04:06, sail wrote: > how about heightForWidth:? Done. https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:159: - (id)init { So, all invocation sites use initWithNibName:nil bundle:nil even though these are the only valid values? That seems confusing, especially given that the second patchset adds another parameter to the init. Would you be fine if just [super init] got replaced with [super initWithNibName:nil bundle:nil]? On 2013/09/04 20:04:06, sail wrote: > rsesek pointed out that NSViewController has an designated initializer > (initWithNibName:bundle). > This should be replaced with that initializer, same with the [super init] call > below. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:20: const int kOverlayTextInterlineSpacing = 10; Hm. Technically, they'll be shared with the views code at some point. I'll make them CGFloat for now but we might need to revert back... On 2013/09/04 20:04:06, sail wrote: > CGFloat?, same below https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:113: CGFloat arrowHalfWidth = kArrowWidth / 2.0; Will do in follow-up CL, since I'd otherwise have a bunch of unrelated changes in this CL. (But generally agreed) On 2013/09/04 20:04:06, sail wrote: > I know this was already there but could we put kArrowWidth (and the other > constant in autofill_dialog_constants.h) inside a namespace? Maybe "autofill" > namespace?
https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:159: - (id)init { > Would you be fine if just [super init] got replaced with [super > initWithNibName:nil bundle:nil]? Yea that makes sense. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:40: base::scoped_nsobject<NSView> childView_; // Contains all UI elements. maybe add a comment that this is used for the fadeout animation? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:63: - (int)getHeightForWidth:(int)width; Use CGFloat instead? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:41: AnimationDelegateBridge(id<ChromiumAnimationDelegate>); argument name? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:50: }; DISALLOW_... https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:53: id<ChromiumAnimationDelegate> delegate) : delegate_(delegate) {} Instead of using a generic delegate protocol you could just use AutofillOverlayController directly. This code is all private to this file so I don't think generalizing things is helpful. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:73: base::Timer refresh_timer_; // Controls when it's time to refresh overlay. Use OneShotTimer instead? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:77: }; DISALLOW_... https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:119: // stroke is hidden on the sides. "sides" -> "sides and bottom" ? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:120: [arrow moveToPoint:NSMakePoint(NSMinX(bounds) - 1.0, y)]; Instead of adding and subtracting 1, how about doing this: arrowBounds = NSInsetRect(bounds, -1); arrowBounds.size.height++; arrowBounds.origin.y--; https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:121: [arrow relativeLineToPoint:NSMakePoint(NSMidX(bounds) - arrowHalfWidth, 0)]; NSMidX(bounds) is absolute. To make it relative you need to do NSWidth(bounds) / 2.0; Or use lineToPoint:? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:134: [super drawRect:dirtyRect]; don't need https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:162: DCHECK(messages[i].alignment == gfx::ALIGN_CENTER); DCHECK_EQ? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:176: CGFloat labelHeight = NSHeight([label frame]); DCHECK([label isKindOfClass:[NSTextField class])? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:220: const autofill::DialogOverlayState& state = delegate_->GetDialogOverlay(); move this below the if statement? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:239: id delegate = [[[self view] window] windowController]; You can use the explicit type here since you're doing performSelector. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:240: if ([delegate respondsToSelector:@selector(requestRelayout)]) I know that this pattern already existed in this codebase but normally we do this using protocols. Something like: if ([[windowController class] conformsToProtocol:@protocol(RequestRelayoutProtocol)]) { ... } https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:281: return 0; remove? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:308: - (void)animationProgressed:(const ui::Animation*) animation { no space before animation https://codereview.chromium.org/23674004/diff/12001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/23674004/diff/12001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:424: autofill::DialogOverlayState state; Is this used anywhere? https://codereview.chromium.org/23674004/diff/12001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:435: [contentView setPostsFrameChangedNotifications:YES]; Don't need this since you're observing the window?
https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/1/chrome/browser/ui/cocoa/autof... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:159: - (id)init { On 2013/09/04 21:18:51, sail wrote: > > > Would you be fine if just [super init] got replaced with [super > > initWithNibName:nil bundle:nil]? > > Yea that makes sense. Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:40: base::scoped_nsobject<NSView> childView_; // Contains all UI elements. On 2013/09/04 21:18:51, sail wrote: > maybe add a comment that this is used for the fadeout animation? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:63: - (int)getHeightForWidth:(int)width; Reluctant, because '0' is a special case - and equality on floats makes me feel weird :) On 2013/09/04 21:18:51, sail wrote: > Use CGFloat instead? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:41: AnimationDelegateBridge(id<ChromiumAnimationDelegate>); On 2013/09/04 21:18:51, sail wrote: > argument name? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:50: }; On 2013/09/04 21:18:51, sail wrote: > DISALLOW_... Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:53: id<ChromiumAnimationDelegate> delegate) : delegate_(delegate) {} There are more animations coming - this will be hoisted out soon-ish. On 2013/09/04 21:18:51, sail wrote: > Instead of using a generic delegate protocol you could just use > AutofillOverlayController directly. This code is all private to this file so I > don't think generalizing things is helpful. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:73: base::Timer refresh_timer_; // Controls when it's time to refresh overlay. On 2013/09/04 21:18:51, sail wrote: > Use OneShotTimer instead? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:77: }; On 2013/09/04 21:18:51, sail wrote: > DISALLOW_... Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:119: // stroke is hidden on the sides. On 2013/09/04 21:18:51, sail wrote: > "sides" -> "sides and bottom" ? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:120: [arrow moveToPoint:NSMakePoint(NSMinX(bounds) - 1.0, y)]; On 2013/09/04 21:18:51, sail wrote: > Instead of adding and subtracting 1, how about doing this: > arrowBounds = NSInsetRect(bounds, -1); > arrowBounds.size.height++; > arrowBounds.origin.y--; Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:121: [arrow relativeLineToPoint:NSMakePoint(NSMidX(bounds) - arrowHalfWidth, 0)]; On 2013/09/04 21:18:51, sail wrote: > NSMidX(bounds) is absolute. To make it relative you need to do NSWidth(bounds) / > 2.0; > Or use lineToPoint:? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:134: [super drawRect:dirtyRect]; On 2013/09/04 21:18:51, sail wrote: > don't need Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:162: DCHECK(messages[i].alignment == gfx::ALIGN_CENTER); On 2013/09/04 21:18:51, sail wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:176: CGFloat labelHeight = NSHeight([label frame]); I suppose. Do we usually do that when iterating over NSArrays? On 2013/09/04 21:18:51, sail wrote: > DCHECK([label isKindOfClass:[NSTextField class])? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:220: const autofill::DialogOverlayState& state = delegate_->GetDialogOverlay(); On 2013/09/04 21:18:51, sail wrote: > move this below the if statement? Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:239: id delegate = [[[self view] window] windowController]; On 2013/09/04 21:18:51, sail wrote: > You can use the explicit type here since you're doing performSelector. Done. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:240: if ([delegate respondsToSelector:@selector(requestRelayout)]) Hm. I've been planning to actually have an AutofillWindowController - there are other things I'd like to expose without more performSelector footwork. Let's chat about this off-line? On 2013/09/04 21:18:51, sail wrote: > I know that this pattern already existed in this codebase but normally we do > this using protocols. Something like: > if ([[windowController class] > conformsToProtocol:@protocol(RequestRelayoutProtocol)]) { > ... > } https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:281: return 0; Indeed. Surprised clang didn't yell at me. Thanks for catching! On 2013/09/04 21:18:51, sail wrote: > remove? https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:308: - (void)animationProgressed:(const ui::Animation*) animation { On 2013/09/04 21:18:51, sail wrote: > no space before animation Done. https://codereview.chromium.org/23674004/diff/12001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/23674004/diff/12001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:424: autofill::DialogOverlayState state; #@(*#@. I need an explicit setState for this. Got trigger-happy refactoring. Done. On 2013/09/04 21:18:52, sail wrote: > Is this used anywhere? https://codereview.chromium.org/23674004/diff/12001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:435: [contentView setPostsFrameChangedNotifications:YES]; Seems that way. Done. On 2013/09/04 21:18:52, sail wrote: > Don't need this since you're observing the window?
lgtm! https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:63: - (int)getHeightForWidth:(int)width; On 2013/09/04 22:19:01, groby wrote: > Reluctant, because '0' is a special case - and equality on floats makes me feel > weird :) > > On 2013/09/04 21:18:51, sail wrote: > > Use CGFloat instead? > I think it's still a good idea to make this a CGFloat. We check against 0 all the time. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:53: id<ChromiumAnimationDelegate> delegate) : delegate_(delegate) {} On 2013/09/04 22:19:01, groby wrote: > There are more animations coming - this will be hoisted out soon-ish. > > On 2013/09/04 21:18:51, sail wrote: > > Instead of using a generic delegate protocol you could just use > > AutofillOverlayController directly. This code is all private to this file so I > > don't think generalizing things is helpful. > ok https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:176: CGFloat labelHeight = NSHeight([label frame]); On 2013/09/04 22:19:01, groby wrote: > I suppose. Do we usually do that when iterating over NSArrays? > On 2013/09/04 21:18:51, sail wrote: > > DCHECK([label isKindOfClass:[NSTextField class])? > Not usually. I'm just worried about the subviews being the type you assume they are. Anyone could add a subview to this view. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:240: if ([delegate respondsToSelector:@selector(requestRelayout)]) On 2013/09/04 22:19:01, groby wrote: > Hm. I've been planning to actually have an AutofillWindowController - there are > other things I'd like to expose without more performSelector footwork. > > Let's chat about this off-line? > > On 2013/09/04 21:18:51, sail wrote: > > I know that this pattern already existed in this codebase but normally we do > > this using protocols. Something like: > > if ([[windowController class] > > conformsToProtocol:@protocol(RequestRelayoutProtocol)]) { > > ... > > } > Ok, makes sense. https://codereview.chromium.org/23674004/diff/19001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/23674004/diff/19001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:438: object:[self window]]; Just to be extra safe, can we add a DCHECK([self window]) here?
https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.h:63: - (int)getHeightForWidth:(int)width; Done, then. On 2013/09/04 23:08:25, sail wrote: > On 2013/09/04 22:19:01, groby wrote: > > Reluctant, because '0' is a special case - and equality on floats makes me > feel > > weird :) > > > > On 2013/09/04 21:18:51, sail wrote: > > > Use CGFloat instead? > > > > I think it's still a good idea to make this a CGFloat. We check against 0 all > the time. https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... File chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm (right): https://codereview.chromium.org/23674004/diff/3001/chrome/browser/ui/cocoa/au... chrome/browser/ui/cocoa/autofill/autofill_overlay_controller.mm:176: CGFloat labelHeight = NSHeight([label frame]); On 2013/09/04 23:08:25, sail wrote: > On 2013/09/04 22:19:01, groby wrote: > > I suppose. Do we usually do that when iterating over NSArrays? > > On 2013/09/04 21:18:51, sail wrote: > > > DCHECK([label isKindOfClass:[NSTextField class])? > > > > Not usually. I'm just worried about the subviews being the type you assume they > are. Anyone could add a subview to this view. Done. https://codereview.chromium.org/23674004/diff/19001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/23674004/diff/19001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:438: object:[self window]]; Done - but it's really belt, suspender & shoelaces :) This is an NSWindowController, and the initializer creates window to be used with this, _and_ the initializer is called right before show. Curious: Why are you worried about the window being nil? Is it _that_ expensive to get move notifications for all windows? On 2013/09/04 23:08:25, sail wrote: > Just to be extra safe, can we add a DCHECK([self window]) here?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/23674004/23001
https://codereview.chromium.org/23674004/diff/19001/chrome/browser/ui/cocoa/a... File chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm (right): https://codereview.chromium.org/23674004/diff/19001/chrome/browser/ui/cocoa/a... chrome/browser/ui/cocoa/autofill/autofill_dialog_cocoa.mm:438: object:[self window]]; On 2013/09/05 00:44:20, groby wrote: > Done - but it's really belt, suspender & shoelaces :) This is an > NSWindowController, and the initializer creates window to be used with this, > _and_ the initializer is called right before show. > > Curious: Why are you worried about the window being nil? Is it _that_ expensive > to get move notifications for all windows? > > On 2013/09/04 23:08:25, sail wrote: > > Just to be extra safe, can we add a DCHECK([self window]) here? > Oh, you're right. Yea you don't need the DCHECK(). In so many cases we do something like -[view window] and end up listening to notifications for all windows.
Retried try job too often on ios_dbg_simulator for step(s) ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/23674004/23001
Message was sent while issue was closed.
Change committed as 221399 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
