Enlarge size of touch target for several location bar buttons without affecting visual layout.

Enlarge size of touch target for several location bar buttons without affecting visual layout.

BUG=129751
TEST=This issue affects layout code for the location bar. It should not change the visible layout at all, but in touch layout it should allow you to click just outside of the icons for many of the buttons (e.g. the bookmark star) and get a "hit" on the button. This change needs to be tested with the following different types of buttons in the location bar, in both touch (e.g. Win8 Metro) and non-touch layouts, to make sure the layout looks right: "Chrome to Mobile" button, content settings button (e.g. blocked JavaScript, blocked plug-ins), page action buttons (from extensions), the location icon (far left of the omnibox, normally) and the bookmark star (far right).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142124

[Jói, 2012-06-11 13:40:04]

Peter, please take a look, this is per discussion in the bug on how to get a
quick fix in ASAP, while the longer-term better fix will be to implement tap
fuzzing (issue 129794).

Cheers,
Jói

[Jói, 2012-06-12 21:09:17]

pkasting:  Whoops, looks like I forgot to add you as a reviewer when I
sent this out yesterday.  Please take a look, see below.

On Mon, Jun 11, 2012 at 1:40 PM,  <joi@chromium.org> wrote:
> Reviewers: ,
>
> Message:
> Peter, please take a look, this is per discussion in the bug on how to get a
> quick fix in ASAP, while the longer-term better fix will be to implement tap
> fuzzing (issue 129794).
>
> Cheers,
> Jói
>
>
> Description:
> Enlarge size of touch target for bookmark star without affecting visual
> layout.
>
> BUG=129751
>
>
> Please review this at http://codereview.chromium.org/10519008/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/ui/views/location_bar/chrome_to_mobile_view.h
>  M chrome/browser/ui/views/location_bar/chrome_to_mobile_view.cc
>  M chrome/browser/ui/views/location_bar/content_setting_image_view.h
>  M chrome/browser/ui/views/location_bar/content_setting_image_view.cc
>  M chrome/browser/ui/views/location_bar/location_bar_view.cc
>  M chrome/browser/ui/views/location_bar/location_icon_view.h
>  M chrome/browser/ui/views/location_bar/location_icon_view.cc
>  M chrome/browser/ui/views/location_bar/page_action_with_badge_view.h
>  M chrome/browser/ui/views/location_bar/page_action_with_badge_view.cc
>  M chrome/browser/ui/views/location_bar/star_view.h
>  M chrome/browser/ui/views/location_bar/star_view.cc
>  A chrome/browser/ui/views/location_bar/touchable_location_bar_view.h
>  A chrome/browser/ui/views/location_bar/touchable_location_bar_view.cc
>  M chrome/chrome_browser.gypi
>
>

[Peter Kasting, 2012-06-12 22:03:52]

http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/touchable_location_bar_view.h (right):

