Fix assertion starting cast_shell (breaking thread IO restrictions).

Fix assertion starting cast_shell (breaking thread IO restrictions).

cast_shell creates aura::WindowTreeHost during main loop, and when running with
ozone-platform=egltest this causes a thread_restriction I/O FATAL.

BUG=448802
Committed: https://crrev.com/a9d2cb99125b7598354b7acfa074a1893144221d
Cr-Commit-Position: refs/heads/master@{#312869}

[halliwell, 2015-01-15 21:58:26]

halliwell@chromium.org changed reviewers:
+ spang@chromium.org

[halliwell, 2015-01-15 23:03:07]

halliwell@chromium.org changed reviewers:
+ jochen@chromium.org

[spang, 2015-01-16 01:20:29]

https://codereview.chromium.org/853073003/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/853073003/diff/40001/PRESUBMIT.py#newcode177
PRESUBMIT.py:177:
r"^ui[\\\/]events[\\\/]ozone[\\\/]device[\\\/]device_manager_manual\.cc$",
Doh! Didn't realize this presubmit exited. We probably shouldn't extend this
list even though its the I/O is only in test code.

I think we should change DeviceManagerManual to scan once at startup, and then
keep that list forever. We could even post to base::WorkerPool to avoid doing it
on the main thread.

[jochen (gone - plz use gerrit), 2015-01-16 10:50:02]

not lgtm

please fix the code to not do IO on a restricted thread

[spang, 2015-01-16 18:19:15]

lgtm

[halliwell, 2015-01-16 18:21:20]

halliwell@chromium.org changed reviewers:
+ gunsch@chromium.org

[halliwell, 2015-01-16 18:21:53]

On 2015/01/16 18:19:15, spang wrote:
> lgtm

@gunsch please check chromecast/ changes

[spang, 2015-01-16 18:29:28]

On 2015/01/16 18:21:53, halliwell wrote:
> On 2015/01/16 18:19:15, spang wrote:
> > lgtm
> 
> @gunsch please check chromecast/ changes

jochen: This code is just for testing, it doesn't run in the product. If you
feel very strongly about how some test code works, we could stick a
base::WorkerPool::PostTask in here pretty easily.

[halliwell, 2015-01-16 18:32:21]

On 2015/01/16 18:29:28, spang wrote:
> On 2015/01/16 18:21:53, halliwell wrote:
> > On 2015/01/16 18:19:15, spang wrote:
> > > lgtm
> > 
> > @gunsch please check chromecast/ changes
> 
> jochen: This code is just for testing, it doesn't run in the product. If you
> feel very strongly about how some test code works, we could stick a
> base::WorkerPool::PostTask in here pretty easily.

I moved the IO work to the constructor, it gets run pre main message loop so we
don't need the ScopedAllowIO object any more.

[gunsch, 2015-01-16 19:06:52]

Hmm, I'm unclear on why getting the startup URL needs to happen before the IO
sandbox?

[halliwell, 2015-01-16 19:23:53]

On 2015/01/16 19:06:52, gunsch wrote:
> Hmm, I'm unclear on why getting the startup URL needs to happen before the IO
> sandbox?

Because it has a code path for file path URLs that ends up doing file IO:

return net::FilePathToFileURL(base::FilePath(args[0]));

[gunsch, 2015-01-16 19:24:14]

chromecast/ lgtm

[jochen (gone - plz use gerrit), 2015-01-19 12:49:42]

why not just post a task to a thread that can do IO?

[halliwell, 2015-01-22 15:39:40]

On 2015/01/19 12:49:42, jochen (slow) wrote:
> why not just post a task to a thread that can do IO?

jochen, spang - ptal

[jochen (gone - plz use gerrit), 2015-01-22 16:28:15]

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
File ui/events/ozone/device/device_manager_manual.cc (right):

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
ui/events/ozone/device/device_manager_manual.cc:54: true /* task_is_slow */);
not sure this is a slow task - this will start a new dedicated thread (which is
a good idea for something running something like a minute or longer, but
otherwise not required)

[spang, 2015-01-22 20:45:57]

