Remove the ash-enable-touch-view-testing flag

Remove the ash-enable-touch-view-testing flag

BUG=682530
Review-Url: https://codereview.chromium.org/2642853006
Cr-Commit-Position: refs/heads/master@{#446401}
Committed: https://chromium.googlesource.com/chromium/src/+/4bc67b96bd6c94833b002162e0bab92ff17a9752

[afakhry, 2017-01-19 02:34:26]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 02:34:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 03:26:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 03:26:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2017-01-19 18:47:54]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 18:48:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 19:46:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 19:46:27]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-01-19 20:50:11]

afakhry@chromium.org changed reviewers:
+ derat@chromium.org

[afakhry, 2017-01-19 20:50:12]

derat@, could you please review this CL? Thanks!

[Daniel Erat, 2017-01-19 22:33:28]

derat@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Daniel Erat, 2017-01-19 22:33:29]

+mgiuca

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
File ash/common/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
ash/common/wm/maximize_mode/maximize_mode_controller.cc:128: return
have_seen_accelerometer_data_ ||
the comment makes me think that this should possibly be calling IsEnabled()
instead.

[afakhry, 2017-01-24 03:00:09]

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
File ash/common/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
ash/common/wm/maximize_mode/maximize_mode_controller.cc:128: return
have_seen_accelerometer_data_ ||
On 2017/01/19 22:33:29, Daniel Erat wrote:
> the comment makes me think that this should possibly be calling IsEnabled()
> instead.

That's a reasonable alternative, though from mgiuca's reply on the bug thread,
this comment might be obsolete.

[Daniel Erat, 2017-01-24 04:30:07]

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
File ash/common/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
ash/common/wm/maximize_mode/maximize_mode_controller.cc:128: return
have_seen_accelerometer_data_ ||
On 2017/01/24 03:00:09, afakhry wrote:
> On 2017/01/19 22:33:29, Daniel Erat wrote:
> > the comment makes me think that this should possibly be calling IsEnabled()
> > instead.
> 
> That's a reasonable alternative, though from mgiuca's reply on the bug thread,
> this comment might be obsolete.

would you mind updating/deleting the comment as part of this, if it's in fact
obsolete? i would've recommended adding a TODO to fix it, but a TODO to update a
TODO seems a bit silly. :-)

[afakhry, 2017-01-24 17:23:46]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 17:24:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-01-24 17:24:08]

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
File ash/common/wm/maximize_mode/maximize_mode_controller.cc (right):

https://codereview.chromium.org/2642853006/diff/20001/ash/common/wm/maximize_...
ash/common/wm/maximize_mode/maximize_mode_controller.cc:128: return
have_seen_accelerometer_data_ ||
On 2017/01/24 04:30:06, Daniel Erat wrote:
> On 2017/01/24 03:00:09, afakhry wrote:
> > On 2017/01/19 22:33:29, Daniel Erat wrote:
> > > the comment makes me think that this should possibly be calling
IsEnabled()
> > > instead.
> > 
> > That's a reasonable alternative, though from mgiuca's reply on the bug
thread,
> > this comment might be obsolete.
> 
> would you mind updating/deleting the comment as part of this, if it's in fact
> obsolete? i would've recommended adding a TODO to fix it, but a TODO to update
a
> TODO seems a bit silly. :-)

Done.

[Daniel Erat, 2017-01-24 17:38:46]

lgtm

thanks!

[afakhry, 2017-01-24 17:40:20]

afakhry@chromium.org changed reviewers:
+ holte@chromium.org

[afakhry, 2017-01-24 17:40:21]

holte@ for histograms.xml. Thanks!

[commit-bot: I haz the power, 2017-01-24 19:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 19:25:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2017-01-24 22:17:16]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 22:17:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[afakhry, 2017-01-24 22:20:53]

https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):

https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:455:
EXPECT_TRUE(maximize_mode_controller()->CanEnterMaximizeMode());
derat@ This test doesn't make sense anymore, and hence removed. We can enter
maximized mode as long as touch view is enabled. There's no point of being able
to enter maximize mode if touch view is disabled. It makes me question the need
for |have_seen_accelerometer_data_|.

WDYT?

[Daniel Erat, 2017-01-24 22:23:29]

https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):

https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:455:
EXPECT_TRUE(maximize_mode_controller()->CanEnterMaximizeMode());
On 2017/01/24 22:20:53, afakhry wrote:
> derat@ This test doesn't make sense anymore, and hence removed. We can enter
> maximized mode as long as touch view is enabled. There's no point of being
able
> to enter maximize mode if touch view is disabled. It makes me question the
need
> for |have_seen_accelerometer_data_|.
> 
> WDYT?

