Remove touch layout

Remove touch layout
Re removed touch resources, but didn't remove the code that changes the layout. This CL removed all touch layouts.

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

[oshima, 2014-04-22 20:22:33]

girard@, can you take a first look?

https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc
File ui/base/layout.cc (left):

https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc#oldcode60
ui/base/layout.cc:60: return base::win::IsMetroProcess() &&
ui::IsTouchDevicePresent();
This is always false on Win because IsTouchDevicePresent always return false on
Win.

[girard, 2014-04-22 20:29:29]

I'm not sure if we use touch layout anymore... at least, I didn't think we were.

Killing all this code is okay by me.

https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc
File ui/base/layout.cc (left):

https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc#oldcode60
ui/base/layout.cc:60: return base::win::IsMetroProcess() &&
ui::IsTouchDevicePresent();
On 2014/04/22 20:22:34, oshima wrote:
> This is always false on Win because IsTouchDevicePresent always return false
on
> Win.

IsTouchDevicePresent returns the appropriate value on Windows. 
See
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...

[girard, 2014-04-22 20:35:33]

LGTM

[oshima, 2014-04-22 20:51:59]

https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc
File ui/base/layout.cc (left):

https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc#oldcode60
ui/base/layout.cc:60: return base::win::IsMetroProcess() &&
ui::IsTouchDevicePresent();
On 2014/04/22 20:29:30, girard wrote:
> On 2014/04/22 20:22:34, oshima wrote:
> > This is always false on Win because IsTouchDevicePresent always return false
> on
> > Win.
> 
> IsTouchDevicePresent returns the appropriate value on Windows. 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...

Hmm, I wonder why my search didn't hit win impl (it hit only default and x11
impl).
Does this mean touch UI has been used for win8 tablet?

[ananta, 2014-04-23 02:06:51]

On 2014/04/22 20:51:59, oshima wrote:
> https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc
> File ui/base/layout.cc (left):
> 
>
https://codereview.chromium.org/247193002/diff/40001/ui/base/layout.cc#oldcode60
> ui/base/layout.cc:60: return base::win::IsMetroProcess() &&
> ui::IsTouchDevicePresent();
> On 2014/04/22 20:29:30, girard wrote:
> > On 2014/04/22 20:22:34, oshima wrote:
> > > This is always false on Win because IsTouchDevicePresent always return
false
> > on
> > > Win.
> > 
> > IsTouchDevicePresent returns the appropriate value on Windows. 
> > See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...
> 
> Hmm, I wonder why my search didn't hit win impl (it hit only default and x11
> impl).
> Does this mean touch UI has been used for win8 tablet?

The LAYOUT_TOUCH enum is used to determine tab spacing, menus, etc. This CL as
is will render Chrome
unusable on a touch only device. A middle ground would be ideal.

[oshima, 2014-04-23 02:06:55]

Touch optimized UI has been deprecated long time ago,
assets hasn't been updated for long time, which has been deleted early this
year, so we can't really get back to "full supported" state.

That's being said, we may still want to keep some aspect of touch UI for win8
tablet device (although I'm not sure if ash is good for tablet form factor
anyway). I'll hold this for now and will discuss with cpu when he's back.

[oshima, 2014-04-24 00:50:13]

On 2014/04/23 02:06:55, oshima wrote:
> Touch optimized UI has been deprecated long time ago,
> assets hasn't been updated for long time, which has been deleted early this
> year, so we can't really get back to "full supported" state.
> 
> That's being said, we may still want to keep some aspect of touch UI for win8
> tablet device (although I'm not sure if ash is good for tablet form factor
> anyway). I'll hold this for now and will discuss with cpu when he's back.

looks like IsMetroProcess always returns false, so TOUCH layout must have not
been on by default.
sky@, can you do OWNERS review while ananta@ is confirming?

[oshima, 2014-04-24 01:40:46]

On 2014/04/24 00:50:13, oshima wrote:
> On 2014/04/23 02:06:55, oshima wrote:
> > Touch optimized UI has been deprecated long time ago,
> > assets hasn't been updated for long time, which has been deleted early this
> > year, so we can't really get back to "full supported" state.
> > 
> > That's being said, we may still want to keep some aspect of touch UI for
win8
> > tablet device (although I'm not sure if ash is good for tablet form factor
> > anyway). I'll hold this for now and will discuss with cpu when he's back.
> 
> looks like IsMetroProcess always returns false, so TOUCH layout must have not
> been on by default.
> sky@, can you do OWNERS review while ananta@ is confirming?

I confirmed that we're not using touch layout on win8 + touch. (tested on yoga)

[sky, 2014-04-24 03:29:01]

