Permit MaterialDesignController to make IO call

Permit MaterialDesignController to make IO call

As a one-time call on startup, permit
MaterialDesignController to check for
available touch devices on Chrome OS.

BUG=596294
Committed: https://crrev.com/3368658a432ad62dc20b090fd97832707e77a929
Cr-Commit-Position: refs/heads/master@{#392186}

[tdanderson, 2016-03-30 19:09:13]

tdanderson@chromium.org changed reviewers:
+ dnicoara@chromium.org, sadrul@chromium.org

[tdanderson, 2016-03-30 19:09:14]

Sadrul and Dan, here is the implementation of your suggestion; please take a
look. Note that presubmit at the command line gave me the following:

Banned functions were used.
    ui/base/material_design/material_design_controller.cc:58:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

[sadrul, 2016-03-30 19:14:17]

sadrul@chromium.org changed reviewers:
+ pkasting@chromium.org

[sadrul, 2016-03-30 19:14:18]

+pkasting@

https://codereview.chromium.org/1844033002/diff/1/ui/base/material_design/mat...
File ui/base/material_design/material_design_controller.cc (right):

https://codereview.chromium.org/1844033002/diff/1/ui/base/material_design/mat...
ui/base/material_design/material_design_controller.cc:58:
base::ThreadRestrictions::ScopedAllowIO allow_io;
I am not a fan of this. Can we just stick with the block above? How often does
the code fall back into here?

[tdanderson, 2016-03-30 19:26:12]

https://codereview.chromium.org/1844033002/diff/1/ui/base/material_design/mat...
File ui/base/material_design/material_design_controller.cc (right):

https://codereview.chromium.org/1844033002/diff/1/ui/base/material_design/mat...
ui/base/material_design/material_design_controller.cc:58:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2016/03/30 19:14:18, sadrul wrote:
> I am not a fan of this. Can we just stick with the block above? How often does
> the code fall back into here?

The block above is insufficient since whenever the UI restarts (for example,
when changing something in about:flags and then clicking restart) the async
device scan hasn't been completed by the time we reach here.

Upon booting the device, I have observed that device_lists_complete() is true
(line 50) by the time MaterialDesignController needs to know whether to use
material or material-hybrid. But I do not know enough about this to say with any
certainty that that will always be the case. (My current understanding is that
we only need to query MD vs MD hybrid after the user has logged in, and by that
time device_lists_complete() is probably going to be true)

[Peter Kasting, 2016-03-30 19:32:14]

Can we find a place up the callstack from this where we can make this
functionality async?  Then we can punt it to the blocking pool or some other
place where IO is legitimate.

We try not to allow IO calls, even single ones, on the UI thread, especially
during startup, when the disk is generally thrashing.  This has a high
likelihood of stalling the UI thread for a user-visible amount of time at
precisely the point when we least want to stall it.

I realize you are trying to make an existing issue simply not cause a DCHECK,
but unless there's absolutely no other way, it would be better to fix this than
allow it.

[tdanderson, 2016-03-31 16:18:22]

On 2016/03/30 19:32:14, Peter Kasting wrote:
> Can we find a place up the callstack from this where we can make this
> functionality async?  Then we can punt it to the blocking pool or some other
> place where IO is legitimate.
> 
> We try not to allow IO calls, even single ones, on the UI thread, especially
> during startup, when the disk is generally thrashing.  This has a high
> likelihood of stalling the UI thread for a user-visible amount of time at
> precisely the point when we least want to stall it.
> 
> I realize you are trying to make an existing issue simply not cause a DCHECK,
> but unless there's absolutely no other way, it would be better to fix this
than
> allow it.

Sadrul, do you have any concrete suggestions here? I don't see how
we can make this async and still accomplish the same objective.
Perhaps we can chat in person.

In the meanwhile, I'd vote for landing this CL as-is since it isn't
making anything worse, but it is fixing an issue in
developers' workflows.

[Peter Kasting, 2016-03-31 21:09:54]

On 2016/03/31 16:18:22, tdanderson wrote:
> On 2016/03/30 19:32:14, Peter Kasting wrote:
> > Can we find a place up the callstack from this where we can make this
> > functionality async?  Then we can punt it to the blocking pool or some other
> > place where IO is legitimate.
> > 
> > We try not to allow IO calls, even single ones, on the UI thread, especially
> > during startup, when the disk is generally thrashing.  This has a high
> > likelihood of stalling the UI thread for a user-visible amount of time at
> > precisely the point when we least want to stall it.
> > 
> > I realize you are trying to make an existing issue simply not cause a
DCHECK,
> > but unless there's absolutely no other way, it would be better to fix this
> than
> > allow it.
> 
> Sadrul, do you have any concrete suggestions here? I don't see how
> we can make this async and still accomplish the same objective.

Default to one MD style, if the async result comes back the other way make a
change and fire a theme changed notification?

[tdanderson, 2016-04-01 21:02:50]

On 2016/03/31 21:09:54, Peter Kasting wrote:
> On 2016/03/31 16:18:22, tdanderson wrote:
> > On 2016/03/30 19:32:14, Peter Kasting wrote:
> > > Can we find a place up the callstack from this where we can make this
> > > functionality async?  Then we can punt it to the blocking pool or some
other
> > > place where IO is legitimate.
> > > 
> > > We try not to allow IO calls, even single ones, on the UI thread,
especially
> > > during startup, when the disk is generally thrashing.  This has a high
> > > likelihood of stalling the UI thread for a user-visible amount of time at
> > > precisely the point when we least want to stall it.
> > > 
> > > I realize you are trying to make an existing issue simply not cause a
> DCHECK,
> > > but unless there's absolutely no other way, it would be better to fix this
> > than
> > > allow it.
> > 
> > Sadrul, do you have any concrete suggestions here? I don't see how
> > we can make this async and still accomplish the same objective.
> 
> Default to one MD style, if the async result comes back the other way make a
> change and fire a theme changed notification?

Thanks - I made a bit of progress on that approach today, but I have yet
to figure out the right timing for the notification; currently everything
is drawing half-material and half-hybrid.

Note I am OOO starting today so I won't be able to pick this up again for
at least another week. If a fix for the failing DCHECK is critical to
get in by the M-51 branch then someone should land the CL as-is.

[tdanderson, 2016-04-20 15:52:51]

On 2016/04/01 21:02:50, tdanderson wrote:
> On 2016/03/31 21:09:54, Peter Kasting wrote:
> > On 2016/03/31 16:18:22, tdanderson wrote:
> > > On 2016/03/30 19:32:14, Peter Kasting wrote:
> > > > Can we find a place up the callstack from this where we can make this
> > > > functionality async?  Then we can punt it to the blocking pool or some
> other
> > > > place where IO is legitimate.
> > > > 
> > > > We try not to allow IO calls, even single ones, on the UI thread,
> especially
> > > > during startup, when the disk is generally thrashing.  This has a high
> > > > likelihood of stalling the UI thread for a user-visible amount of time
at
> > > > precisely the point when we least want to stall it.
> > > > 
> > > > I realize you are trying to make an existing issue simply not cause a
> > DCHECK,
> > > > but unless there's absolutely no other way, it would be better to fix
this
> > > than
> > > > allow it.
> > > 
> > > Sadrul, do you have any concrete suggestions here? I don't see how
> > > we can make this async and still accomplish the same objective.
> > 
> > Default to one MD style, if the async result comes back the other way make a
> > change and fire a theme changed notification?
> 
> Thanks - I made a bit of progress on that approach today, but I have yet
> to figure out the right timing for the notification; currently everything
> is drawing half-material and half-hybrid.
> 
> Note I am OOO starting today so I won't be able to pick this up again for
> at least another week. If a fix for the failing DCHECK is critical to
> get in by the M-51 branch then someone should land the CL as-is.

Alternate approach based on Peter's suggestion of firing a
theme changed notification: https://codereview.chromium.org/1906563002/

[sadrul, 2016-05-06 21:37:09]

lgtm, with the hope that https://codereview.chromium.org/1906563002/ will land
soon and remove this code altogether (remember to remove the PRESUBMIT entry
too)

[tdanderson, 2016-05-06 21:41:13]

tdanderson@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-05-06 21:41:56]

lgtm

[tdanderson, 2016-05-06 21:42:07]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-05-06 21:42:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1844033002/#ps40001
(title: "presubmit change")

[commit-bot: I haz the power, 2016-05-06 21:42:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844033002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844033002/40001

[commit-bot: I haz the power, 2016-05-06 22:40:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-06 22:42:20]

Description was changed from

==========
Permit MaterialDesignController to make IO call

As a one-time call on startup, permit
MaterialDesignController to check for
available touch devices on Chrome OS.

BUG=596294
==========

to

==========
Permit MaterialDesignController to make IO call

As a one-time call on startup, permit
MaterialDesignController to check for
available touch devices on Chrome OS.

BUG=596294

Committed: https://crrev.com/3368658a432ad62dc20b090fd97832707e77a929
Cr-Commit-Position: refs/heads/master@{#392186}
==========

[commit-bot: I haz the power, 2016-05-06 22:42:21]

Patchset 3 (id:??) landed as
https://crrev.com/3368658a432ad62dc20b090fd97832707e77a929
Cr-Commit-Position: refs/heads/master@{#392186}