i don't know the full background here, but since we have a flag to enable touch
view that (i think) is set on all devices that should use touch view, i agree
that it seems much better to just use that. waiting for an accelerometer event
seems error-prone in both directions.

[commit-bot: I haz the power, 2017-01-24 23:54:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 23:54:16]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-01-25 00:57:23]

https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:95548: -  <int value="-1334327410"
label="ash-enable-touch-view-testing"/>
This should stay in, since it identifies the meaning of data sent by older
clients.

[afakhry, 2017-01-25 01:39:59]

The CQ bit was checked by afakhry@chromium.org to run a CQ dry run

[afakhry, 2017-01-25 01:40:09]

https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):

https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:455:
EXPECT_TRUE(maximize_mode_controller()->CanEnterMaximizeMode());
On 2017/01/24 22:23:28, Daniel Erat wrote:
> On 2017/01/24 22:20:53, afakhry wrote:
> > derat@ This test doesn't make sense anymore, and hence removed. We can enter
> > maximized mode as long as touch view is enabled. There's no point of being
> able
> > to enter maximize mode if touch view is disabled. It makes me question the
> need
> > for |have_seen_accelerometer_data_|.
> > 
> > WDYT?
> 
> i don't know the full background here, but since we have a flag to enable
touch
> view that (i think) is set on all devices that should use touch view, i agree
> that it seems much better to just use that. waiting for an accelerometer event
> seems error-prone in both directions.

I filled https://bugs.chromium.org/p/chromium/issues/detail?id=684932 to decide
the fate of that field.

https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:95548: -  <int value="-1334327410"
label="ash-enable-touch-view-testing"/>
On 2017/01/25 00:57:23, Steven Holte wrote:
> This should stay in, since it identifies the meaning of data sent by older
> clients.

Ah, right. Done. Thanks!

[commit-bot: I haz the power, 2017-01-25 01:40:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 03:04:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 03:04:16]

Dry run: This issue passed the CQ dry run.

[jonross, 2017-01-25 14:33:19]

On 2017/01/25 01:40:09, afakhry wrote:
>
https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
> File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):
> 
>
https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
> ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:455:
> EXPECT_TRUE(maximize_mode_controller()->CanEnterMaximizeMode());
> On 2017/01/24 22:23:28, Daniel Erat wrote:
> > On 2017/01/24 22:20:53, afakhry wrote:
> > > derat@ This test doesn't make sense anymore, and hence removed. We can
enter
> > > maximized mode as long as touch view is enabled. There's no point of being
> > able
> > > to enter maximize mode if touch view is disabled. It makes me question the
> > need
> > > for |have_seen_accelerometer_data_|.
> > > 
> > > WDYT?
> > 
> > i don't know the full background here, but since we have a flag to enable
> touch
> > view that (i think) is set on all devices that should use touch view, i
agree
> > that it seems much better to just use that. waiting for an accelerometer
event
> > seems error-prone in both directions.
> 
> I filled https://bugs.chromium.org/p/chromium/issues/detail?id=684932 to
decide
> the fate of that field.
> 
>
https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:95548: -  <int value="-1334327410"
> label="ash-enable-touch-view-testing"/>
> On 2017/01/25 00:57:23, Steven Holte wrote:
> > This should stay in, since it identifies the meaning of data sent by older
> > clients.
> 
> Ah, right. Done. Thanks!

NLGTM

That testing flag is used to simulate the maximized mode UI with linux chromium
chromium os builds. So that we can debug issues without having to deploy to
chromebooks

[jonross, 2017-01-25 14:33:41]

