Remove touch optimized layout on ChromeOS

Remove touch optimized layout on ChromeOS

BUG=138281
TEST=No touch optimized ui in about flags, nor does it get set when a touch screen is plugged in.


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

[flackr, 2012-08-11 00:15:55]

Hey Rick,

This should effectively disable automatic touch optimized UI but still allow
manually setting the flag. Resource paks also won't be installed but will be
loaded if available and the flag is passed.

PTAL, thanks. This should be safe to land whenever we're happy with the
non-touch layout.

[Rick Byers, 2012-08-13 16:35:54]

On 2012/08/11 00:15:55, flackr wrote:
> Hey Rick,
> 
> This should effectively disable automatic touch optimized UI but still allow
> manually setting the flag. Resource paks also won't be installed but will be
> loaded if available and the flag is passed.
> 
> PTAL, thanks. This should be safe to land whenever we're happy with the
> non-touch layout.

Looks good, except I think this will break the pointer/hover media queries.  I'd
suggest you make the bit passed to WebKit conditional only on the presence of a
touch screen, as opposed to the value of this flag.  We'll still have to update
WebCore to not use touch-specific metrics, but that's an existing bug.
One piece missing here is

[flackr, 2012-08-13 19:33:24]

On 2012/08/13 16:35:54, Rick Byers wrote:
> On 2012/08/11 00:15:55, flackr wrote:
> > Hey Rick,
> > 
> > This should effectively disable automatic touch optimized UI but still allow
> > manually setting the flag. Resource paks also won't be installed but will be
> > loaded if available and the flag is passed.
> > 
> > PTAL, thanks. This should be safe to land whenever we're happy with the
> > non-touch layout.
> 
> Looks good, except I think this will break the pointer/hover media queries. 
I'd
> suggest you make the bit passed to WebKit conditional only on the presence of
a
> touch screen, as opposed to the value of this flag.  We'll still have to
update
> WebCore to not use touch-specific metrics, but that's an existing bug.
> One piece missing here is

Done. We still use the touch layout for metro apps so I've preserved using this
to toggle the touch mode but also set the flag if a touch screen is detected on
chromeos.

[Rick Byers, 2012-08-13 20:49:06]

lgtm, thanks!

[flackr, 2012-08-14 13:34:21]

Scott, can you take a look? Thanks!

[sky, 2012-08-14 15:49:43]

I believe this means GetDisplayLayout() no longer returns LAYOUT_TOUCH for
chromeos, right? If so, I think you need to update a few places that look at
this switch. Currently we use GetDisplayLayout() for a combination of sizing and
behavior. I think we still want the behavior, right? For example, the tabstrip
behaves differently when touch is available. We still want that behavior, right?

[flackr, 2012-08-14 15:54:05]

On 2012/08/14 15:49:43, sky wrote:
> I believe this means GetDisplayLayout() no longer returns LAYOUT_TOUCH for
> chromeos, right? If so, I think you need to update a few places that look at
> this switch. Currently we use GetDisplayLayout() for a combination of sizing
and
> behavior. I think we still want the behavior, right? For example, the tabstrip
> behaves differently when touch is available. We still want that behavior,
right?

As far as I know we want to change the non-touch behaviour to be more like the
touch behaviour, but some of the touch sizing is too drastic so the plan is to
change it on a case by case basis.

Also, I've purposely preserved the ability to set the flag from the command-line
for chromeos for now, but the user has to copy the touch resource paks in
manually.

[Rick Byers, 2012-08-14 15:58:18]

