Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1305143008:
[Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() (Closed)
Description[Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility()
This CL allows bookmark apps opened in windows on Mac to show and
hide the location bar in a manner consistent with other platforms. The
location bar is hidden when the window is at the same origin as the
bookmark app, and shown when the window is navigated away
from that origin. The resizing is implemented in a manner consistent
with the bookmarks bar.
Prior to this CL, app windows and popups did not display the full
toolbar view. Instead, they only displayed the location bar view (an
NSTextField subclass). This view-level substitution prevented the
app window toolbar from being resized in the same manner as the
bookmarks bar. This CL refactors the implementation so that the
view-level distinction is removed. App windows and popups which
only display the location bar now use the full toolbar view, with the
location bar resized to be full width and the undesired navigation
and wrench buttons hidden.
BUG=491792, 499741
Committed: https://crrev.com/5c98d330e26e8547932f7fdac8ef1b55e928c782
Cr-Commit-Position: refs/heads/master@{#356226}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing reviewer comments #Patch Set 3 : Fix comment #
Total comments: 16
Patch Set 4 : Implement a location bar only view for the toolbar #Patch Set 5 : Remove unnecessary super view calls #Patch Set 6 : Adding and fixing unit tests #
Total comments: 38
Patch Set 7 : Addressing reviewer feedback #
Total comments: 36
Patch Set 8 : Restoring popup windows to their original look #
Total comments: 12
Patch Set 9 : Addressing reviewer feedback #
Total comments: 8
Patch Set 10 : Addressing reviewer feedback #Patch Set 11 : More explicit size testing #
Total comments: 6
Patch Set 12 : Addressing nits #
Messages
Total messages: 32 (7 generated)
dominickn@chromium.org changed reviewers: + tapted@chromium.org
PTAL - I'm not sure if I've done this in a good/correct way or not. I've tried to avoid sending excess notifications and avoid re-layouting the browser window if possible. Thanks!
The CL description should be Something like `[Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility()` Then, there is already machinery for adding/removing views from the browser window (or collpasing/expanding them). i.e. it's already done for the bookmarks bar and download shelf, so we need to hook into the same machinery. BB and DS do it by implementing AnimatableView. ToolbarView doesn't currently do this, so I think that's the change you need. Then just have UpdateLocationBarVisibility() call either [GetToolbarView() setHeight:] or [GetToolbarView() animateToNewHeight] -- this should automatically call BrowserWindowController's `resizeView`, which will call layoutSubviews for you. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.h:186: BOOL shouldShowToolbar_; needs a comment. Importantly, it needs to explain why it's different to `- (BOOL)hasToolbar` Also perhaps invert the logic so that `NO` is the default case, then you don't need to initialize it (i.e. toolbarHidden_). But! I don't think you need this anyway. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller_private.h:79: - (void)showToolbar:(NSNotification*)notification; all objc methods in .h files need comments, per go/objcguide https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller_private.mm:918: if (shouldShowToolbar_) { I think you can read `shouldShowToolbar` by just asking the location bar whether it's hidden (then would just need to trigger a call to layoutSubviews). But all that should be irrelevant if we can just collapse the height of the ToolbarView using AnimatableView. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:278: bool show_location_bar_; Can you just infer this from [field_ isHidden]? https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:213: [[NSNotificationCenter defaultCenter] Something like [[BrowserWindowController browserWindowControllerForView:[self view]] setToolbarHidden:YES]; would probably be a lot nicer than the notifications. But I don't think this is the way to go about it anyway.
This is great, thanks for the detailed feedback! I wasn't originally sure if going the whole hog and wiring up Toolbar to the AnimatableView was the right way to go.
Thanks for the feedback! PTAL - now implemented using AnimatableView and in as consistent a manner to the bookmark bar as possible. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller.h:186: BOOL shouldShowToolbar_; On 2015/09/08 05:55:05, tapted wrote: > needs a comment. Importantly, it needs to explain why it's different to `- > (BOOL)hasToolbar` Also perhaps invert the logic so that `NO` is the default > case, then you don't need to initialize it (i.e. toolbarHidden_). But! I don't > think you need this anyway. Done. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller_private.h:79: - (void)showToolbar:(NSNotification*)notification; On 2015/09/08 05:55:05, tapted wrote: > all objc methods in .h files need comments, per go/objcguide Done. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller_private.mm:918: if (shouldShowToolbar_) { On 2015/09/08 05:55:05, tapted wrote: > I think you can read `shouldShowToolbar` by just asking the location bar whether > it's hidden (then would just need to trigger a call to layoutSubviews). But all > that should be irrelevant if we can just collapse the height of the ToolbarView > using AnimatableView. Done. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:278: bool show_location_bar_; On 2015/09/08 05:55:06, tapted wrote: > Can you just infer this from [field_ isHidden]? Done. https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/1/chrome/browser/ui/cocoa/loc... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:213: [[NSNotificationCenter defaultCenter] On 2015/09/08 05:55:06, tapted wrote: > Something like > > [[BrowserWindowController browserWindowControllerForView:[self view]] > setToolbarHidden:YES]; would probably be a lot nicer than the notifications. But > I don't think this is the way to go about it anyway. Done.
Can you also add a test to browser_window_layout_unittest.mm? https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:994: - (void)resizeView:(NSView*)view newHeight:(CGFloat)height { BWC is a `ViewResizer` -- AnimatableView's delegate. So calling setHeight or animateToNewHeight should end up calling this function, which is responsible for calling layoutSubviews. Perhaps you're missing a call to [[toolbarController animatableView] setResizeDelegate:BWC()] -- look at bookmark_bar_controller.mm:299 . You'll probably need an extra argument to initWithCommands, and update BWC initWithBrowser around line 329 so that it looks a bit more like bookmarkBarController_ The separate call to layoutSubviews in updateToolbarVisibility: shouldn't be necessary. (Right? Or, can you explain why it is needed?) https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:916: [layout setToolbarHidden:[toolbarController_ view].isHidden]; use [[toolbarController_ view] isHidden] (dot notation is only for structs) https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_layout.h (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_layout.h:54: nit: remove newline https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_layout.h:57: BOOL toolbarHidden; nit: newline after (otherwise it looks like the comment applies to all 3 params) https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:211: updateToolbarVisibility:visible withAnimation:animate]; We should be able to find a way to call it directly here sorta like [[BWC() toolbarController] updateVisibility:visible withAnimation:animate]; https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:615: return (AnimatableView*)[super view]; use base::mac::ObjCCastStrict<AnimatableView>([super view]); (and remove the DCHECK) (the usage in backgroundGradientView above is outdated..) https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:746: CGFloat newHeight = (visible ? kBaseToolbarHeightNormal : 0); parens (..) not required https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:751: [view setHeight:newHeight]; Shouldn't setting the height to zero be sufficient to hide it? (why is LayoutParams.toolbarHidden needed?)
https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_layout.h (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_layout.h:57: BOOL toolbarHidden; Also, maybe this can be obsoleted by setting toolbarHeight below to 0? (perhaps it should be called `targetToolbarHeight` in that case... in which case it could perhaps also make LocationBarViewMac::visible_ obsolete? -- i.e. it would be nicer to derive LocationBarViewMac::visible_ rather than cache it)
Having done a bit more snooping, it turns out that this is more complex than it first seems. For apps in windows, [toolbarController_ view] actually doesn't return a ToolbarView, it returns an AutoCompleteTextField (the underlying locationBarView_), because toolbars are disabled in window mode. This means that using ToolbarView (inheriting AnimatableView) for the resizing crashes the browser on resize, failing a DCHECK at browser_window_controller.mm:newHeight. AutoCompleteTextField already has some animation related code in it, but it can't inherit from AnimatableView. It looks like it'll be pretty hairy to untangle the view substitution going on here. What do you think about returning to something like my original approach, which manually toggled the height in the toolbar controller and triggered layoutSubviews? This is proving to be quite an illuminating experience.
On 2015/10/13 03:03:21, dominickn wrote: > Having done a bit more snooping, it turns out that this is more complex than it > first seems. > > For apps in windows, [toolbarController_ view] actually doesn't return a > ToolbarView, it returns an AutoCompleteTextField (the underlying > locationBarView_), because toolbars are disabled in window mode. This seems like a poor design - we should maybe change that. Instead have toolbar support a "location bar only" mode or something. Or, at least do something closer to what views does. > This means that > using ToolbarView (inheriting AnimatableView) for the resizing crashes the > browser on resize, failing a DCHECK at browser_window_controller.mm:newHeight. > AutoCompleteTextField already has some animation related code in it, but it > can't inherit from AnimatableView. Can you just plonk the AutoCompleteTextField in a new AnimatableView? (i.e. as a subview, rather than a subclass). (i.e. replicate the browser situation, but instead of the "location bar only" mode for ToolbarView, replace the ToolbarView with a vanilla AnimatableView, or some new class that inherits from AnimatableView). > It looks like it'll be pretty hairy to untangle the view substitution going on > here. If it's tangled, we should untangle it before adding more functionality. > What do you think about returning to something like my original approach, > which manually toggled the height in the toolbar controller and triggered > layoutSubviews? > > This is proving to be quite an illuminating experience.
Acknowledged. I'll have more of a think about this tomorrow. If I change the way it all fits together I might file another bug to track it. Otherwise I can also try the subview approach. Thanks!
Patchset #6 (id:90001) has been deleted
Alright, this latest version has eliminated the split view and implemented a new method to show the location bar only (and hide the remaining toolbar buttons). This makes things much cleaner. PTAL, thanks! https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:994: - (void)resizeView:(NSView*)view newHeight:(CGFloat)height { On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > BWC is a `ViewResizer` -- AnimatableView's delegate. So calling setHeight or > animateToNewHeight should end up calling this function, which is responsible for > calling layoutSubviews. > > Perhaps you're missing a call to [[toolbarController animatableView] > setResizeDelegate:BWC()] -- look at bookmark_bar_controller.mm:299 . You'll > probably need an extra argument to initWithCommands, and update BWC > initWithBrowser around line 329 so that it looks a bit more like > bookmarkBarController_ > > The separate call to layoutSubviews in updateToolbarVisibility: shouldn't be > necessary. (Right? Or, can you explain why it is needed?) Done. This eliminates the need to call layoutSubviews, thanks! https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:916: [layout setToolbarHidden:[toolbarController_ view].isHidden]; On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > use [[toolbarController_ view] isHidden] (dot notation is only for structs) Changes removed https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_layout.h (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_layout.h:57: BOOL toolbarHidden; Changes discarded. https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:211: updateToolbarVisibility:visible withAnimation:animate]; On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > We should be able to find a way to call it directly here sorta like > > [[BWC() toolbarController] updateVisibility:visible withAnimation:animate]; Done. https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:615: return (AnimatableView*)[super view]; On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > use base::mac::ObjCCastStrict<AnimatableView>([super view]); (and remove the > DCHECK) > > (the usage in backgroundGradientView above is outdated..) Done. https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:746: CGFloat newHeight = (visible ? kBaseToolbarHeightNormal : 0); On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > parens (..) not required Done. https://codereview.chromium.org/1305143008/diff/30011/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:751: [view setHeight:newHeight]; On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > Shouldn't setting the height to zero be sufficient to hide it? (why is > LayoutParams.toolbarHidden needed?) That was previously necessary because of the lack of setting the resizeDelegate. Thanks!
Still going through review, so this was a bit rushed (all I have for now - sorry!) Also: not sure if you saw this: On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > Can you also add a test to browser_window_layout_unittest.mm? https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:98: // the toolbar model and back/forward menus. nit: update comment here https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:249: nibFileNamed:@"Toolbar"])) { shouldn't a resizeDelegate argument be required for the nibFileNamed overload as well, to support the header change? https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:253: // Set the resize delegate (BWC) so that resizing triggers a layout redraw nit: this comment doesn't add much, and can be dropped https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:595: // Make location bar not editable when in a pop-up. is this consistent with what views does? (then, update comment to mention the new window style?) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:600: [self shouldHideToolbarButtons:(!toolbar && locBar)]; nit: no parens required https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:606: return (BackgroundGradientView*)[self view]; might as well change this to ObjCCastStrict if diff is picking it up anyway https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:611: DCHECK([[self view] isKindOfClass:[AnimatableView class]]); DCHECK not required (happens in ObjCCastStrict) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:737: - (void)updateVisibility:(BOOL)visible withAnimation:(BOOL)animate { setToolbarHidden? https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:746: [self setHasToolbar:hasToolbar_ hasLocationBar:visible]; DCHECK(hasToolbar_)? (and pass YES) here? https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:753: // Perform the animation, which will cause the BWC to resize this view in nit: (shouldn't abbreviate in comments): BWC -> BrowserWindowController https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:784: CGFloat moveX = [backButton_ frame].size.width + I think this stuff should be part of the layout function (e.g. does calling layout again clobber the changes here?) i.e. set a bool - preferably something that can be set in the initialiser and never changes.. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:965: ToolbarView* toolbarView = (ToolbarView*)view; ObjCCastStrict But! actually. Now I see this code, I think we should just make a toolbarView accessor that does this, and just get rid of the backgroundGradieView and animatableView accessors
https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:766: if (hasToolbar_ || !hasLocationBar_) I think the only caller does [self shouldHideToolbarButtons:(!toolbar && locBar)]; arg = !toolbar && locBar if = toolbar || !locBar toolbar locBar arg if YES YES YES NO YES NO NO YES NO YES NO YES NO NO NO YES So... is it correct that, after the return below, `hide` will always be true? In which case I think this method can just be called showLocationBarOnly (or permanentlyHideAllButtons), and document that there is no way to escape from this state. Then all the layout logic below can just boil down to locationFrame = [locationBar_ frame]; locationFrame.origin.x = 0; locationFrame.size.width = NSWidth(<toolbar>) - padding? [locationBar_ setFrame:locationFrame]; [button* setHidden:YES]; https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:771: DCHECK([backButton_ isHidden] == [forwardButton_ isHidden] && perhaps DCHECK_EQ(NO, [backButton_ isHidden] | [forwardButton_ isHidden] | ...) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:774: BOOL hidden = [backButton_ isHidden]; hidden isn't used after this, so I'd move this into the `if` https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:784: CGFloat moveX = [backButton_ frame].size.width + On 2015/10/16 00:06:17, tapted (traveling until Oct22) wrote: > I think this stuff should be part of the layout function (e.g. does calling > layout again clobber the changes here?) > > i.e. set a bool - preferably something that can be set in the initialiser and > never changes.. Hm, since the homeButton hiding is here, I guess here is appropriate. But try to keep this method together with the home button hiding. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:785: [forwardButton_ frame].size.width + It's more typical to do NSWidth([forwardButton_ frame]) than this https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:786: [reloadButton_ frame].size.width - 3.0; If these are laid out together already, you can just do NSMaxX([reloadButton_ frame]) (which should be the sum of widths, plus the padding on the left of the back button) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:786: [reloadButton_ frame].size.width - 3.0; This "3" feels a bit magic. Is is 3 * kSomeNegativePaddingOrSomething. I don't understand the comment about the frame edges being on top of each other (maybe refer to a method that sets these?) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:805: [self adjustLocationSizeBy:dX animate:NO]; since this is being called on `self` and we pass `NO` to animate, I think this will be simpler with something like NSView* lastButton = showHomeButton_.GetValue() ? homeButton_ : homeButton_; locationFrame = [locationBar_ frame]; locationFrame.origin.x = hide ? 0 : NSMaxX([lastButton frame]); locationFrame.size.width = NSWidth(<toolbar>) - (hide ? NSMinX([browserActionsContainerView_ frame] : 0) - NSMinX(locationFrame); [locationBar_ setFrame:locationFrame]; (plus any padding you need) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:806: [locationBar_ setFrame:[self adjustRect:[locationBar_ frame] this clobbers the [locationBar_ setFrame:locationFrame]; done in adjustLocationSizeBy no?
Description was changed from ========== [Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() This CL allows bookmark apps opened in windows on Mac to show and hide the location bar in a manner consistent with other platforms. The location bar is hidden when the window is at the same origin as the bookmark app, and shown when the window is navigated away from that origin. The resizing is implemented in a manner consistent with the bookmarks bar. Prior to this CL, app windows did not display the toolbar, and only displayed the location bar (an NSTextField subclass). This precluded the bar from being resized in the same manner as the bookmarks bar. This CL refactors this so that the view-level distinction is removed. App windows which display the location bar now display the full toolbar, with the undesired navigation and wrench buttons hidden. BUG=491792,499741 ========== to ========== [Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() This CL allows bookmark apps opened in windows on Mac to show and hide the location bar in a manner consistent with other platforms. The location bar is hidden when the window is at the same origin as the bookmark app, and shown when the window is navigated away from that origin. The resizing is implemented in a manner consistent with the bookmarks bar. Prior to this CL, app windows did not display the toolbar, and only displayed the location bar (an NSTextField subclass). This precluded the bar from being resized in the same manner as the bookmarks bar. This CL refactors this so that the view-level distinction is removed. App windows which display the location bar now display the full toolbar, with the undesired navigation and wrench buttons hidden. BUG=491792,499741 ==========
Thanks! On 2015/10/12 23:56:43, tapted (traveling until Oct22) wrote: > Can you also add a test to browser_window_layout_unittest.mm? I added the test to toolbar_controller_unittests because there are no longer any changes to browser_window_layout (and it has a test for the toolbar not shown / location bar shown layout). https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:98: // the toolbar model and back/forward menus. On 2015/10/16 00:06:16, tapted wrote: > nit: update comment here Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:249: nibFileNamed:@"Toolbar"])) { On 2015/10/16 00:06:16, tapted wrote: > shouldn't a resizeDelegate argument be required for the nibFileNamed overload as > well, to support the header change? Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:253: // Set the resize delegate (BWC) so that resizing triggers a layout redraw On 2015/10/16 00:06:17, tapted wrote: > nit: this comment doesn't add much, and can be dropped Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:595: // Make location bar not editable when in a pop-up. On 2015/10/16 00:06:16, tapted wrote: > is this consistent with what views does? (then, update comment to mention the > new window style?) Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:600: [self shouldHideToolbarButtons:(!toolbar && locBar)]; On 2015/10/16 00:06:16, tapted wrote: > nit: no parens required Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:606: return (BackgroundGradientView*)[self view]; On 2015/10/16 00:06:17, tapted wrote: > might as well change this to ObjCCastStrict if diff is picking it up anyway Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:611: DCHECK([[self view] isKindOfClass:[AnimatableView class]]); On 2015/10/16 00:06:16, tapted wrote: > DCHECK not required (happens in ObjCCastStrict) Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:737: - (void)updateVisibility:(BOOL)visible withAnimation:(BOOL)animate { On 2015/10/16 00:06:17, tapted wrote: > setToolbarHidden? This method can set the toolbar to hidden or visible, so I didn't use that name. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:746: [self setHasToolbar:hasToolbar_ hasLocationBar:visible]; On 2015/10/16 00:06:17, tapted wrote: > DCHECK(hasToolbar_)? (and pass YES) here? hasToolbar_ isn't usually YES in this case. For bookmark apps, hasToolbar_ is NO and hasLocationBar_ is YES https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:753: // Perform the animation, which will cause the BWC to resize this view in On 2015/10/16 00:06:17, tapted wrote: > nit: (shouldn't abbreviate in comments): BWC -> BrowserWindowController Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:766: if (hasToolbar_ || !hasLocationBar_) On 2015/10/17 00:44:15, tapted wrote: > I think the only caller does > > [self shouldHideToolbarButtons:(!toolbar && locBar)]; > > arg = !toolbar && locBar > if = toolbar || !locBar > > toolbar locBar arg if > YES YES YES NO > YES NO NO YES > NO YES NO YES > NO NO NO YES > > > So... is it correct that, after the return below, `hide` will always be true? > > > In which case I think this method can just be called showLocationBarOnly (or > permanentlyHideAllButtons), and document that there is no way to escape from > this state. > > Then all the layout logic below can just boil down to > > locationFrame = [locationBar_ frame]; > locationFrame.origin.x = 0; > locationFrame.size.width = NSWidth(<toolbar>) - padding? > [locationBar_ setFrame:locationFrame]; > > [button* setHidden:YES]; I tried to keep this as general as possible so that it could be unwound. But I'm happy to keep it as a one-way transform as that will make it much cleaner. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:771: DCHECK([backButton_ isHidden] == [forwardButton_ isHidden] && On 2015/10/17 00:44:15, tapted wrote: > perhaps > > DCHECK_EQ(NO, [backButton_ isHidden] | [forwardButton_ isHidden] | ...) Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:774: BOOL hidden = [backButton_ isHidden]; On 2015/10/17 00:44:15, tapted wrote: > hidden isn't used after this, so I'd move this into the `if` Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:784: CGFloat moveX = [backButton_ frame].size.width + On 2015/10/17 00:44:15, tapted wrote: > On 2015/10/16 00:06:17, tapted (traveling until Oct22) wrote: > > I think this stuff should be part of the layout function (e.g. does calling > > layout again clobber the changes here?) > > > > i.e. set a bool - preferably something that can be set in the initialiser and > > never changes.. > > Hm, since the homeButton hiding is here, I guess here is appropriate. But try to > keep this method together with the home button hiding. Done. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:806: [locationBar_ setFrame:[self adjustRect:[locationBar_ frame] On 2015/10/17 00:44:15, tapted wrote: > this clobbers the [locationBar_ setFrame:locationFrame]; done in > adjustLocationSizeBy no? It's assumed that adjustLocationSizeBy won't be called in conjunction with this: I'll document it. https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:965: ToolbarView* toolbarView = (ToolbarView*)view; On 2015/10/16 00:06:16, tapted wrote: > ObjCCastStrict > > > But! actually. Now I see this code, I think we should just make a toolbarView > accessor that does this, and just get rid of the backgroundGradieView and > animatableView accessors Done.
Looking pretty good! As well as window resizing, we also need to check that the buttons are happy in this state. Like, I'm not sure about extensions - -should they be able to show their browser action if they do "stuff" Does wrench behave -- e.g. do we ensure that we don't try to show bubbles on it like "extension blah is requesting new permissions" (when in doubt, do what views does) https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/110001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:737: - (void)updateVisibility:(BOOL)visible withAnimation:(BOOL)animate { On 2015/10/20 04:40:22, dominickn wrote: > On 2015/10/16 00:06:17, tapted wrote: > > setToolbarHidden? > > This method can set the toolbar to hidden or visible, so I didn't use that name. Ah, I meant since Cocoa nomenclature is "[NSView isHidden]", and this is an ObjectiveC function, we should use the same, and flip the logic in the bridge instead. This would make the `if` below more readable and avoid the need for the explainer comment above it. But, `hasLocationBar` is the reverse already, and it was there before you, so I don't feel strongly about this. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:172: // Hide the back, forward, reload, home, and wrench buttons of the toolbar, nit: no comma after `toolbar,`. Or a new sentence, e.g. This allows the location bar to occupy https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:175: // the toolbar or location bar width should be made. what if the window is resized? https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:176: - (void)showLocationBarOnly; move this declaration to `@interface ToolbarController()` in the .mm file (and reorder method definitions to match) https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:189: // A set of private methods used by subclasses. Do not call these directly This comment seems out of date... (pretty sure nothing calls this from a subclass, otherwise stuff would have broke) I think this declaration and the other methods definiton can be removed, remove the `nibFileNamed` argument in the overload's definition, and substitute nibName with the @"Toolbar" string literal. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:74: const NSTimeInterval kToolBarAnimationDuration = 0.12; Can this be shared with something? (just asking... not sure if that's a good change) https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:742: [self setHasToolbar:hasToolbar_ hasLocationBar:visible]; This is in the wrong place. But.. I'm wondering if it can simply be removed... The constructor of a Browser calls HostedAppBrowserController::UpdateLocationBarVisibility(false). Presumably that's because something has already called setHasToolbar:NO hasLocationBar:YES -- maybe this is only (effectively) happening on views at the moment, and there's a missing piece for Cocoa where that should be happening during construction. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:762: if (hasToolbar_ || !hasLocationBar_) Can this be a DCHECK now? (with the reverse logic) https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:786: if (!showHomeButton_.GetValue()) This check isn't needed https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:935: ToolbarView* view = [self toolbarView]; view -> toolbarView https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:137: // Simulate a popup going fullscreen and back by performing the reparenting Make sure you test this to ensure it still behaves https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:155: [bar_ setHasToolbar:NO hasLocationBar:YES]; Before calling this, EXPECT_FOO the button visibility for the other states (which *can* be returned from). I.e., do setHasToolbar:YES hasLocationBar:YES setHasToolbar:NO hasLocationBar:NO setHasToolbar:NO hasLocationBar:YES https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:157: EXPECT_FALSE([[[bar_ toolbarViews] objectAtIndex:kLocationIndex] isHidden]); This is a lot of repetition. Since bar_ is a data member, I think nicest would be a helper method on ToolbarControllerTest like NSView* ToolbarControllerTest::GetSubviewAt(SubviewIndex index) {..} (after giving the enum a name) https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:173: EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]); what about the browser action container?
Also, we shouldn't change the appearance of popups. And probably bookmark apps with a location bar need to look more like the popup look anyway. See http://imgur.com/a/XSki4 -- top image is a popup currently - middle is a hosted app with this patch, bottom is a popup with this patch.
Description was changed from ========== [Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() This CL allows bookmark apps opened in windows on Mac to show and hide the location bar in a manner consistent with other platforms. The location bar is hidden when the window is at the same origin as the bookmark app, and shown when the window is navigated away from that origin. The resizing is implemented in a manner consistent with the bookmarks bar. Prior to this CL, app windows did not display the toolbar, and only displayed the location bar (an NSTextField subclass). This precluded the bar from being resized in the same manner as the bookmarks bar. This CL refactors this so that the view-level distinction is removed. App windows which display the location bar now display the full toolbar, with the undesired navigation and wrench buttons hidden. BUG=491792,499741 ========== to ========== [Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() This CL allows bookmark apps opened in windows on Mac to show and hide the location bar in a manner consistent with other platforms. The location bar is hidden when the window is at the same origin as the bookmark app, and shown when the window is navigated away from that origin. The resizing is implemented in a manner consistent with the bookmarks bar. Prior to this CL, app windows and popups did not display the toolbar, and only displayed the location bar (an NSTextField subclass). This prevented the toolbar from being resized in the same manner as the bookmarks bar. This CL refactors this so that the view-level distinction is removed. App windows and popups which display the location bar now display the full toolbar, with the undesired navigation and wrench buttons hidden. BUG=491792,499741 ==========
Popup windows have been restored to their previous look, with bookmark app windows now looking the same as popups. PTAL, thanks! https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:172: // Hide the back, forward, reload, home, and wrench buttons of the toolbar, On 2015/10/20 06:31:08, tapted wrote: > nit: no comma after `toolbar,`. Or a new sentence, e.g. This allows the location > bar to occupy Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:175: // the toolbar or location bar width should be made. On 2015/10/20 06:31:08, tapted wrote: > what if the window is resized? That *just works*. Have updated the comment. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:176: - (void)showLocationBarOnly; On 2015/10/20 06:31:09, tapted wrote: > move this declaration to `@interface ToolbarController()` in the .mm file (and > reorder method definitions to match) Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:189: // A set of private methods used by subclasses. Do not call these directly On 2015/10/20 06:31:08, tapted wrote: > This comment seems out of date... (pretty sure nothing calls this from a > subclass, otherwise stuff would have broke) > > I think this declaration and the other methods definiton can be removed, remove > the `nibFileNamed` argument in the overload's definition, and substitute nibName > with the @"Toolbar" string literal. Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:74: const NSTimeInterval kToolBarAnimationDuration = 0.12; On 2015/10/20 06:31:09, tapted wrote: > Can this be shared with something? (just asking... not sure if that's a good > change) I set this value to be the same as the bookmark bar animation, but I don't know if it's worth sharing the value between the two as they both currently use an anonymous constant for it. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:742: [self setHasToolbar:hasToolbar_ hasLocationBar:visible]; On 2015/10/20 06:31:09, tapted wrote: > This is in the wrong place. But.. I'm wondering if it can simply be removed... > > The constructor of a Browser calls > HostedAppBrowserController::UpdateLocationBarVisibility(false). > > Presumably that's because something has already called setHasToolbar:NO > hasLocationBar:YES -- maybe this is only (effectively) happening on views at the > moment, and there's a missing piece for Cocoa where that should be happening > during construction. HostedAppBrowserController::UpdateLocationBarVisibility(false) eventually calls this function via the LocationBarViewMac, and that call is the same as when the visibility is changed. I've changed how showLocationOnly is called and moved it explicitly here to resolve the pop up window visual differences. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:762: if (hasToolbar_ || !hasLocationBar_) On 2015/10/20 06:31:09, tapted wrote: > Can this be a DCHECK now? (with the reverse logic) Now that the callsite for this method has changed, this can only be a DCHECK if the callsite is guarded by an if (!hasToolbar_ && hasLocationBar_) (since setHasToolbar will update the location bar visibility). https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:786: if (!showHomeButton_.GetValue()) On 2015/10/20 06:31:09, tapted wrote: > This check isn't needed Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:935: ToolbarView* view = [self toolbarView]; On 2015/10/20 06:31:09, tapted wrote: > view -> toolbarView Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:137: // Simulate a popup going fullscreen and back by performing the reparenting On 2015/10/20 06:31:09, tapted wrote: > Make sure you test this to ensure it still behaves This should now be unaffected by the showLocationBarOnly call; the only change is that it's the toolbarView being used, not locationBarView. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:155: [bar_ setHasToolbar:NO hasLocationBar:YES]; On 2015/10/20 06:31:09, tapted wrote: > Before calling this, EXPECT_FOO the button visibility for the other states > (which *can* be returned from). I.e., do > > setHasToolbar:YES hasLocationBar:YES > setHasToolbar:NO hasLocationBar:NO > setHasToolbar:NO hasLocationBar:YES Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:157: EXPECT_FALSE([[[bar_ toolbarViews] objectAtIndex:kLocationIndex] isHidden]); On 2015/10/20 06:31:09, tapted wrote: > This is a lot of repetition. Since bar_ is a data member, I think nicest would > be a helper method on ToolbarControllerTest like > > NSView* ToolbarControllerTest::GetSubviewAt(SubviewIndex index) {..} > > (after giving the enum a name) Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:173: EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]); On 2015/10/20 06:31:09, tapted wrote: > what about the browser action container? I believe that for popup and bookmark app windows there is no browser action controller.
https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:74: const NSTimeInterval kToolBarAnimationDuration = 0.12; On 2015/10/26 02:53:04, dominickn wrote: > On 2015/10/20 06:31:09, tapted wrote: > > Can this be shared with something? (just asking... not sure if that's a good > > change) > > I set this value to be the same as the bookmark bar animation, but I don't know > if it's worth sharing the value between the two as they both currently use an > anonymous constant for it. So probably it should move to animatable_view.mm and the `duration` argument removed, since everything passes 0.12. Something for another CL though. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:742: [self setHasToolbar:hasToolbar_ hasLocationBar:visible]; On 2015/10/26 02:53:04, dominickn wrote: > On 2015/10/20 06:31:09, tapted wrote: > > This is in the wrong place. But.. I'm wondering if it can simply be removed... > > > > The constructor of a Browser calls > > HostedAppBrowserController::UpdateLocationBarVisibility(false). > > > > Presumably that's because something has already called setHasToolbar:NO > > hasLocationBar:YES -- maybe this is only (effectively) happening on views at > the > > moment, and there's a missing piece for Cocoa where that should be happening > > during construction. > > HostedAppBrowserController::UpdateLocationBarVisibility(false) eventually calls > this function via the LocationBarViewMac, and that call is the same as when the > visibility is changed. I've changed how showLocationOnly is called and moved it > explicitly here to resolve the pop up window visual differences. The point is that, currently, browser_window_controller.mm is the only place that calls setHasToolbar:hasLocationBar and it does it exactly once, immediately after the ToolbarController is initialized. What breaks if this line is removed? And (assuming something breaks), can it be resolved by having the current call site always pass YES for hasLocationBar? (perhaps, only when there is a hosted_app_controller_). That is, the case where there *is* a hosted_app_controller but the location bar is hidden should be _always_ handled by setting the height to 0. If hasLocationBar starts off as `NO` then the toolbar is in a strange state until the first time it's shown (sure, it's hidden, so you can't _see_ the strange state, but it's there..) https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:137: // Simulate a popup going fullscreen and back by performing the reparenting On 2015/10/26 02:53:04, dominickn wrote: > On 2015/10/20 06:31:09, tapted wrote: > > Make sure you test this to ensure it still behaves > > This should now be unaffected by the showLocationBarOnly call; the only change > is that it's the toolbarView being used, not locationBarView. So did you open site that does HTML5 fullscreen in a popup, and ask it to go fullscreen? The issue is that there used to be code around this to resolve ownership problems that came about with the weird locationView hack. The hierarchy is now changed, so ownership patters could break. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:155: [bar_ setHasToolbar:NO hasLocationBar:YES]; On 2015/10/26 02:53:04, dominickn wrote: > On 2015/10/20 06:31:09, tapted wrote: > > Before calling this, EXPECT_FOO the button visibility for the other states > > (which *can* be returned from). I.e., do > > > > setHasToolbar:YES hasLocationBar:YES > > setHasToolbar:NO hasLocationBar:NO > > setHasToolbar:NO hasLocationBar:YES > > Done. Ping https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:173: EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]); On 2015/10/26 02:53:04, dominickn wrote: > On 2015/10/20 06:31:09, tapted wrote: > > what about the browser action container? > > I believe that for popup and bookmark app windows there is no browser action > controller. Can we include it in the test? https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:208: // to update. This comment is a bit confusing. Perhaps just "To show or hide the location bar, coordinate with the toolbar that contains it". Or even just drop the comment - doesn't add much. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:150: nit: remove blank line https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:865: NSRect locationFrame = [locationBar_ frame]; nit: remove (see below) https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:867: // Adjust the toolbar frame to match the height of the location bar. perhaps "Ensure the location bar fills the toolbar." https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:868: toolbarFrame.size.height = locationFrame.size.height; Unless kLocationBarHeight can be made obsolete, I think you should just use that here. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:875: [locationBar_ setFrame:locationFrame]; Not much from locationFrame ends up getting used, so I'd go for [locationBar_ setFrame:NSMakeRect(0, 0, NSWidth([[self view] frame]), kLocationBarHeight)];
Thanks! https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:74: const NSTimeInterval kToolBarAnimationDuration = 0.12; On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > On 2015/10/26 02:53:04, dominickn wrote: > > On 2015/10/20 06:31:09, tapted wrote: > > > Can this be shared with something? (just asking... not sure if that's a > good > > > change) > > > > I set this value to be the same as the bookmark bar animation, but I don't > know > > if it's worth sharing the value between the two as they both currently use an > > anonymous constant for it. > > So probably it should move to animatable_view.mm and the `duration` argument > removed, since everything passes 0.12. Something for another CL though. Acknowledged. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:742: [self setHasToolbar:hasToolbar_ hasLocationBar:visible]; Removing the call breaks the animation for bookmark apps because it's using the isHidden property of the location bar view to toggle the visible/not visible state. hasLocationBar is always YES for bookmark apps anyway, so that doesn't fix the issue. I've added a call to [view setHidden:!visible] to fix this. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:137: // Simulate a popup going fullscreen and back by performing the reparenting On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > On 2015/10/26 02:53:04, dominickn wrote: > > On 2015/10/20 06:31:09, tapted wrote: > > > Make sure you test this to ensure it still behaves > > > > This should now be unaffected by the showLocationBarOnly call; the only change > > is that it's the toolbarView being used, not locationBarView. > > So did you open site that does HTML5 fullscreen in a popup, and ask it to go > fullscreen? > > The issue is that there used to be code around this to resolve ownership > problems that came about with the weird locationView hack. The hierarchy is now > changed, so ownership patters could break. I tested a site with HTML5 fullscreen in a popup and it seemed to work the same as without the patch. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:155: [bar_ setHasToolbar:NO hasLocationBar:YES]; On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > On 2015/10/26 02:53:04, dominickn wrote: > > On 2015/10/20 06:31:09, tapted wrote: > > > Before calling this, EXPECT_FOO the button visibility for the other states > > > (which *can* be returned from). I.e., do > > > > > > setHasToolbar:YES hasLocationBar:YES > > > setHasToolbar:NO hasLocationBar:NO > > > setHasToolbar:NO hasLocationBar:YES > > > > Done. > > Ping Oops. Done. https://codereview.chromium.org/1305143008/diff/130001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:173: EXPECT_TRUE([[[bar_ toolbarViews] objectAtIndex:kHomeIndex] isHidden]); On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > On 2015/10/26 02:53:04, dominickn wrote: > > On 2015/10/20 06:31:09, tapted wrote: > > > what about the browser action container? > > > > I believe that for popup and bookmark app windows there is no browser action > > controller. > > Can we include it in the test? Done. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:208: // to update. On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > This comment is a bit confusing. Perhaps just "To show or hide the location bar, > coordinate with the toolbar that contains it". Or even just drop the comment - > doesn't add much. Done. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:150: On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > nit: remove blank line Done. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:865: NSRect locationFrame = [locationBar_ frame]; On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > nit: remove (see below) Done. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:867: // Adjust the toolbar frame to match the height of the location bar. On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > perhaps "Ensure the location bar fills the toolbar." Done. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:868: toolbarFrame.size.height = locationFrame.size.height; On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > Unless kLocationBarHeight can be made obsolete, I think you should just use that > here. Done. https://codereview.chromium.org/1305143008/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:875: [locationBar_ setFrame:locationFrame]; On 2015/10/26 04:40:55, tapted (OOO soon) wrote: > Not much from locationFrame ends up getting used, so I'd go for > > [locationBar_ setFrame:NSMakeRect(0, 0, NSWidth([[self view] frame]), > kLocationBarHeight)]; Done.
now that setHasToolbar is back to being called only once during construction, there are a few other things to check up on https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:589: // Display the toolbar with only the location bar inside it. No-ops if the The No-ops comment is no longer needed, now we are assured that setHasToolbar: is always called exactly once https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:739: [view setHidden:!visible]; I think this line should be removed. The problem is that setting it hidden gives AnimatableView nothing to animate when hiding. When I try it, it first flashes to grey, then (effectively) it's just the WebContentsView that animates up. So, then I think the problem is that you lose the `target` visibility, and AnimatableView doesn't offer a way to get it. Two options: 1) Suppress redundant calls to updateVisibility elsewhere, or find other ways to eliminate the redundant calls themselves so we don't have to bother suppressing them 2) Add (back?) the `BOOL locationBarHidden_;` member (here, or a `bool` on LocationBarViewMac) and comment about its purpose and where the redundant calls come from I guess AnimatableView could also be updated to provide some kind of targetVisibility, but one of those seems more appropriate https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:856: if ([backButton_ isHidden]) { Likewise, this can be // -showLocationBarOnly is only ever called once, shortly after initialization, // so the regular buttons should all be visible. DCHECK(![backButton_ isHidden]); (probably not worth checking the rest. And perhaps even better just to remove this entirely. (needs something though, since I'm pretty sure the DCHECK_EQ below is now unreachable) https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:147: [bar_ setHasToolbar:YES hasLocationBar:YES]; Hm - does this make sense? I think we say that the state set above is inescapable. Can you see if this test went in to test something specific? Maybe it is not testing anything interesting, since this will presumably fail if you try my `DCHECK(![backButton_ isHidden]);` suggestion
Thanks! https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:589: // Display the toolbar with only the location bar inside it. No-ops if the On 2015/10/26 07:55:34, tapted (OOO soon) wrote: > The No-ops comment is no longer needed, now we are assured that setHasToolbar: > is always called exactly once Done. https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:739: [view setHidden:!visible]; On 2015/10/26 07:55:34, tapted (OOO soon) wrote: > I think this line should be removed. The problem is that setting it hidden gives > AnimatableView nothing to animate when hiding. When I try it, it first flashes > to grey, then (effectively) it's just the WebContentsView that animates up. > > So, then I think the problem is that you lose the `target` visibility, and > AnimatableView doesn't offer a way to get it. > > Two options: > 1) Suppress redundant calls to updateVisibility elsewhere, or find other ways > to eliminate the redundant calls themselves so we don't have to bother > suppressing them > 2) Add (back?) the `BOOL locationBarHidden_;` member (here, or a `bool` on > LocationBarViewMac) and comment about its purpose and where the redundant calls > come from > > I guess AnimatableView could also be updated to provide some kind of > targetVisibility, but one of those seems more appropriate Done (option 2). https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:856: if ([backButton_ isHidden]) { On 2015/10/26 07:55:34, tapted (OOO soon) wrote: > Likewise, this can be > > // -showLocationBarOnly is only ever called once, shortly after initialization, > // so the regular buttons should all be visible. > DCHECK(![backButton_ isHidden]); > > (probably not worth checking the rest. > > And perhaps even better just to remove this entirely. (needs something though, > since I'm pretty sure the DCHECK_EQ below is now unreachable) Done. https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm (right): https://codereview.chromium.org/1305143008/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller_unittest.mm:147: [bar_ setHasToolbar:YES hasLocationBar:YES]; On 2015/10/26 07:55:34, tapted (OOO soon) wrote: > Hm - does this make sense? I think we say that the state set above is > inescapable. Can you see if this test went in to test something specific? Maybe > it is not testing anything interesting, since this will presumably fail if you > try my `DCHECK(![backButton_ isHidden]);` suggestion From the blame and log, it doesn't look like it's testing any specific situation. The call was added when the setHasToolbar was refactored from one to two arguments. I've removed this call. This particular test and a few others look a bit odd (not really any EXPECT calls); I might have a look in another CL to make them more comprehensive.
lgtm % nits https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:205: // If a visibility change is requested and the required visibility doesn't This mostly restates the code. I think the important part is something like // Track the target location bar visibility to avoid redundant transitions // being initiated when one is already in progress. https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:168: // Updates the visibility of the toolbar if the requested visibility differs nit: remove "if the requested visibility differs from the current visibility, " https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:723: ToolbarView* view = [self toolbarView]; nit: this can just be used inline, below. There's no longer any codepath that evaluates it more than once, and this should give more context to the usage below without negatively affecting code format/readability.
Thanks for all of the help and patience! :) https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:205: // If a visibility change is requested and the required visibility doesn't On 2015/10/26 23:48:08, tapted (OOO soon) wrote: > This mostly restates the code. I think the important part is something like > > // Track the target location bar visibility to avoid redundant transitions > // being initiated when one is already in progress. Done. https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.h (right): https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.h:168: // Updates the visibility of the toolbar if the requested visibility differs On 2015/10/26 23:48:08, tapted (OOO soon) wrote: > nit: remove "if the requested visibility differs from the current visibility, " Done. https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/1305143008/diff/210001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:723: ToolbarView* view = [self toolbarView]; On 2015/10/26 23:48:08, tapted (OOO soon) wrote: > nit: this can just be used inline, below. There's no longer any codepath that > evaluates it more than once, and this should give more context to the usage > below without negatively affecting code format/readability. Done.
Description was changed from ========== [Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() This CL allows bookmark apps opened in windows on Mac to show and hide the location bar in a manner consistent with other platforms. The location bar is hidden when the window is at the same origin as the bookmark app, and shown when the window is navigated away from that origin. The resizing is implemented in a manner consistent with the bookmarks bar. Prior to this CL, app windows and popups did not display the toolbar, and only displayed the location bar (an NSTextField subclass). This prevented the toolbar from being resized in the same manner as the bookmarks bar. This CL refactors this so that the view-level distinction is removed. App windows and popups which display the location bar now display the full toolbar, with the undesired navigation and wrench buttons hidden. BUG=491792,499741 ========== to ========== [Mac] Implement LocationBarViewMac::UpdateLocationBarVisibility() This CL allows bookmark apps opened in windows on Mac to show and hide the location bar in a manner consistent with other platforms. The location bar is hidden when the window is at the same origin as the bookmark app, and shown when the window is navigated away from that origin. The resizing is implemented in a manner consistent with the bookmarks bar. Prior to this CL, app windows and popups did not display the full toolbar view. Instead, they only displayed the location bar view (an NSTextField subclass). This view-level substitution prevented the app window toolbar from being resized in the same manner as the bookmarks bar. This CL refactors the implementation so that the view-level distinction is removed. App windows and popups which only display the location bar now use the full toolbar view, with the location bar resized to be full width and the undesired navigation and wrench buttons hidden. BUG=491792,499741 ==========
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1305143008/#ps230001 (title: "Addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305143008/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305143008/230001
Message was sent while issue was closed.
Committed patchset #12 (id:230001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/5c98d330e26e8547932f7fdac8ef1b55e928c782 Cr-Commit-Position: refs/heads/master@{#356226} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
