Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10802090:
fixed the distorted behavior on the mediastreaminfobar on Mac (Closed)
|
Created:
8 years, 5 months ago by no longer working on chromium Modified:
8 years, 4 months ago Reviewers:
Scott Hess - ex-Googler CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis patch makes the "options" drop box a proper size, so that it does not need to be as wide as "Always allow this site to use this camera and this microphone and camera".
It guarantees no overlap when the window is resizing.
It also corrects the layout by putting the cancelButton on the left of the okButton.
BUG=137837, 138447
TEST= try with site: https://apprtc.appspot.com
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148960
Patch Set 1 #Patch Set 2 : ready for review. #
Total comments: 27
Patch Set 3 : addressed comments from shess #
Total comments: 9
Patch Set 4 : addressed the comments and don't rebuild the menu in arrangeInfobarLayout #
Total comments: 7
Patch Set 5 : rebased and addressed the comments #
Messages
Total messages: 9 (0 generated)
Shess, could you please review this CL? Thanks, BR; SX
https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.h (right): https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.h:30: // Space between controls in pixels - read from the NIB. maybe replace "read" with "generated" or "captured". Initially I was like "Wait, you can set float values in nibs?" https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:26: static const CGFloat kSpaceBetweenControls = 8; Should use of this also be replaced by spaceBetweenControls_? https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:38: void SizeAndPlaceControl(NSView* toModify, NSView* anchor, bool after) { AFAICT, this is the only user of kSpaceBetweenControls, and this is no longer called by anyone. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:130: NSMinX(cancelButtonFrame) - NSMaxX(okButtonFrame); There are cases in our UI where the effective boundaries of buttons extends somewhat beyond the visual boundaries, resulting in a slight overlap. This would be more robust if the condition is NSMinX() < NSMinX(). https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:136: [deviceMenu_ setAutoresizingMask:NSViewMinXMargin]; Can this line move to the initialization section in -constructView? It looks like it was originally in the -init*, after -setBezelStyle. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:141: relativeTo:nil]; Logically, this feels like it belongs somewhere else, like maybe -constructView (since it's always in the hierarchy), or -arrangeInfobarLayout (if it were to optionally be in the hierarchy). https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:142: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; This seems like it might be contrary to your -sizeToFit calls when arranging the layout. Maybe it wouldn't work right in some locales? In the previous code SizeAndPlaceControl() was calling this when placing things. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:187: // From left to right: label_, okButton_, cancelButton_ and deviceMenu_. The comment implies that it's label, cancel, ok. From the code below, it looks like deviceMenu_ actually is maybe left of closeButton_, with it slightly different than right of okButton_, so maybe don't include it in this list at all. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:198: // Hide the deviceMenu_ in case there is overlap. I think the comment could be clearer, it's only hidden if the smaller version cannot fit. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:200: [self rebuildDeviceMenu:YES]; I think that this would make more sense if you had -rebuildDeviceMenu as it used to be, and then call [[[deviceMenu_ menu] itemAtIndex:0] setTitle:@""] here. Or wrap it in a -hideDeviceMenuTitle method if you like. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:203: [deviceMenu_ setFrame:oldFrame]; Does -sizeToFit work here? Or does that allow things to get _too_ small? If things get too small in that case, then I suggest capturing the deviceMenu_ frame _before_ title-hiding, to make sure you don't get a re-calculated size. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:238: NSArray* allControls = [self allControls]; Is -allControls used anywhere else. As things currently stand, I think this loop is worse than unrolling things for the two affected buttons. [Unless you have a lot of controls, like 8, I would greatly prefer just replicating the code. It's a little like table-driven tests, it's easier to see what the actual items are being set to when they're just listed out.] https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:245: [[button cell] setArrowPosition:NSPopUpArrowAtBottom]; My guess is that at one point you had deviceMenu_ in -allControls, and then had to back it out?
please take another look. Thanks. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.h (right): https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.h:30: // Space between controls in pixels - read from the NIB. On 2012/07/25 18:28:15, shess wrote: > maybe replace "read" with "generated" or "captured". Initially I was like > "Wait, you can set float values in nibs?" Done. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:26: static const CGFloat kSpaceBetweenControls = 8; On 2012/07/25 18:28:15, shess wrote: > Should use of this also be replaced by spaceBetweenControls_? removed https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:38: void SizeAndPlaceControl(NSView* toModify, NSView* anchor, bool after) { On 2012/07/25 18:28:15, shess wrote: > AFAICT, this is the only user of kSpaceBetweenControls, and this is no longer > called by anyone. Good catch. They are removed now. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:130: NSMinX(cancelButtonFrame) - NSMaxX(okButtonFrame); On 2012/07/25 18:28:15, shess wrote: > There are cases in our UI where the effective boundaries of buttons extends > somewhat beyond the visual boundaries, resulting in a slight overlap. This > would be more robust if the condition is NSMinX() < NSMinX(). The condition is to guarantee that the spaceBetweenControls_ is a positive value, in case switching to NSMinX() < NSMinX(), is there any chance of getting a negative value when the overlap happens? https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:136: [deviceMenu_ setAutoresizingMask:NSViewMinXMargin]; On 2012/07/25 18:28:15, shess wrote: > Can this line move to the initialization section in -constructView? It looks > like it was originally in the -init*, after -setBezelStyle. Done. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:141: relativeTo:nil]; On 2012/07/25 18:28:15, shess wrote: > Logically, this feels like it belongs somewhere else, like maybe -constructView > (since it's always in the hierarchy), or -arrangeInfobarLayout (if it were to > optionally be in the hierarchy). Done. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:142: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; On 2012/07/25 18:28:15, shess wrote: > This seems like it might be contrary to your -sizeToFit calls when arranging the > layout. Maybe it wouldn't work right in some locales? In the previous code > SizeAndPlaceControl() was calling this when placing things. removed this -sizeToFit function here. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:187: // From left to right: label_, okButton_, cancelButton_ and deviceMenu_. On 2012/07/25 18:28:15, shess wrote: > The comment implies that it's label, cancel, ok. From the code below, it looks > like deviceMenu_ actually is maybe left of closeButton_, with it slightly > different than right of okButton_, so maybe don't include it in this list at > all. thanks. In linux and windows, the cancel button is right of the ok button, I messed them up in the comments. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:198: // Hide the deviceMenu_ in case there is overlap. On 2012/07/25 18:28:15, shess wrote: > I think the comment could be clearer, it's only hidden if the smaller version > cannot fit. Done. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:200: [self rebuildDeviceMenu:YES]; On 2012/07/25 18:28:15, shess wrote: > I think that this would make more sense if you had -rebuildDeviceMenu as it used > to be, and then call [[[deviceMenu_ menu] itemAtIndex:0] setTitle:@""] here. Or > wrap it in a -hideDeviceMenuTitle method if you like. Good idea, then we don't need to rebuild the menu for each resize. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:203: [deviceMenu_ setFrame:oldFrame]; On 2012/07/25 18:28:15, shess wrote: > Does -sizeToFit work here? Or does that allow things to get _too_ small? > > If things get too small in that case, then I suggest capturing the deviceMenu_ > frame _before_ title-hiding, to make sure you don't get a re-calculated size. Thanks for pointing it out. -sizeToFit works the same here. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:238: NSArray* allControls = [self allControls]; On 2012/07/25 18:28:15, shess wrote: > Is -allControls used anywhere else. As things currently stand, I think this > loop is worse than unrolling things for the two affected buttons. > > [Unless you have a lot of controls, like 8, I would greatly prefer just > replicating the code. It's a little like table-driven tests, it's easier to see > what the actual items are being set to when they're just listed out.] Done. https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:245: [[button cell] setArrowPosition:NSPopUpArrowAtBottom]; On 2012/07/25 18:28:15, shess wrote: > My guess is that at one point you had deviceMenu_ in -allControls, and then had > to back it out? sorry for the uncleaned debug code, I had moved the call out to -arrangeInfobarLayout
https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/2001/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:130: NSMinX(cancelButtonFrame) - NSMaxX(okButtonFrame); On 2012/07/26 10:17:47, xians1 wrote: > On 2012/07/25 18:28:15, shess wrote: > > There are cases in our UI where the effective boundaries of buttons extends > > somewhat beyond the visual boundaries, resulting in a slight overlap. This > > would be more robust if the condition is NSMinX() < NSMinX(). > > The condition is to guarantee that the spaceBetweenControls_ is a positive > value, in case switching to NSMinX() < NSMinX(), is there any chance of getting > a negative value when the overlap happens? Hmm. I read the condition as making sure that it calculated the right difference regardless of the ordering of the buttons. As I mentioned, there have been cases where overlap between buttons was indicated. If the button overlap is intentional, then you should be using a negative number. If the button overlap is unintentional, then taking the absolute value will not make it correct. If you want to enforce sanity, just take the difference and use a DCHECK_GT(space, 0), don't arbitrarily invert the nib file's spacing. https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (left): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:235: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; It looks to me like the GTM version of sizeToFit has substantial changes WRT buttons. I suspect that you probably want to use that version of sizeToFit instead of the button's default -sizeToFit. Maybe test both ways and see what the visual impact is, like does the button shift position, or does the text w/in the button shift position, relative to the baseline of the other overview text. https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:190: [[label_ superview] replaceSubview:label_ with:label1_.get()]; Does this really need the get() on label1_? It shouldn't need it if label_ doesn't need it. https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:207: [deviceMenu_ setAutoresizingMask:NSViewMinXMargin]; Move this above the NSMenu line so the deviceMenu_ setup is together.
https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (left): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:235: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; On 2012/07/26 19:39:41, shess wrote: > It looks to me like the GTM version of sizeToFit has substantial changes WRT > buttons. I suspect that you probably want to use that version of sizeToFit > instead of the button's default -sizeToFit. Maybe test both ways and see what > the visual impact is, like does the button shift position, or does the text w/in > the button shift position, relative to the baseline of the other overview text. I guess you what meant is deviceMenu_ sizeToFit]; in line 163. It works the same as using GTM, so I changed it to GTM. https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:190: [[label_ superview] replaceSubview:label_ with:label1_.get()]; On 2012/07/26 19:39:41, shess wrote: > Does this really need the get() on label1_? It shouldn't need it if label_ > doesn't need it. Old code, but strangely all the replaceSubview API is called with .get() in chrome. But I checked the scoped_nsobject source code, it should be all right to skip the .get() https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:207: [deviceMenu_ setAutoresizingMask:NSViewMinXMargin]; On 2012/07/26 19:39:41, shess wrote: > Move this above the NSMenu line so the deviceMenu_ setup is together. Done. https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:107: DCHECK(NSMaxX(cancelButtonFrame) < NSMinX(okButtonFrame)); We don't allow overlap between the buttons. So it is definitely a good idea to add a DCHECK. Do you know if it is all right to add a DCHECK like this here? I am not sure how the OS knows cancel button is on the left of the ok button, I suppose arrangeInfobarLayout has not been called yet.
LGTM, with the -setBezelStyle: and -setAutoresizingMask: change indicated, your option on moving the one -sizeToFit into the title-setting method. https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (left): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:235: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; On 2012/07/27 10:09:55, xians1 wrote: > On 2012/07/26 19:39:41, shess wrote: > > It looks to me like the GTM version of sizeToFit has substantial changes WRT > > buttons. I suspect that you probably want to use that version of sizeToFit > > instead of the button's default -sizeToFit. Maybe test both ways and see what > > the visual impact is, like does the button shift position, or does the text > w/in > > the button shift position, relative to the baseline of the other overview > text. > > I guess you what meant is deviceMenu_ sizeToFit]; in line 163. > It works the same as using GTM, so I changed it to GTM. Basically, I think you probably need to use the GTM version for all -[NSButton sizeToFit] calls, and never mix them for the same control unless you're really careful. I don't fully understand what GTM is doing, here, my best guess is that it's trying to maintain some Interface Builder sizing quirks so that manual changes don't cause weird sizing changes. The only case I'm really concerned about is with the @"" title, where it might fit to a smaller image-only outline. If that happens, you might want to grab the height of okButton_ and make it square. But if it works as-is, leave it be (it might also fit to the font for height and the text+image for width). https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:190: [[label_ superview] replaceSubview:label_ with:label1_.get()]; On 2012/07/27 10:09:55, xians1 wrote: > On 2012/07/26 19:39:41, shess wrote: > > Does this really need the get() on label1_? It shouldn't need it if label_ > > doesn't need it. > > Old code, but strangely all the replaceSubview API is called with .get() in > chrome. > But I checked the scoped_nsobject source code, it should be all right to skip > the .get() I think the rule of thumb is to use the non-get() version unless it doesn't compile. Objective-C is dynamic, so having the right typing info only changes compile-time warnings. But if the method's receiver is ambiguously typed, that can make the method typing ambiguous because it doesn't know which is the real receiver, and then it doesn't know how to derive the type correctly. https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:107: DCHECK(NSMaxX(cancelButtonFrame) < NSMinX(okButtonFrame)); On 2012/07/27 10:09:56, xians1 wrote: > We don't allow overlap between the buttons. So it is definitely a good idea to > add a DCHECK. > Do you know if it is all right to add a DCHECK like this here? I am not sure how > the OS knows cancel button is on the left of the ok button, I suppose > arrangeInfobarLayout has not been called yet. The nib file controls this, the OS doesn't care. So the DCHECK makes sure that someone doesn't edit the nib file to put the controls in the other order, or overlap them. I don't think it should ever just re-arrange things willy-nilly, so I think this is quite sufficient. https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:170: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; Maybe move this line (and the occurance above) right into -showDeviceMenuTitle:? https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:202: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; I would put the setBezelStyle: and setAutoresizingMask: lines above the -rebuildDeviceMenu line, and definitely above the -sizeToFitView: line. I would consider them in the might-or-might-not-modify sizing class of methods.
Many thanks to Shess. I am going to land it soon. https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (left): https://chromiumcodereview.appspot.com/10802090/diff/2002/chrome/browser/ui/c... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:235: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; On 2012/07/27 16:35:20, shess wrote: > On 2012/07/27 10:09:55, xians1 wrote: > > On 2012/07/26 19:39:41, shess wrote: > > > It looks to me like the GTM version of sizeToFit has substantial changes WRT > > > buttons. I suspect that you probably want to use that version of sizeToFit > > > instead of the button's default -sizeToFit. Maybe test both ways and see > what > > > the visual impact is, like does the button shift position, or does the text > > w/in > > > the button shift position, relative to the baseline of the other overview > > text. > > > > I guess you what meant is deviceMenu_ sizeToFit]; in line 163. > > It works the same as using GTM, so I changed it to GTM. > > Basically, I think you probably need to use the GTM version for all -[NSButton > sizeToFit] calls, and never mix them for the same control unless you're really > careful. I don't fully understand what GTM is doing, here, my best guess is > that it's trying to maintain some Interface Builder sizing quirks so that manual > changes don't cause weird sizing changes. > > The only case I'm really concerned about is with the @"" title, where it might > fit to a smaller image-only outline. If that happens, you might want to grab > the height of okButton_ and make it square. But if it works as-is, leave it be > (it might also fit to the font for height and the text+image for width). Thanks for pointing out. I just found out that it was not a square button in @"" title case. So we we need to make it square explicitly. Meanwhile, we need to skid calling GTM sizeToFit function in this case, otherwise the square size won't apply. Hope it is fine. https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... File chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm (right): https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:107: DCHECK(NSMaxX(cancelButtonFrame) < NSMinX(okButtonFrame)); On 2012/07/27 16:35:20, shess wrote: > On 2012/07/27 10:09:56, xians1 wrote: > > We don't allow overlap between the buttons. So it is definitely a good idea to > > add a DCHECK. > > Do you know if it is all right to add a DCHECK like this here? I am not sure > how > > the OS knows cancel button is on the left of the ok button, I suppose > > arrangeInfobarLayout has not been called yet. > > The nib file controls this, the OS doesn't care. So the DCHECK makes sure that > someone doesn't edit the nib file to put the controls in the other order, or > overlap them. I don't think it should ever just re-arrange things willy-nilly, > so I think this is quite sufficient. Done. https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:170: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; On 2012/07/27 16:35:20, shess wrote: > Maybe move this line (and the occurance above) right into -showDeviceMenuTitle:? Done. https://chromiumcodereview.appspot.com/10802090/diff/11001/chrome/browser/ui/... chrome/browser/ui/cocoa/infobars/media_stream_infobar_controller.mm:202: [GTMUILocalizerAndLayoutTweaker sizeToFitView:deviceMenu_]; On 2012/07/27 16:35:20, shess wrote: > I would put the setBezelStyle: and setAutoresizingMask: lines above the > -rebuildDeviceMenu line, and definitely above the -sizeToFitView: line. I would > consider them in the might-or-might-not-modify sizing class of methods. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10802090/8002
Change committed as 148960 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