On 2012/08/14 15:54:05, flackr wrote:
> On 2012/08/14 15:49:43, sky wrote:
> > I believe this means GetDisplayLayout() no longer returns LAYOUT_TOUCH for
> > chromeos, right? If so, I think you need to update a few places that look at
> > this switch. Currently we use GetDisplayLayout() for a combination of sizing
> and
> > behavior. I think we still want the behavior, right? For example, the
tabstrip
> > behaves differently when touch is available. We still want that behavior,
> right?
> 
> As far as I know we want to change the non-touch behaviour to be more like the
> touch behaviour, but some of the touch sizing is too drastic so the plan is to
> change it on a case by case basis.
> 
> Also, I've purposely preserved the ability to set the flag from the
command-line
> for chromeos for now, but the user has to copy the touch resource paks in
> manually.

Yes, there are separate bugs tracking adjusting the behavior, see
http://code.google.com/p/chromium/issues/list?q=label:UnifyTouchUI

We want to land this change now though in advance of each bug landing to stop
the bleeding - even though it'll mean some temporary 'regressions' on
touch-enabled devices (we think that's better than continuing to delude
ourselves now that the decision has been made that we won't have a
touch-specific UI mode).

Regarding the tab strip, this is what we talked about briefly when I was in MTV
- tab strip should just work as we want it to (desktop sizings with touch
stacking behavior based on event type).

[sky, 2012-08-14 16:09:26]

On 2012/08/14 15:58:18, Rick Byers wrote:
> On 2012/08/14 15:54:05, flackr wrote:
> > On 2012/08/14 15:49:43, sky wrote:
> > > I believe this means GetDisplayLayout() no longer returns LAYOUT_TOUCH for
> > > chromeos, right? If so, I think you need to update a few places that look
at
> > > this switch. Currently we use GetDisplayLayout() for a combination of
sizing
> > and
> > > behavior. I think we still want the behavior, right? For example, the
> tabstrip
> > > behaves differently when touch is available. We still want that behavior,
> > right?
> > 
> > As far as I know we want to change the non-touch behaviour to be more like
the
> > touch behaviour, but some of the touch sizing is too drastic so the plan is
to
> > change it on a case by case basis.
> > 
> > Also, I've purposely preserved the ability to set the flag from the
> command-line
> > for chromeos for now, but the user has to copy the touch resource paks in
> > manually.
> 
> Yes, there are separate bugs tracking adjusting the behavior, see
> http://code.google.com/p/chromium/issues/list?q=label:UnifyTouchUI
> 
> We want to land this change now though in advance of each bug landing to stop
> the bleeding - even though it'll mean some temporary 'regressions' on
> touch-enabled devices (we think that's better than continuing to delude
> ourselves now that the decision has been made that we won't have a
> touch-specific UI mode).
> 
> Regarding the tab strip, this is what we talked about briefly when I was in
MTV
> - tab strip should just work as we want it to (desktop sizings with touch
> stacking behavior based on event type).

As I said, LAYOUT_TOUCH can mean two things: adjust the sizes AND change the
behavior. The tabstrip is an example that both changes the size and changes the
behavior when LAYOUT_TOUCH is set. I believe this patch removes both the size
change AND the behavior change. What I'm saying is that this patch gives purely
desktop behavior for the tabstrip since we have no way currently to express only
the behavior aspect of LAYOUT_TOUCH.

I'll LGTM this patch, since it sounds like you guys are eager to land it. But
someone needs to go through and look at every where we're switching on
LAYOUT_TOUCH and reenable the behavior changes based on some new switch. Also,
it may be there is now some dead code in the chromeos side since it sounds like
we're never going with LAYOUT_TOUCH there.

  -Scott

[sky, 2012-08-14 16:19:40]