lgtm

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
File ui/events/ozone/device/device_manager_manual.cc (right):

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
ui/events/ozone/device/device_manager_manual.cc:34: , weak_ptr_factory_(this) {
whitespace issue, please git cl format.

[halliwell, 2015-01-22 21:56:52]

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
File ui/events/ozone/device/device_manager_manual.cc (right):

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
ui/events/ozone/device/device_manager_manual.cc:34: , weak_ptr_factory_(this) {
On 2015/01/22 20:45:57, spang wrote:
> whitespace issue, please git cl format.

Done.

https://codereview.chromium.org/853073003/diff/80001/ui/events/ozone/device/d...
ui/events/ozone/device/device_manager_manual.cc:54: true /* task_is_slow */);
On 2015/01/22 16:28:15, jochen (slow) wrote:
> not sure this is a slow task - this will start a new dedicated thread (which
is
> a good idea for something running something like a minute or longer, but
> otherwise not required)

Done.

[dnicoara, 2015-01-22 22:38:57]

https://codereview.chromium.org/853073003/diff/100001/ui/events/ozone/device/...
File ui/events/ozone/device/device_manager_manual.cc (right):

https://codereview.chromium.org/853073003/diff/100001/ui/events/ozone/device/...
ui/events/ozone/device/device_manager_manual.cc:39: if (have_scanned_devices_) {
Drive-by comment: If multiple callers end up scanning before the worker task
returns, there will be multiple calls to OnDevicesScanned, right? You'll need to
diff between the stored list and the new list otherwise all the devices that
appear in both lists will be duplicates. (Or maybe just discard the old list.)

I wonder if it's worth dropping |have_scanned_devices_| and always posting the
call to ScanDevicesOnWorkerThread()?

[spang, 2015-01-22 22:42:03]

https://codereview.chromium.org/853073003/diff/100001/ui/events/ozone/device/...
File ui/events/ozone/device/device_manager_manual.cc (right):

https://codereview.chromium.org/853073003/diff/100001/ui/events/ozone/device/...
ui/events/ozone/device/device_manager_manual.cc:39: if (have_scanned_devices_) {
On 2015/01/22 22:38:57, dnicoara wrote:
> Drive-by comment: If multiple callers end up scanning before the worker task
> returns, there will be multiple calls to OnDevicesScanned, right? You'll need
to
> diff between the stored list and the new list otherwise all the devices that
> appear in both lists will be duplicates. (Or maybe just discard the old list.)
> 
> I wonder if it's worth dropping |have_scanned_devices_| and always posting the
> call to ScanDevicesOnWorkerThread()?

have_scanned_devices_ is set here in this function, not after the scan
completes. It is not possible to kick off a 2nd scan.

[dnicoara, 2015-01-22 22:43:13]

https://codereview.chromium.org/853073003/diff/100001/ui/events/ozone/device/...
File ui/events/ozone/device/device_manager_manual.cc (right):

https://codereview.chromium.org/853073003/diff/100001/ui/events/ozone/device/...
ui/events/ozone/device/device_manager_manual.cc:39: if (have_scanned_devices_) {
On 2015/01/22 22:42:02, spang wrote:
> On 2015/01/22 22:38:57, dnicoara wrote:
> > Drive-by comment: If multiple callers end up scanning before the worker task
> > returns, there will be multiple calls to OnDevicesScanned, right? You'll
need
> to
> > diff between the stored list and the new list otherwise all the devices that
> > appear in both lists will be duplicates. (Or maybe just discard the old
list.)
> > 
> > I wonder if it's worth dropping |have_scanned_devices_| and always posting
the
> > call to ScanDevicesOnWorkerThread()?
> 
> have_scanned_devices_ is set here in this function, not after the scan
> completes. It is not possible to kick off a 2nd scan.

Oh, sorry, didn't notice it. Sorry for the noise.

[jochen (gone - plz use gerrit), 2015-01-23 14:18:35]

lgtm

[halliwell, 2015-01-23 16:54:26]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2015-01-23 16:55:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853073003/100001

[commit-bot: I haz the power, 2015-01-23 17:44:30]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-23 17:45:19]

Patchset 6 (id:??) landed as
https://crrev.com/a9d2cb99125b7598354b7acfa074a1893144221d
Cr-Commit-Position: refs/heads/master@{#312869}