Calls ShortcutHelper::FinalizeLauncherIcon() on worker thread instead of on IO thread

Calls ShortcutHelper::FinalizeLauncherIcon() on worker thread instead of on IO thread

BUG=635347
Committed: https://crrev.com/5774087e1934b4d45ebca21786c461ec5a467410
Cr-Commit-Position: refs/heads/master@{#411068}

[pkotwicz, 2016-08-07 20:51:54]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-08-07 20:51:55]

dominickn@ can you please take a look?

I think that we should not be calling ShortcutHelper::FinalizeLauncherIcon() on
the IO thread.

According to https://www.chromium.org/developers/design-documents/threading
"We also need to avoid blocking I/O on the IO thread.  The reason is that if we
block it for an expensive operation, say disk access, then IPC messages don't
get processed.  The effect is that the user can't interact with a page.  Note
that asynchronous/overlapped IO are fine."

The reason for this CL is that I want to make WebApkInstaller do file IO, which
led me to discovering that file IO should not be done on the IO thread. I am
planning on writing a followup CL which moves
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() to a background thread.

[dominickn, 2016-08-08 03:13:41]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[dominickn, 2016-08-08 03:13:42]

Interestingly, this unwinds crrev.com/1114783002, which among other things
converted ShortcutHelper from posting to the worker pool to posting to the IO
thread.

+dfalcantara: do you remember if there was an important reason for shifting
threads? It was before my time, and the CL review doesn't seem to have anything
relevant.

Makes sense to me to move back to the worker thread unless dfalcantara has an
important reason for it to be on the IO thread.

https://codereview.chromium.org/2218413002/diff/1/chrome/browser/android/shor...
File chrome/browser/android/shortcut_helper.h (right):

https://codereview.chromium.org/2218413002/diff/1/chrome/browser/android/shor...
chrome/browser/android/shortcut_helper.h:105: // Must not be called on the UI
thread.
Nit: "Must be called on a background worker thread." would be more accurate I
think, since it DCHECKs this.

https://codereview.chromium.org/2218413002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2218413002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:214:
content::BrowserThread::PostBlockingPoolTask(
I'm not sure if creating a launcher icon should block shutdown. Perhaps
content::BrowserThread::GetBlockingPool()->PostWorkerTask(... SKIP_ON_SHUTDOWN)?
Not overly fussed either way though.

https://codereview.chromium.org/2218413002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc (right):

https://codereview.chromium.org/2218413002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:89: const
SkBitmap& bitmap,
Nit: I think this indentation is wrong, you may as well fix it while you're here
if that's the case. :)

[pkotwicz, 2016-08-08 03:25:18]

I forwarded you an e-mail where I ask dfalcantara@ about why this is done on the
I/O thread. Based on the discussion in the e-mail it seems like there wasn't a
strong reason for posting to the I/O thread in crrev.com/1114783002

[dominickn, 2016-08-08 03:32:07]

On 2016/08/08 03:25:18, pkotwicz wrote:
> I forwarded you an e-mail where I ask dfalcantara@ about why this is done on
the
> I/O thread. Based on the discussion in the e-mail it seems like there wasn't a
> strong reason for posting to the I/O thread in crrev.com/1114783002

Great, lgtm % nits from #2. :)

[gone, 2016-08-08 17:14:11]

lgtm % dom's comments.

If anything, that email and CL show my naievete when dealing with native
Chromium code -_-

https://chromiumcodereview.appspot.com/2218413002/diff/1/chrome/browser/andro...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/2218413002/diff/1/chrome/browser/andro...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:214:
content::BrowserThread::PostBlockingPoolTask(
On 2016/08/08 03:13:42, dominickn wrote:
> I'm not sure if creating a launcher icon should block shutdown. Perhaps
> content::BrowserThread::GetBlockingPool()->PostWorkerTask(...
SKIP_ON_SHUTDOWN)?
> Not overly fussed either way though.

If you're thinking about shutdown on Android, we don't actually _have_ a clean
shutdown procedure: Android just kills the process when it feels like and we
don't get a reliable signal about it.  Not sure of the value of switching to the
Blocking thing, though I'm not familiar with the call.

[pkotwicz, 2016-08-09 15:50:31]

pkotwicz@chromium.org changed reviewers:
+ yfriedman@chromium.org

[pkotwicz, 2016-08-09 15:50:32]

Adding Yaron as a reviewer for his C++ experience.

Should I be using BrowserThread::PostBlockingPoolTask() or
WorkerPool::PostTask() or something else?

[Yaron, 2016-08-09 20:52:19]

I think you can get the blocking worker pool that it SKIP_ON_SHUTDOWN and use
that:

(e.g.https://cs.chromium.org/chromium/src/chrome/browser/chromeos/app_mode/kiosk_app_manager.cc?sq=package:chromium&dr=CSs&rcl=1470626560&l=136)

https://codereview.chromium.org/2218413002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc (right):

https://codereview.chromium.org/2218413002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:222: if
(!web_contents() || !weak_observer_) return;
Hmm. I don't think we should check the web_contents() here and the background
thread logic doesn't actually depend on it. Instead, it should be checked when
passed back to the UI thread.

https://codereview.chromium.org/2218413002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc:235:
icon_bitmap, shortcut_info_.url, &is_generated);
similarly, the url should be passed in, mutated (currently on L238) and then
passed back as part of the ::NotifyObserver call.

[pkotwicz, 2016-08-10 00:28:39]

Patchset #3 (id:40001) has been deleted

[pkotwicz, 2016-08-10 01:17:56]

Yaron can you please take another look?

[Yaron, 2016-08-10 15:00:10]

lgtm

[pkotwicz, 2016-08-10 15:20:54]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-10 15:20:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2218413002/#ps60001
(title: "Merge branch 'master' into webapk_builder_impl2_thread0")

[commit-bot: I haz the power, 2016-08-10 15:21:13]

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, 2016-08-10 15:25:12]

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

[commit-bot: I haz the power, 2016-08-10 15:25:14]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[pkotwicz, 2016-08-10 17:25:18]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-08-10 17:26:20]

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, 2016-08-10 17:36:55]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-10 17:39:56]

Description was changed from

==========
Calls ShortcutHelper::FinalizeLauncherIcon() on worker thread instead of on IO
thread

BUG=635347
==========

to

==========
Calls ShortcutHelper::FinalizeLauncherIcon() on worker thread instead of on IO
thread

BUG=635347

Committed: https://crrev.com/5774087e1934b4d45ebca21786c461ec5a467410
Cr-Commit-Position: refs/heads/master@{#411068}
==========

[commit-bot: I haz the power, 2016-08-10 17:39:57]

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