On 2012/08/14 16:09:26, sky wrote:
> On 2012/08/14 15:58:18, Rick Byers wrote:
> > On 2012/08/14 15:54:05, flackr wrote:
> > > On 2012/08/14 15:49:43, sky wrote:
> > > > I believe this means GetDisplayLayout() no longer returns LAYOUT_TOUCH
for
> > > > chromeos, right? If so, I think you need to update a few places that
look
> at
> > > > this switch. Currently we use GetDisplayLayout() for a combination of
> sizing
> > > and
> > > > behavior. I think we still want the behavior, right? For example, the
> > tabstrip
> > > > behaves differently when touch is available. We still want that
behavior,
> > > right?
> > > 
> > > As far as I know we want to change the non-touch behaviour to be more like
> the
> > > touch behaviour, but some of the touch sizing is too drastic so the plan
is
> to
> > > change it on a case by case basis.
> > > 
> > > Also, I've purposely preserved the ability to set the flag from the
> > command-line
> > > for chromeos for now, but the user has to copy the touch resource paks in
> > > manually.
> > 
> > Yes, there are separate bugs tracking adjusting the behavior, see
> > http://code.google.com/p/chromium/issues/list?q=label:UnifyTouchUI
> > 
> > We want to land this change now though in advance of each bug landing to
stop
> > the bleeding - even though it'll mean some temporary 'regressions' on
> > touch-enabled devices (we think that's better than continuing to delude
> > ourselves now that the decision has been made that we won't have a
> > touch-specific UI mode).
> > 
> > Regarding the tab strip, this is what we talked about briefly when I was in
> MTV
> > - tab strip should just work as we want it to (desktop sizings with touch
> > stacking behavior based on event type).
> 
> As I said, LAYOUT_TOUCH can mean two things: adjust the sizes AND change the
> behavior. The tabstrip is an example that both changes the size and changes
the
> behavior when LAYOUT_TOUCH is set. I believe this patch removes both the size
> change AND the behavior change. What I'm saying is that this patch gives
purely
> desktop behavior for the tabstrip since we have no way currently to express
only
> the behavior aspect of LAYOUT_TOUCH.
> 
> I'll LGTM this patch, since it sounds like you guys are eager to land it. But
> someone needs to go through and look at every where we're switching on
> LAYOUT_TOUCH and reenable the behavior changes based on some new switch. Also,
> it may be there is now some dead code in the chromeos side since it sounds
like
> we're never going with LAYOUT_TOUCH there.
> 
>   -Scott

I took a quick look. browser_tab_strip_controller.cc is the place that needs to
change for the tabstrip. wrench_menu need to change too, but I'm not positive
there. Those are the only possible places that need to be changed.

Regardless, there is some dead code after this change that should be removed
too. So, it shouldn't be that hard to include the changes in this patch, but
I'll leave it up to you guys.

  -Scott

[Rick Byers, 2012-08-14 17:18:56]

On 2012/08/14 16:19:40, sky wrote:
> On 2012/08/14 16:09:26, sky wrote:
> > On 2012/08/14 15:58:18, Rick Byers wrote:
> > > On 2012/08/14 15:54:05, flackr wrote:
> > > > On 2012/08/14 15:49:43, sky wrote:
> > > > > I believe this means GetDisplayLayout() no longer returns LAYOUT_TOUCH
> for
> > > > > chromeos, right? If so, I think you need to update a few places that
> look
> > at
> > > > > this switch. Currently we use GetDisplayLayout() for a combination of
> > sizing
> > > > and
> > > > > behavior. I think we still want the behavior, right? For example, the
> > > tabstrip
> > > > > behaves differently when touch is available. We still want that
> behavior,
> > > > right?
> > > > 
> > > > As far as I know we want to change the non-touch behaviour to be more
like
> > the
> > > > touch behaviour, but some of the touch sizing is too drastic so the plan
> is
> > to
> > > > change it on a case by case basis.
> > > > 
> > > > Also, I've purposely preserved the ability to set the flag from the
> > > command-line
> > > > for chromeos for now, but the user has to copy the touch resource paks
in
> > > > manually.
> > > 
> > > Yes, there are separate bugs tracking adjusting the behavior, see
> > > http://code.google.com/p/chromium/issues/list?q=label:UnifyTouchUI
> > > 
> > > We want to land this change now though in advance of each bug landing to
> stop
> > > the bleeding - even though it'll mean some temporary 'regressions' on
> > > touch-enabled devices (we think that's better than continuing to delude
> > > ourselves now that the decision has been made that we won't have a
> > > touch-specific UI mode).
> > > 
> > > Regarding the tab strip, this is what we talked about briefly when I was
in
> > MTV
> > > - tab strip should just work as we want it to (desktop sizings with touch
> > > stacking behavior based on event type).
> > 
> > As I said, LAYOUT_TOUCH can mean two things: adjust the sizes AND change the
> > behavior. The tabstrip is an example that both changes the size and changes
> the
> > behavior when LAYOUT_TOUCH is set. I believe this patch removes both the
size
> > change AND the behavior change. What I'm saying is that this patch gives
> purely
> > desktop behavior for the tabstrip since we have no way currently to express
> only
> > the behavior aspect of LAYOUT_TOUCH.