On 2017/01/25 14:33:19, jonross wrote:
> On 2017/01/25 01:40:09, afakhry wrote:
> >
>
https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
> > File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):
> > 
> >
>
https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
> > ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:455:
> > EXPECT_TRUE(maximize_mode_controller()->CanEnterMaximizeMode());
> > On 2017/01/24 22:23:28, Daniel Erat wrote:
> > > On 2017/01/24 22:20:53, afakhry wrote:
> > > > derat@ This test doesn't make sense anymore, and hence removed. We can
> enter
> > > > maximized mode as long as touch view is enabled. There's no point of
being
> > > able
> > > > to enter maximize mode if touch view is disabled. It makes me question
the
> > > need
> > > > for |have_seen_accelerometer_data_|.
> > > > 
> > > > WDYT?
> > > 
> > > i don't know the full background here, but since we have a flag to enable
> > touch
> > > view that (i think) is set on all devices that should use touch view, i
> agree
> > > that it seems much better to just use that. waiting for an accelerometer
> event
> > > seems error-prone in both directions.
> > 
> > I filled https://bugs.chromium.org/p/chromium/issues/detail?id=684932 to
> decide
> > the fate of that field.
> > 
> >
>
https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
> > File tools/metrics/histograms/histograms.xml (left):
> > 
> >
>
https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
> > tools/metrics/histograms/histograms.xml:95548: -  <int value="-1334327410"
> > label="ash-enable-touch-view-testing"/>
> > On 2017/01/25 00:57:23, Steven Holte wrote:
> > > This should stay in, since it identifies the meaning of data sent by older
> > > clients.
> > 
> > Ah, right. Done. Thanks!
> 
> NLGTM
> 
> That testing flag is used to simulate the maximized mode UI with linux
chromium
> chromium os builds. So that we can debug issues without having to deploy to
> chromebooks

Not LGTM

[jonross, 2017-01-25 14:59:55]

On 2017/01/25 14:33:41, jonross wrote:
> On 2017/01/25 14:33:19, jonross wrote:
> > On 2017/01/25 01:40:09, afakhry wrote:
> > >
> >
>
https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
> > > File ash/wm/maximize_mode/maximize_mode_controller_unittest.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2642853006/diff/60001/ash/wm/maximize_mode/ma...
> > > ash/wm/maximize_mode/maximize_mode_controller_unittest.cc:455:
> > > EXPECT_TRUE(maximize_mode_controller()->CanEnterMaximizeMode());
> > > On 2017/01/24 22:23:28, Daniel Erat wrote:
> > > > On 2017/01/24 22:20:53, afakhry wrote:
> > > > > derat@ This test doesn't make sense anymore, and hence removed. We can
> > enter
> > > > > maximized mode as long as touch view is enabled. There's no point of
> being
> > > > able
> > > > > to enter maximize mode if touch view is disabled. It makes me question
> the
> > > > need
> > > > > for |have_seen_accelerometer_data_|.
> > > > > 
> > > > > WDYT?
> > > > 
> > > > i don't know the full background here, but since we have a flag to
enable
> > > touch
> > > > view that (i think) is set on all devices that should use touch view, i
> > agree
> > > > that it seems much better to just use that. waiting for an accelerometer
> > event
> > > > seems error-prone in both directions.
> > > 
> > > I filled https://bugs.chromium.org/p/chromium/issues/detail?id=684932 to
> > decide
> > > the fate of that field.
> > > 
> > >
> >
>
https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
> > > File tools/metrics/histograms/histograms.xml (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2642853006/diff/60001/tools/metrics/histogram...
> > > tools/metrics/histograms/histograms.xml:95548: -  <int value="-1334327410"
> > > label="ash-enable-touch-view-testing"/>
> > > On 2017/01/25 00:57:23, Steven Holte wrote:
> > > > This should stay in, since it identifies the meaning of data sent by
older
> > > > clients.
> > > 
> > > Ah, right. Done. Thanks!
> > 
> > NLGTM
> > 
> > That testing flag is used to simulate the maximized mode UI with linux
> chromium
> > chromium os builds. So that we can debug issues without having to deploy to
> > chromebooks
> 
> Not LGTM

Nevermind, I had not realized that --ash-debug-shortcuts would allow for this
testing without the existence of this flag.

LGTM

[afakhry, 2017-01-26 17:39:28]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-01-26 17:39:28]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2642853006/#ps80001
(title: "holte's comment.")

[commit-bot: I haz the power, 2017-01-26 17:39:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 19:44:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485452368143400,
"parent_rev": "546947724ee674f3f7f662d4942ceb2916604191", "commit_rev":
"4bc67b96bd6c94833b002162e0bab92ff17a9752"}

[commit-bot: I haz the power, 2017-01-26 19:44:44]

Description was changed from

==========
Remove the ash-enable-touch-view-testing flag

BUG=682530
==========

to

==========
Remove the ash-enable-touch-view-testing flag

BUG=682530

Review-Url: https://codereview.chromium.org/2642853006
Cr-Commit-Position: refs/heads/master@{#446401}
Committed:
https://chromium.googlesource.com/chromium/src/+/4bc67b96bd6c94833b002162e0ba...
==========

[commit-bot: I haz the power, 2017-01-26 19:44:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4bc67b96bd6c94833b002162e0ba...