http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:16: class
TouchableLocationBarViewBase {
I don't understand what splitting this class in two buys you.

It seems like making this class be the templated TouchableLocationBarView (and
rolling the bits of that class into this one) would work fine.

http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:27: int
touch_horizontal_padding() const;
Nit: Should be camel-cased since implementation is not a simple inline getter

[Jói, 2012-06-12 23:23:44]

The reason for the base class is to leave code that doesn't need the
template parameter as non-templated code, which AFAIUI reduces the load on
the compiler/linker.  It's a pretty common pattern I think.

Cheers,
Jói
On Jun 12, 2012 10:03 PM, <pkasting@chromium.org> wrote:

>
> http://codereview.chromium.**org/10519008/diff/2001/chrome/**
>
browser/ui/views/location_bar/**touchable_location_bar_view.h<http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/location_bar/touchable_location_bar_view.h>
> File chrome/browser/ui/views/**location_bar/touchable_**
> location_bar_view.h
> (right):
>
> http://codereview.chromium.**org/10519008/diff/2001/chrome/**
>
browser/ui/views/location_bar/**touchable_location_bar_view.h#**newcode16<http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/location_bar/touchable_location_bar_view.h#newcode16>
> chrome/browser/ui/views/**location_bar/touchable_**location_bar_view.h:16:
> class TouchableLocationBarViewBase {
> I don't understand what splitting this class in two buys you.
>
> It seems like making this class be the templated
> TouchableLocationBarView (and rolling the bits of that class into this
> one) would work fine.
>
> http://codereview.chromium.**org/10519008/diff/2001/chrome/**
>
browser/ui/views/location_bar/**touchable_location_bar_view.h#**newcode27<http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/location_bar/touchable_location_bar_view.h#newcode27>
> chrome/browser/ui/views/**location_bar/touchable_**location_bar_view.h:27:
> int touch_horizontal_padding() const;
> Nit: Should be camel-cased since implementation is not a simple inline
> getter
>
>
http://codereview.chromium.**org/10519008/<http://codereview.chromium.org/105...
>

[Peter Kasting, 2012-06-12 23:27:27]

On 2012/06/12 23:23:44, Jói wrote:
> The reason for the base class is to leave code that doesn't need the
> template parameter as non-templated code, which AFAIUI reduces the load on
> the compiler/linker.  It's a pretty common pattern I think.

This code is so simple anyway that I think we should go ahead and optimize for
simplicity and readability.  I'd just make it one class.

LGTM otherwise.

[Jói, 2012-06-13 11:29:38]

Thanks, will commit once I've done a quick sanity test on the new version.

Cheers,
Jói

http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/touchable_location_bar_view.h (right):

http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:16: class
TouchableLocationBarViewBase {
On 2012/06/12 22:03:52, Peter Kasting wrote:
> I don't understand what splitting this class in two buys you.
> 
> It seems like making this class be the templated TouchableLocationBarView (and
> rolling the bits of that class into this one) would work fine.

Rolled it into one class as requested.  This simplified things down to two
methods.

http://codereview.chromium.org/10519008/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:27: int
touch_horizontal_padding() const;
On 2012/06/12 22:03:52, Peter Kasting wrote:
> Nit: Should be camel-cased since implementation is not a simple inline getter

This method went away with the simplification.

[Peter Kasting, 2012-06-13 17:09:16]

Actually I just realized that you can probably avoid templates altogether by
using composition rather than inheritance.

Basically, make the constructor take a View* and store that as a member for you
to call set_border() on.  Then classes using this can have it as a member and
pass |this| to it, instead of inheriting from it.

This should let you move the function definitions out of the header into a .cc
file, avoid templates, and be more compliant with the Google style guide (which
frowns on multiple inheritance) all at the same time.

Sorry I didn't think of this sooner...

http://codereview.chromium.org/10519008/diff/5031/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/touchable_location_bar_view.h (right):

http://codereview.chromium.org/10519008/diff/5031/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:43: void
TouchableLocationBarView<Concrete>::Init() {
Nit: Order definitions the same as the declarations.

[Jói, 2012-06-13 17:18:01]

In my opinion, using a mix-in is more elegant since there is an is-a
relationship, and if I switch to composition that means adding an identical
method to several classes (i.e. GetBuiltInHorizontalPadding) without them
sharing a common base that ensures they are all updated at the same time should
the method ever change.

We do have multiple inheritance in a few places in the Chrome codebase, although
I agree as a general tool it's usually to be avoided, and the style guide
doesn't really allow it.

Mix-ins (that take the concrete class as a template parameter and downcast to it
to add functionality) are, I think, one of the forms of multiple inheritance we
should accept in certain cases.  However, if you really prefer, I will switch to
composition.  Let me know.

Cheers,
Jói

http://codereview.chromium.org/10519008/diff/5031/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/touchable_location_bar_view.h (right):

http://codereview.chromium.org/10519008/diff/5031/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:43: void
TouchableLocationBarView<Concrete>::Init() {
On 2012/06/13 17:09:16, Peter Kasting wrote:
> Nit: Order definitions the same as the declarations.

Done.

[Peter Kasting, 2012-06-13 17:46:47]

On 2012/06/13 17:18:01, Jói wrote:
> In my opinion, using a mix-in is more elegant since there is an is-a
> relationship, and if I switch to composition that means adding an identical
> method to several classes (i.e. GetBuiltInHorizontalPadding) without them
> sharing a common base that ensures they are all updated at the same time
should
> the method ever change.

Why would you need to add this method to those classes?  Can't it stay as a
method on this class?

I agree that if you had to move it out that would be gross, but I'm missing why
you'd need to.

[Jói, 2012-06-13 17:48:36]

It is called by LocationBarView in several places; since we're talking
composition, either I need to add the method to all of the classes, or
I need to add an accessor to retrieve the TouchableLocationBarView
object that you then call the method on.  Both are grosser (IMHO) than
doing this as a mix-in :)

Cheers,
Jói


On Wed, Jun 13, 2012 at 5:46 PM,  <pkasting@chromium.org> wrote:
> On 2012/06/13 17:18:01, Jói wrote:
>>
>> In my opinion, using a mix-in is more elegant since there is an is-a
>> relationship, and if I switch to composition that means adding an
>> identical
>> method to several classes (i.e. GetBuiltInHorizontalPadding) without them
>> sharing a common base that ensures they are all updated at the same time
>
> should
>>
>> the method ever change.
>
>
> Why would you need to add this method to those classes?  Can't it stay as a
> method on this class?
>
> I agree that if you had to move it out that would be gross, but I'm missing
> why
> you'd need to.
>
> http://codereview.chromium.org/10519008/

[Peter Kasting, 2012-06-13 18:15:36]

On 2012/06/13 17:48:36, Jói wrote:
> It is called by LocationBarView in several places; since we're talking
> composition, either I need to add the method to all of the classes, or
> I need to add an accessor to retrieve the TouchableLocationBarView
> object that you then call the method on.  Both are grosser (IMHO) than
> doing this as a mix-in :)

Or you could add a second class by inheritance to provide the accessor :)  J/K

Anyway, I understand now.  You're right that inheritance is the simplest, and I
think I agree that that's the route I'd normally go.

Reading the style guide again, however, it's really firm about the "no multiple
inheritance" point, and I'm uncomfortable violating it (though I know there are
many places in Chrome that violate this; it's probably the most-broken style
rule in our codebase).  I guess my reluctant conclusion is that we should
probably do composition and add a call-through method for
GetBuiltInHorizontalPadding(), unless we want to start a chromium-dev thread
arguing that our style guide should relax this multiple inheritance rule.

[Jói, 2012-06-13 20:52:38]

Yeah I think you're right.  I don't think there will be consensus
about changing the style guide, and in any case it's worth a fair bit
simply to keep it as close as possible to the Google style guide.

What I think I'll do is this; let me know if you concur:

a) Add a pure virtual interface that has the TouchableLocationBarView
name, and keep the is-a relationship and the inheritance pattern we
have today (but this approach falls within the style guide).  This
would have just the GetBuiltInHorizontalPadding method in it.

b) Either add a TouchableLocationBarViewImpl class that is intended
for composition, or just protected static methods in the
TouchableLocationBarView class (one of them would be a pure static for
GetBuiltInHorizontalPadding, the other would take a View* and call
set_border on it to implement Init.  I'm leaning towards the static
methods; we have related static methods in various pure virtual
interface classes in several locations in the code base and I think
there's nothing wrong with that pattern.

Cheers,
Jói


On Wed, Jun 13, 2012 at 6:15 PM,  <pkasting@chromium.org> wrote:
> On 2012/06/13 17:48:36, Jói wrote:
>>
>> It is called by LocationBarView in several places; since we're talking
>> composition, either I need to add the method to all of the classes, or
>> I need to add an accessor to retrieve the TouchableLocationBarView
>> object that you then call the method on.  Both are grosser (IMHO) than
>> doing this as a mix-in :)
>
>
> Or you could add a second class by inheritance to provide the accessor :)
>  J/K
>
> Anyway, I understand now.  You're right that inheritance is the simplest,
> and I
> think I agree that that's the route I'd normally go.
>
> Reading the style guide again, however, it's really firm about the "no
> multiple
> inheritance" point, and I'm uncomfortable violating it (though I know there
> are
> many places in Chrome that violate this; it's probably the most-broken style
> rule in our codebase).  I guess my reluctant conclusion is that we should
> probably do composition and add a call-through method for
> GetBuiltInHorizontalPadding(), unless we want to start a chromium-dev thread
> arguing that our style guide should relax this multiple inheritance rule.
>
> http://codereview.chromium.org/10519008/

[Jói, 2012-06-13 21:09:33]

I implemented the pure virtual interface / static implementation
methods approach, please see patch set 7 at
http://codereview.chromium.org/10519008.

Cheers,
Jói


On Wed, Jun 13, 2012 at 8:51 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Yeah I think you're right.  I don't think there will be consensus
> about changing the style guide, and in any case it's worth a fair bit
> simply to keep it as close as possible to the Google style guide.
>
> What I think I'll do is this; let me know if you concur:
>
> a) Add a pure virtual interface that has the TouchableLocationBarView
> name, and keep the is-a relationship and the inheritance pattern we
> have today (but this approach falls within the style guide).  This
> would have just the GetBuiltInHorizontalPadding method in it.
>
> b) Either add a TouchableLocationBarViewImpl class that is intended
> for composition, or just protected static methods in the
> TouchableLocationBarView class (one of them would be a pure static for
> GetBuiltInHorizontalPadding, the other would take a View* and call
> set_border on it to implement Init.  I'm leaning towards the static
> methods; we have related static methods in various pure virtual
> interface classes in several locations in the code base and I think
> there's nothing wrong with that pattern.
>
> Cheers,
> Jói
>
>
> On Wed, Jun 13, 2012 at 6:15 PM,  <pkasting@chromium.org> wrote:
>> On 2012/06/13 17:48:36, Jói wrote:
>>>
>>> It is called by LocationBarView in several places; since we're talking
>>> composition, either I need to add the method to all of the classes, or
>>> I need to add an accessor to retrieve the TouchableLocationBarView
>>> object that you then call the method on.  Both are grosser (IMHO) than
>>> doing this as a mix-in :)
>>
>>
>> Or you could add a second class by inheritance to provide the accessor :)
>>  J/K
>>
>> Anyway, I understand now.  You're right that inheritance is the simplest,
>> and I
>> think I agree that that's the route I'd normally go.
>>
>> Reading the style guide again, however, it's really firm about the "no
>> multiple
>> inheritance" point, and I'm uncomfortable violating it (though I know there
>> are
>> many places in Chrome that violate this; it's probably the most-broken style
>> rule in our codebase).  I guess my reluctant conclusion is that we should
>> probably do composition and add a call-through method for
>> GetBuiltInHorizontalPadding(), unless we want to start a chromium-dev thread
>> arguing that our style guide should relax this multiple inheritance rule.
>>
>> http://codereview.chromium.org/10519008/

[Jói, 2012-06-14 00:12:03]

pkasting: Gentle ping, hoping to land this tomorrow morning my time.

Cheers,
Jói
On Jun 13, 2012 9:08 PM, "Jói Sigurðsson" <joi@chromium.org> wrote:

> I implemented the pure virtual interface / static implementation
> methods approach, please see patch set 7 at
> http://codereview.chromium.org/10519008.
>
> Cheers,
> Jói
>
>
> On Wed, Jun 13, 2012 at 8:51 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > Yeah I think you're right.  I don't think there will be consensus
> > about changing the style guide, and in any case it's worth a fair bit
> > simply to keep it as close as possible to the Google style guide.
> >
> > What I think I'll do is this; let me know if you concur:
> >
> > a) Add a pure virtual interface that has the TouchableLocationBarView
> > name, and keep the is-a relationship and the inheritance pattern we
> > have today (but this approach falls within the style guide).  This
> > would have just the GetBuiltInHorizontalPadding method in it.
> >
> > b) Either add a TouchableLocationBarViewImpl class that is intended
> > for composition, or just protected static methods in the
> > TouchableLocationBarView class (one of them would be a pure static for
> > GetBuiltInHorizontalPadding, the other would take a View* and call
> > set_border on it to implement Init.  I'm leaning towards the static
> > methods; we have related static methods in various pure virtual
> > interface classes in several locations in the code base and I think
> > there's nothing wrong with that pattern.
> >
> > Cheers,
> > Jói
> >
> >
> > On Wed, Jun 13, 2012 at 6:15 PM,  <pkasting@chromium.org> wrote:
> >> On 2012/06/13 17:48:36, Jói wrote:
> >>>
> >>> It is called by LocationBarView in several places; since we're talking
> >>> composition, either I need to add the method to all of the classes, or
> >>> I need to add an accessor to retrieve the TouchableLocationBarView
> >>> object that you then call the method on.  Both are grosser (IMHO) than
> >>> doing this as a mix-in :)
> >>
> >>
> >> Or you could add a second class by inheritance to provide the accessor
> :)
> >>  J/K
> >>
> >> Anyway, I understand now.  You're right that inheritance is the
> simplest,
> >> and I
> >> think I agree that that's the route I'd normally go.
> >>
> >> Reading the style guide again, however, it's really firm about the "no
> >> multiple
> >> inheritance" point, and I'm uncomfortable violating it (though I know
> there
> >> are
> >> many places in Chrome that violate this; it's probably the most-broken
> style
> >> rule in our codebase).  I guess my reluctant conclusion is that we
> should
> >> probably do composition and add a call-through method for
> >> GetBuiltInHorizontalPadding(), unless we want to start a chromium-dev
> thread
> >> arguing that our style guide should relax this multiple inheritance
> rule.
> >>
> >> http://codereview.chromium.org/10519008/
>

[Peter Kasting, 2012-06-14 00:36:04]

LGTM

http://codereview.chromium.org/10519008/diff/4016/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/touchable_location_bar_view.h (right):

http://codereview.chromium.org/10519008/diff/4016/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:18: // To
make your location bar View named FooBlat extend itself into the
Nit: Just remove "named FooBlat"

http://codereview.chromium.org/10519008/diff/4016/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:20: //
TouchableLocationBarView interface, implements its
Nit: implements->implement

[commit-bot: I haz the power, 2012-06-14 10:04:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10519008/16016

[Jói, 2012-06-14 10:05:02]

Thanks, fixed those and submitted to CQ.

On Thu, Jun 14, 2012 at 12:36 AM,  <pkasting@chromium.org> wrote:
> LGTM
>
>
>
http://codereview.chromium.org/10519008/diff/4016/chrome/browser/ui/views/loc...
> File chrome/browser/ui/views/location_bar/touchable_location_bar_view.h
> (right):
>
>
http://codereview.chromium.org/10519008/diff/4016/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:18:
> // To make your location bar View named FooBlat extend itself into the
> Nit: Just remove "named FooBlat"
>
>
http://codereview.chromium.org/10519008/diff/4016/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/touchable_location_bar_view.h:20:
> // TouchableLocationBarView interface, implements its
> Nit: implements->implement
>
> http://codereview.chromium.org/10519008/

[commit-bot: I haz the power, 2012-06-14 11:07:03]

Change committed as 142124