Ah, sorry - I misunderstood then.  When we talked in MTV I thought you said that
LAYOUT_TOUCH impacts only the size, and the behavior is controlled by the event
type.  I've been trying to play with this lately, but all the crashes when using
the tab strip with touch have made it hard to see the behavior.  Yes I agree
completely - the behavior needs to be based purely off of the event type, not
the layout.  flackr@ is going to file a bug for that.

> > I'll LGTM this patch, since it sounds like you guys are eager to land it.
But
> > someone needs to go through and look at every where we're switching on
> > LAYOUT_TOUCH and reenable the behavior changes based on some new switch.
Also,
> > it may be there is now some dead code in the chromeos side since it sounds
> like
> > we're never going with LAYOUT_TOUCH there.
> > 
> >   -Scott
> 
> I took a quick look. browser_tab_strip_controller.cc is the place that needs
to
> change for the tabstrip. wrench_menu need to change too, but I'm not positive
> there. Those are the only possible places that need to be changed.

Thanks for checking.  http://code.google.com/p/chromium/issues/detail?id=137345
already tracks updating the wrench menu to the new style & behavior (touch and
non-touch on CrOS).  So it sounds like it's only the tab strip we were missing.

> Regardless, there is some dead code after this change that should be removed
> too. So, it shouldn't be that hard to include the changes in this patch, but
> I'll leave it up to you guys.

The code isn't technically 'dead' in that it's still available by explicitly
setting --touch-optimized-ui, right?  I'm  not quite convinced 100% that
touch-optimized-ui mode is going away for good - I think after people use touch
for awhile without it they may decide it should come back (or at the very least,
want to compare between the two).  Regardless I agree we need to make sure to
clean it up once we're convinced the change is final.  flackr@ is going to file
an M24 bug against himself for this.  Sound good?

[sky, 2012-08-14 17:31:13]

SGTM

