Clean-up positioning and spacing in chrome/browser/ui/views/location_bar/location_bar_view.cc

Clean-up positioning and spacing in chrome/browser/ui/views/location_bar/location_bar_view.cc

This CL systematizes location bar decoration layout. Decorations now simply need to be added to one of two lists (left and right-hand side).

The CL has been manually/visually tested in various configurations:
- With/without the action box
- With/without instant extended
- With/without touch optimized UI

The refactoring also solves a few minor bugs. 

BUG=163820, 166994, 166997, 167004
TBR=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175070

[beaudoin, 2012-12-20 03:19:18]

This is a big one, made as part of the fixit. It greatly simplifies adding and
laying out decorations in the omnibox for views.

I've designed it to match very precisely the previous behavior (although I've
fixed some bugs in the process). I also tested it extensively under windows,
trying all the code paths I could see affecting this. The refactored version
seems to match to original pixel-perfect in all cases (again, save for bug
fixes).

@sky/@pkasting: Not sure which of you is the best for a cover-to-cover review of
that patch. Please grab the "token" when you start reviewing to save some work
for the other.

[Peter Kasting, 2012-12-21 19:17:01]

LGTM; I basically scanned the layout code rather than peering super-closely,
because if you addressed all those issues, I doubt you have major fundamental
bugs.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/ev_bubble_view.cc (right):

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/ev_bubble_view.cc:9: // Set a minimum size
for elided EV bubbles 150 px.
Nit: Grammar

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/ev_bubble_view.cc:10: static const int
kMinBubbleWidth = 150;
Nit: Just declare this in the function where you use it.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:126: struct
LocationBarDecoration {
Consider defining all this code in a separate .h/.cc file.  Files with one
class/struct each are easier to understand.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:200:
ScopedVector<LocationBarDecoration>& decorations,
Instead of some non-scoped functions that each take scoped vectors, consider
something more like:

class LocationBarLayout {
  void AddDecoration(...);
  void LayoutPass1(...);
  ...
}

The caller doesn't really care about the decoration objects except to keep
passing them back to the layout functions, so this abstracts those away.

This might also allow you to make layout cheaper, if you can preserve this
object as a member of the LocationBarView, and not run some of the layout passes
after the first time.  (You might have to modify the pass functions slightly for
this.  For example, maybe you precalculate all the widths of the objects in
different passes, and then you only apply those widths on each subsequent run,
or something.)

Finally -- you can't pass a non-const ref anyway per the style guide.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:843: const double
kMaxBubbleFraction = 0.5;
Nit: Add comment about this

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:928: // Bubble
decoration.
Comment is too brief.  Why are these "bubble decorations"?

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:941: // Collapsible
regular decoration
Nit: Trailing period.  Comment is also pretty brief.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:951: int entry_width =
full_width;  // Starts with full width and update it.
Nit: Grammar.  Comment doesn't add much anyway.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:995:
location_bounds.set_width(std::min(location_needed_width,
Nit: Put this on next line so the args line up

[beaudoin, 2013-01-03 03:30:45]

Hey Peter! Happy New Year, here's a review to kick it off in style. ;)

Actually, I just need you to take another look as I've shuffled things around
according to your suggestions.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/ev_bubble_view.cc (right):

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/ev_bubble_view.cc:9: // Set a minimum size
for elided EV bubbles 150 px.
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Nit: Grammar

Done.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/ev_bubble_view.cc:10: static const int
kMinBubbleWidth = 150;
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Nit: Just declare this in the function where you use it.

Done.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:126: struct
LocationBarDecoration {
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Consider defining all this code in a separate .h/.cc file.  Files with one
> class/struct each are easier to understand.

Done. (Although it's still a local struct, but now within the much smaller
location_bar_layout.cc)

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:200:
ScopedVector<LocationBarDecoration>& decorations,
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Instead of some non-scoped functions that each take scoped vectors, consider
> something more like:
> 
> class LocationBarLayout {
>   void AddDecoration(...);
>   void LayoutPass1(...);
>   ...
> }
> 
> The caller doesn't really care about the decoration objects except to keep
> passing them back to the layout functions, so this abstracts those away.
> 
> This might also allow you to make layout cheaper, if you can preserve this
> object as a member of the LocationBarView, and not run some of the layout
passes
> after the first time.  (You might have to modify the pass functions slightly
for
> this.  For example, maybe you precalculate all the widths of the objects in
> different passes, and then you only apply those widths on each subsequent run,
> or something.)
> 
> Finally -- you can't pass a non-const ref anyway per the style guide.

New LocationBarLayout class: Done.

However I did not do the precalculation trick because the AddDecoration logic is
performed at every layout. Precalculating would either require a major change to
this approach or else some relatively involved logic in LocationBarLayout to
figure out everything that was added is the same as before and that the
available width is also the same.

An additional problem with any caching is that the views:View* in
LocationBarDecoration is weak and I'm not sure I can keep it beyond the lifetime
of LocationBarView::Layout.

Unless you feel strongly about this, I propose not doing any
caching/precomputation for now.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:843: const double
kMaxBubbleFraction = 0.5;
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Nit: Add comment about this

Done.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:928: // Bubble
decoration.
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Comment is too brief.  Why are these "bubble decorations"?

Actually, comment is not really needed, dropped it.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:941: // Collapsible
regular decoration
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Nit: Trailing period.  Comment is also pretty brief.

Comment dropped.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:951: int entry_width =
full_width;  // Starts with full width and update it.
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Nit: Grammar.  Comment doesn't add much anyway.

Comment dropped.

https://codereview.chromium.org/11648024/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:995:
location_bounds.set_width(std::min(location_needed_width,
On 2012/12/21 19:17:01, Peter Kasting wrote:
> Nit: Put this on next line so the args line up

Done.

[commit-bot: I haz the power, 2013-01-03 20:58:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11648024/14002

[commit-bot: I haz the power, 2013-01-03 20:58:14]

Failed to apply patch for chrome/chrome_browser_ui.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser_ui.gypi
  Hunk #1 succeeded at 1576 (offset -4 lines).
  Hunk #2 FAILED at 2251.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/chrome_browser_ui.gypi.rej

Patch:       chrome/chrome_browser_ui.gypi
Index: chrome/chrome_browser_ui.gypi
diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi
index
5f43439516198d35118d6d1e1dd245d32d3884da..ce18351f23346a9cf2f9e94104d8bf0ef4199799
100644
--- a/chrome/chrome_browser_ui.gypi
+++ b/chrome/chrome_browser_ui.gypi
@@ -1580,6 +1580,8 @@
         'browser/ui/views/location_bar/keyword_hint_view.h',
         'browser/ui/views/location_bar/location_bar_decoration_view.cc',
         'browser/ui/views/location_bar/location_bar_decoration_view.h',
+        'browser/ui/views/location_bar/location_bar_layout.cc',
+        'browser/ui/views/location_bar/location_bar_layout.h',
         'browser/ui/views/location_bar/location_bar_view.cc',
         'browser/ui/views/location_bar/location_bar_view.h',
         'browser/ui/views/location_bar/location_icon_view.cc',
@@ -2249,7 +2251,7 @@
           'sources!': [
             'browser/ui/hung_plugin_tab_helper.cc',
             'browser/ui/hung_plugin_tab_helper.h',
-          ], 
+          ],
         }],
         ['safe_browsing==1', {
           'defines': [

[commit-bot: I haz the power, 2013-01-03 21:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11648024/11003

[commit-bot: I haz the power, 2013-01-03 21:48:38]

Presubmit check for 11648024-11003 failed and returned exit status 1.

/b/commit-queue/workdir/chromium/chrome/browser/ui/views/location_bar/location_bar_layout.cc:
Must not have executable bit set
/b/commit-queue/workdir/chromium/chrome/browser/ui/views/location_bar/location_bar_layout.h:
Must not have executable bit set
Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.2s to calculate.

[beaudoin, 2013-01-03 22:10:02]

@ben : TBR for files added to chrome_browser_ui.gypi

[commit-bot: I haz the power, 2013-01-03 22:10:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11648024/9008

[commit-bot: I haz the power, 2013-01-04 00:53:16]

Change committed as 175070