views/location_bar: Eliminate TouchableLocationBarView.

views/location_bar: Eliminate TouchableLocationBarView.

Instead of inheriting from it move the functions into LocationBarView.

BUG=260446
R=pkasting@chromium.org

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

[Peter Kasting, 2013-08-06 20:13:48]

Side note.  I posted on the bug with more about why we want to do this work. 
It's probably not necessary to try and include it in this patch -- better to do
things one step at a time -- but I'd like that work to get done (or conclusively
evaluated as a bad idea) before the bug is closed.

OK, reviewing.

[Peter Kasting, 2013-08-06 20:32:42]

LGTM

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

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1096: return
ui::GetDisplayLayout() == ui::LAYOUT_TOUCH ? GetItemPadding() / 2 : 0;
Nit: Parens around binary subexpr

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:143: // Call this from
the constructor (or during early initialization).
This comment is misleading, it sounds as if it refers to LocationBarView's
constructor.  Clarify.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:368: // right of the
image for this view.
Nit: Also need to update this comment.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_icon_view.cc:39: if (show)
Nit: Simpler:

  SetTooltipText(show ?
      l10n_util::GetStringUTF16(IDS_TOOLTIP_LOCATION_ICON) : string16());

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/mic_search_view.cc (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/mic_search_view.cc:15:
MicSearchView::MicSearchView(views::ButtonListener* button_listener)
Nit: This class is now so simple that perhaps it makes sense to just nuke it and
have LocationBarView just create an ImageView and call all the stuff this calls
in its constructor.

[tfarina, 2013-08-07 18:42:52]

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

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1096: return
ui::GetDisplayLayout() == ui::LAYOUT_TOUCH ? GetItemPadding() / 2 : 0;
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: Parens around binary subexpr

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1096: return
ui::GetDisplayLayout() == ui::LAYOUT_TOUCH ? GetItemPadding() / 2 : 0;
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: Parens around binary subexpr

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:143: // Call this from
the constructor (or during early initialization).
On 2013/08/06 20:32:42, Peter Kasting wrote:
> This comment is misleading, it sounds as if it refers to LocationBarView's
> constructor.  Clarify.

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:368: // right of the
image for this view.
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: Also need to update this comment.

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/mic_search_view.cc (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/mic_search_view.cc:15:
MicSearchView::MicSearchView(views::ButtonListener* button_listener)
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: This class is now so simple that perhaps it makes sense to just nuke it
and
> have LocationBarView just create an ImageView and call all the stuff this
calls
> in its constructor.

Done.

https://codereview.chromium.org/21696003/diff/18001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/mic_search_view.h (right):

https://codereview.chromium.org/21696003/diff/18001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/mic_search_view.h:10: class MicSearchView :
public views::ImageButton {
I forgot to remove the files, I'll do in patch set 4 when I get back home.

[tfarina, 2013-08-07 18:42:53]

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

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1096: return
ui::GetDisplayLayout() == ui::LAYOUT_TOUCH ? GetItemPadding() / 2 : 0;
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: Parens around binary subexpr

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1096: return
ui::GetDisplayLayout() == ui::LAYOUT_TOUCH ? GetItemPadding() / 2 : 0;
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: Parens around binary subexpr

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:143: // Call this from
the constructor (or during early initialization).
On 2013/08/06 20:32:42, Peter Kasting wrote:
> This comment is misleading, it sounds as if it refers to LocationBarView's
> constructor.  Clarify.

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:368: // right of the
image for this view.
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: Also need to update this comment.

Done.

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/mic_search_view.cc (right):

https://codereview.chromium.org/21696003/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/mic_search_view.cc:15:
MicSearchView::MicSearchView(views::ButtonListener* button_listener)
On 2013/08/06 20:32:42, Peter Kasting wrote:
> Nit: This class is now so simple that perhaps it makes sense to just nuke it
and
> have LocationBarView just create an ImageView and call all the stuff this
calls
> in its constructor.

Done.

https://codereview.chromium.org/21696003/diff/18001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/mic_search_view.h (right):

https://codereview.chromium.org/21696003/diff/18001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/mic_search_view.h:10: class MicSearchView :
public views::ImageButton {
I forgot to remove the files, I'll do in patch set 4 when I get back home.

[tfarina, 2013-08-07 22:29:38]

mic_search_view.* files deleted.

[Peter Kasting, 2013-08-09 20:57:27]

LGTM with comments

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1107: return
(ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ? GetItemPadding() / 2
Nit: Break after '?', not before ':'.

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:144: // touchable.
This is still confusing.  This has nothing to do with LocationBarView being
touchable, it's about making child views touchable.  And it should be called
unconditionally for child views, not only if touch is enabled (it makes that
determination internally).

How about:

Uses GetBuiltInHorizontalPaddingForChildViews() to optionally add additional
padding (via an empty border) to |view|.  This should be called during creation
on all child views which are potentially touchable so that when touch is enabled
they will have sufficient padding.

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:145: static void
InitTouchableLocationBarView(views::View* view);
Nit: Perhaps for clarity, this should end "...ChildView" instead of "View".

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:370: // make it more
touchable.
Nit: This comment likewise has problems.  We're not increasing the area of the
location bar.  How about:

...This is nonzero when touch UI is enabled so as to space out child views for
easier targeting.  See InitTouchableLocationBarChildView().

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_icon_view.cc:40: :
base::string16());
Nit: Break after '?' (as my comment had it), not before ':'.

https://codereview.chromium.org/21696003/diff/35001/chrome/utility/importer/b...
File chrome/utility/importer/bookmark_html_reader.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/utility/importer/b...
chrome/utility/importer/bookmark_html_reader.cc:120: if (StartsWithASCII(line,
"<HR>", false))
This change should not be in this CL.

[tfarina, 2013-08-10 02:13:40]

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1107: return
(ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ? GetItemPadding() / 2
On 2013/08/09 20:57:28, Peter Kasting wrote:
> Nit: Break after '?', not before ':'.
this is clang-format not me.

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:144: // touchable.
On 2013/08/09 20:57:28, Peter Kasting wrote:
> This is still confusing.  This has nothing to do with LocationBarView being
> touchable, it's about making child views touchable.  And it should be called
> unconditionally for child views, not only if touch is enabled (it makes that
> determination internally).
> 
> How about:
> 
> Uses GetBuiltInHorizontalPaddingForChildViews() to optionally add additional
> padding (via an empty border) to |view|.  This should be called during
creation
> on all child views which are potentially touchable so that when touch is
enabled
> they will have sufficient padding.

Done.

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:145: static void
InitTouchableLocationBarView(views::View* view);
On 2013/08/09 20:57:28, Peter Kasting wrote:
> Nit: Perhaps for clarity, this should end "...ChildView" instead of "View".

Done.

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:370: // make it more
touchable.
On 2013/08/09 20:57:28, Peter Kasting wrote:
> Nit: This comment likewise has problems.  We're not increasing the area of the
> location bar.  How about:
> 
> ...This is nonzero when touch UI is enabled so as to space out child views for
> easier targeting.  See InitTouchableLocationBarChildView().

Done.

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_icon_view.cc:40: :
base::string16());
On 2013/08/09 20:57:28, Peter Kasting wrote:
> Nit: Break after '?' (as my comment had it), not before ':'.
this is clang-format not me.

https://codereview.chromium.org/21696003/diff/35001/chrome/utility/importer/b...
File chrome/utility/importer/bookmark_html_reader.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/utility/importer/b...
chrome/utility/importer/bookmark_html_reader.cc:120: if (StartsWithASCII(line,
"<HR>", false))
On 2013/08/09 20:57:28, Peter Kasting wrote:
> This change should not be in this CL.
Ops, this shouldn't be here, this was for another patch I was working one.
Thanks for spotting it!

[Peter Kasting, 2013-08-10 07:27:21]

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1107: return
(ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ? GetItemPadding() / 2
On 2013/08/10 02:13:40, tfarina wrote:
> On 2013/08/09 20:57:28, Peter Kasting wrote:
> > Nit: Break after '?', not before ':'.
> this is clang-format not me.

I don't care.  clang-format is wrong.  Do not format code in Chromium this way. 
(Both files)

[tfarina, 2013-08-10 19:11:31]

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/21696003/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1107: return
(ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ? GetItemPadding() / 2
On 2013/08/10 07:27:21, Peter Kasting wrote:
> On 2013/08/10 02:13:40, tfarina wrote:
> > On 2013/08/09 20:57:28, Peter Kasting wrote:
> > > Nit: Break after '?', not before ':'.
> > this is clang-format not me.
> 
> I don't care.  clang-format is wrong.  Do not format code in Chromium this
way. 
> (Both files)

Done.

[commit-bot: I haz the power, 2013-08-10 22:14:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/21696003/68001

[commit-bot: I haz the power, 2013-08-11 05:16:28]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-11 14:20:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/21696003/68001

[commit-bot: I haz the power, 2013-08-12 05:12:44]

Change committed as 216928