I suspect you can nuke a bunch of includes of layout.h now too.

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:558: int
BrowserView::GetOTRIconResourceID() const {
Can you remove this function and replace with IDR_OTR_ICON.

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1069: int
LocationBarView::GetBuiltInHorizontalPaddingForChildViews() {
Can we remove this too?

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:689: kLeftPadding, kTopPadding,
kRightPadding, kBottomPadding);
nit: I think you can fit this all on one line now.

[cpu_(ooo_6.6-7.5), 2014-04-24 03:38:21]

the grd changes can be TBR

[girard, 2014-04-24 12:58:25]

On 2014/04/24 00:50:13, oshima wrote:
> On 2014/04/23 02:06:55, oshima wrote:
> > Touch optimized UI has been deprecated long time ago,
> > assets hasn't been updated for long time, which has been deleted early this
> > year, so we can't really get back to "full supported" state.
> > 
> > That's being said, we may still want to keep some aspect of touch UI for
win8
> > tablet device (although I'm not sure if ash is good for tablet form factor
> > anyway). I'll hold this for now and will discuss with cpu when he's back.
> 
> looks like IsMetroProcess always returns false, so TOUCH layout must have not
> been on by default.
> sky@, can you do OWNERS review while ananta@ is confirming?

IsMetroProcess should work properly (return true or false as appropriate). 

See the code at:
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/metro.cc&...

[oshima, 2014-04-24 13:52:39]

On 2014/04/24 12:58:25, girard wrote:
> On 2014/04/24 00:50:13, oshima wrote:
> > On 2014/04/23 02:06:55, oshima wrote:
> > > Touch optimized UI has been deprecated long time ago,
> > > assets hasn't been updated for long time, which has been deleted early
this
> > > year, so we can't really get back to "full supported" state.
> > > 
> > > That's being said, we may still want to keep some aspect of touch UI for
> win8
> > > tablet device (although I'm not sure if ash is good for tablet form factor
> > > anyway). I'll hold this for now and will discuss with cpu when he's back.
> > 
> > looks like IsMetroProcess always returns false, so TOUCH layout must have
not
> > been on by default.
> > sky@, can you do OWNERS review while ananta@ is confirming?
> 
> IsMetroProcess should work properly (return true or false as appropriate). 
> 
> See the code at:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/metro.cc&...

My understanding is that this return true for viewer but false for browser
process,
so I should have said "always returns false for browser process".  sky@,
ananta@,
am I wrong ?

[sky, 2014-04-24 16:23:34]

I believe you are right. That code only works in the viewer side,
which none of the code Oshima is changing now runs in the viewer
process.

  -Scott

On Thu, Apr 24, 2014 at 6:52 AM,  <oshima@chromium.org> wrote:
> On 2014/04/24 12:58:25, girard wrote:
>>
>> On 2014/04/24 00:50:13, oshima wrote:
>> > On 2014/04/23 02:06:55, oshima wrote:
>> > > Touch optimized UI has been deprecated long time ago,
>> > > assets hasn't been updated for long time, which has been deleted early
>
> this
>>
>> > > year, so we can't really get back to "full supported" state.
>> > >
>> > > That's being said, we may still want to keep some aspect of touch UI
>> > > for
>> win8
>> > > tablet device (although I'm not sure if ash is good for tablet form
>> > > factor
>> > > anyway). I'll hold this for now and will discuss with cpu when he's
>> > > back.
>> >
>> > looks like IsMetroProcess always returns false, so TOUCH layout must
>> > have
>
> not
>>
>> > been on by default.
>> > sky@, can you do OWNERS review while ananta@ is confirming?
>
>
>> IsMetroProcess should work properly (return true or false as appropriate).
>
>
>> See the code at:
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/metro.cc&...
>
> My understanding is that this return true for viewer but false for browser
> process,
> so I should have said "always returns false for browser process".  sky@,
> ananta@,
> am I wrong ?
>
>
> https://codereview.chromium.org/247193002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-04-25 14:16:25]

PTAL

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:558: int
BrowserView::GetOTRIconResourceID() const {
On 2014/04/24 03:29:02, sky wrote:
> Can you remove this function and replace with IDR_OTR_ICON.

Done.

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1069: int
LocationBarView::GetBuiltInHorizontalPaddingForChildViews() {
On 2014/04/24 03:29:02, sky wrote:
> Can we remove this too?

Done.

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/247193002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab.cc:689: kLeftPadding, kTopPadding,
kRightPadding, kBottomPadding);
On 2014/04/24 03:29:02, sky wrote:
> nit: I think you can fit this all on one line now.

Done.

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:826: 0,
I made separate CL to remove this parameter. Let me know if you want to keep the
parameter and I'll update the style.

https://codereview.chromium.org/250793004/

[sky, 2014-04-25 14:22:15]

LGTM

[oshima, 2014-04-25 14:28:59]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-04-25 21:00:14]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-04-25 21:00:20]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:02:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/247193002/100001

[commit-bot: I haz the power, 2014-04-26 00:39:48]

Change committed as 266305

[Dan Beam, 2014-04-26 03:43:32]

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:67: const int kTabIconSize =
gfx::kFaviconSize;
^ this added a static initializer, but I have no idea why...

[oshima, 2014-04-26 03:52:56]

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:67: const int kTabIconSize =
gfx::kFaviconSize;
On 2014/04/26 03:43:32, Dan Beam wrote:
> ^ this added a static initializer, but I have no idea why...

ah, I guess it's because it's value is defined in .cc with extern, doh.
I'll fix it soon.

[Dan Beam, 2014-04-26 03:56:08]

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/247193002/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:67: const int kTabIconSize =
gfx::kFaviconSize;
On 2014/04/26 03:43:32, Dan Beam wrote:
> ^ this added a static initializer, but I have no idea why...

this isn't great but it'll keep the rev landed and fix the tree:
https://codereview.chromium.org/259953002