Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14689007:
Instant Extended: Move omnibox dropdown by 2 pixels (Closed)
|
Created:
7 years, 7 months ago by sail Modified:
7 years, 7 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInstant Extended: Move omnibox dropdown by 2 pixels
This CL moves the omnibox dropdown by 2 pixels.
When the bookmark bar is not pinned the dropdown is now below the toolbar. Previously the dropdown would overlap the toolbar.
Screenshots with bookmark bar not pinned:
before: http://i.imgur.com/62PBHS1.png
after: http://i.imgur.com/3Y6HqGd.png
Screenshots with bookmark bar pinned:
before: http://i.imgur.com/snvuulZ.png
after: http://i.imgur.com/jhI5LFm.png
BUG=235507
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202354
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/14689007/diff/2001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/2001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:264: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; why -1?
https://codereview.chromium.org/14689007/diff/2001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/2001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:264: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; On 2013/05/16 21:50:15, Nico wrote: > why -1? This is for the toolbar separator. Added a comment explaining this.
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; Why was this not needed before? This code is independent of any overlays, no?
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; On 2013/05/16 22:17:57, Nico wrote: > Why was this not needed before? This code is independent of any overlays, no? Previously the overlay never overlapped the toolbar separator. If the bookmark bar was pinned then the toolbar didn't draw a separator. If the bookmark bar was not pinned then the overlay was a few pixels above the bottom of the toolbar. Now when the bookmark bar is not pinned it exactly overlaps the toolbar separator.
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; On 2013/05/16 22:35:30, sail wrote: > On 2013/05/16 22:17:57, Nico wrote: > > Why was this not needed before? This code is independent of any overlays, no? > > Previously the overlay never overlapped the toolbar separator. > > If the bookmark bar was pinned then the toolbar didn't draw a separator. > If the bookmark bar was not pinned then the overlay was a few pixels above the > bottom of the toolbar. > > Now when the bookmark bar is not pinned it exactly overlaps the toolbar > separator. Should the function just return the right value then? Also, do all callers now always pass 0 as parameter? If any don't, is that a bug? chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm still references bookmarks::kBookmarkBarOverlap – is that still correct?
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; On 2013/05/16 22:55:58, Nico wrote: > On 2013/05/16 22:35:30, sail wrote: > > On 2013/05/16 22:17:57, Nico wrote: > > > Why was this not needed before? This code is independent of any overlays, > no? > > > > Previously the overlay never overlapped the toolbar separator. > > > > If the bookmark bar was pinned then the toolbar didn't draw a separator. > > If the bookmark bar was not pinned then the overlay was a few pixels above the > > bottom of the toolbar. > > > > Now when the bookmark bar is not pinned it exactly overlaps the toolbar > > separator. > > Should the function just return the right value then? Also, do all callers now > always pass 0 as parameter? If any don't, is that a bug? Only the overlay passes a constant value here. That's because the overlay position is fixed with respect to the omnibox. All other UIs (bookmark bar, etc...) are relative to the bottom of the toolbar. > > chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm still references > bookmarks::kBookmarkBarOverlap – is that still correct? Yea, this is still correct.
ping
Updated the CL description.
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; On 2013/05/16 23:27:06, sail wrote: > On 2013/05/16 22:55:58, Nico wrote: > > On 2013/05/16 22:35:30, sail wrote: > > > On 2013/05/16 22:17:57, Nico wrote: > > > > Why was this not needed before? This code is independent of any overlays, > > no? > > > > > > Previously the overlay never overlapped the toolbar separator. > > > > > > If the bookmark bar was pinned then the toolbar didn't draw a separator. > > > If the bookmark bar was not pinned then the overlay was a few pixels above > the > > > bottom of the toolbar. > > > > > > Now when the bookmark bar is not pinned it exactly overlaps the toolbar > > > separator. > > > > Should the function just return the right value then? Also, do all callers now > > always pass 0 as parameter? If any don't, is that a bug? > > Only the overlay passes a constant value here. That's because the overlay > position is fixed with respect to the omnibox. All other UIs (bookmark bar, > etc...) are relative to the bottom of the toolbar. I still don't get it. This code is laying out the normal content area in a normal browser window, right? How is this line related to the overlay? > > > > > chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm still references > > bookmarks::kBookmarkBarOverlap – is that still correct? > > Yea, this is still correct.
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; > I still don't get it. This code is laying out the normal content area in a > normal browser window, right? Right. > How is this line related to the overlay? This lays out the tabContentArea. So in this case the tab content area goes from 0 (bottom) to contentAreaTop (bottom of the toolbar). Both the overlay and tab contents are subviews of the tab content area. The overlay is normally displayed at the top of the tabContentArea so its offset (OverlayableContentsController->overlayContentsOffset_) is usually 0. The tab contents is normally displayed below any bars at the top of the browser. So for example, if the bookmark bar was pinned then the offset (OverlayableContentsController->activeContainerOffset_) would be 26.
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; Why is this not desiredHeightForCompresssion:1 ? And shouldn't it be cr_lineWidth?
https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/14689007/diff/8001/chrome/browser/ui/cocoa/br... chrome/browser/ui/cocoa/browser_window_controller_private.mm:265: minToolbarHeight = [toolbarController_ desiredHeightForCompression:0] - 1; On 2013/05/24 18:36:23, rsesek wrote: > Why is this not desiredHeightForCompresssion:1 ? Good idea, done. > And shouldn't it be cr_lineWidth? The view height is an integral value. The actual drawing height is cr_lineWidth.
I'm still a little confused like Nico. This is affecting code that runs regardless of instant extended, so how does this not break non-instant extended if kBookmarkBarOverlap is still 3? https://codereview.chromium.org/14689007/diff/23001/chrome/browser/ui/cocoa/b... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/14689007/diff/23001/chrome/browser/ui/cocoa/b... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:394: CGFloat bookmarkBarOffset = naming: bookmark_bar_offset https://codereview.chromium.org/14689007/diff/23001/chrome/browser/ui/cocoa/b... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:472: CGFloat bookmarkBarOffset = naming
LGTM for some fuzzy value of Good. I ran through the code in my head and I think I have some idea of why this works, but not entirely. I also just verified that this is a noop for non-instant extended. For reference, the offset changed here is used in -[BWC(P) updateContentOffsets], which then sets things for use in -[OverlayableContentsController layoutSubviews]. Sailesh's internal doc (which he should publish on dev.chromium.org) about the hierarchy and Z-order changes make it more apparent as to why this works.
https://codereview.chromium.org/14689007/diff/23001/chrome/browser/ui/cocoa/b... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/14689007/diff/23001/chrome/browser/ui/cocoa/b... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:394: CGFloat bookmarkBarOffset = On 2013/05/24 20:44:27, rsesek wrote: > naming: bookmark_bar_offset Done. https://codereview.chromium.org/14689007/diff/23001/chrome/browser/ui/cocoa/b... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:472: CGFloat bookmarkBarOffset = On 2013/05/24 20:44:27, rsesek wrote: > naming Done.
On 2013/05/24 22:26:18, rsesek wrote: > LGTM for some fuzzy value of Good. Woot, I'll take it!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/14689007/22002
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/14689007/22002
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/14689007/50001
Message was sent while issue was closed.
Change committed as 202354 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