On Tue, Aug 14, 2012 at 10:18 AM,  <rbyers@chromium.org> wrote:
> On 2012/08/14 16:19:40, sky wrote:
>>
>> On 2012/08/14 16:09:26, sky wrote:
>> > On 2012/08/14 15:58:18, Rick Byers wrote:
>> > > On 2012/08/14 15:54:05, flackr wrote:
>> > > > On 2012/08/14 15:49:43, sky wrote:
>> > > > > I believe this means GetDisplayLayout() no longer returns
>> > > > > LAYOUT_TOUCH
>> for
>> > > > > chromeos, right? If so, I think you need to update a few places
>> > > > > that
>> look
>> > at
>> > > > > this switch. Currently we use GetDisplayLayout() for a combination
>> > > > > of
>> > sizing
>> > > > and
>> > > > > behavior. I think we still want the behavior, right? For example,
>> > > > > the
>> > > tabstrip
>> > > > > behaves differently when touch is available. We still want that
>> behavior,
>> > > > right?
>> > > >
>> > > > As far as I know we want to change the non-touch behaviour to be
>> > > > more
>
> like
>>
>> > the
>> > > > touch behaviour, but some of the touch sizing is too drastic so the
>> > > > plan
>> is
>> > to
>> > > > change it on a case by case basis.
>> > > >
>> > > > Also, I've purposely preserved the ability to set the flag from the
>> > > command-line
>> > > > for chromeos for now, but the user has to copy the touch resource
>> > > > paks
>
> in
>>
>> > > > manually.
>> > >
>> > > Yes, there are separate bugs tracking adjusting the behavior, see
>> > > http://code.google.com/p/chromium/issues/list?q=label:UnifyTouchUI
>> > >
>> > > We want to land this change now though in advance of each bug landing
>> > > to
>> stop
>> > > the bleeding - even though it'll mean some temporary 'regressions' on
>> > > touch-enabled devices (we think that's better than continuing to
>> > > delude
>> > > ourselves now that the decision has been made that we won't have a
>> > > touch-specific UI mode).
>> > >
>> > > Regarding the tab strip, this is what we talked about briefly when I
>> > > was
>
> in
>>
>> > MTV
>> > > - tab strip should just work as we want it to (desktop sizings with
>> > > touch
>> > > stacking behavior based on event type).
>> >
>> > As I said, LAYOUT_TOUCH can mean two things: adjust the sizes AND change
>> > the
>> > behavior. The tabstrip is an example that both changes the size and
>> > changes
>> the
>> > behavior when LAYOUT_TOUCH is set. I believe this patch removes both the
>
> size
>>
>> > change AND the behavior change. What I'm saying is that this patch gives
>> purely
>> > desktop behavior for the tabstrip since we have no way currently to
>> > express
>> only
>> > the behavior aspect of LAYOUT_TOUCH.
>
>
> Ah, sorry - I misunderstood then.  When we talked in MTV I thought you said
> that
> LAYOUT_TOUCH impacts only the size, and the behavior is controlled by the
> event
> type.  I've been trying to play with this lately, but all the crashes when
> using
> the tab strip with touch have made it hard to see the behavior.  Yes I agree
> completely - the behavior needs to be based purely off of the event type,
> not
> the layout.  flackr@ is going to file a bug for that.
>
>
>> > I'll LGTM this patch, since it sounds like you guys are eager to land
>> > it.
>
> But
>>
>> > someone needs to go through and look at every where we're switching on
>> > LAYOUT_TOUCH and reenable the behavior changes based on some new switch.
>
> Also,
>>
>> > it may be there is now some dead code in the chromeos side since it
>> > sounds
>> like
>> > we're never going with LAYOUT_TOUCH there.
>> >
>> >   -Scott
>
>
>> I took a quick look. browser_tab_strip_controller.cc is the place that
>> needs
>
> to
>>
>> change for the tabstrip. wrench_menu need to change too, but I'm not
>> positive
>> there. Those are the only possible places that need to be changed.
>
>
> Thanks for checking.
> http://code.google.com/p/chromium/issues/detail?id=137345
> already tracks updating the wrench menu to the new style & behavior (touch
> and
> non-touch on CrOS).  So it sounds like it's only the tab strip we were
> missing.
>
>
>> Regardless, there is some dead code after this change that should be
>> removed
>> too. So, it shouldn't be that hard to include the changes in this patch,
>> but
>> I'll leave it up to you guys.
>
>
> The code isn't technically 'dead' in that it's still available by explicitly
> setting --touch-optimized-ui, right?  I'm  not quite convinced 100% that
> touch-optimized-ui mode is going away for good - I think after people use
> touch
> for awhile without it they may decide it should come back (or at the very
> least,
> want to compare between the two).  Regardless I agree we need to make sure
> to
> clean it up once we're convinced the change is final.  flackr@ is going to
> file
> an M24 bug against himself for this.  Sound good?
>
>
> http://codereview.chromium.org/10832252/

[commit-bot: I haz the power, 2012-08-15 13:00:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10832252/9001

[commit-bot: I haz the power, 2012-08-15 15:22:57]

Change committed as 151694