Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread instead of IO thread

Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

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

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

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

[pkotwicz, 2016-08-10 02:01:57]

Dominick and Yaron, can you please take a look?

[pkotwicz, 2016-08-10 02:23:23]

Description was changed from

==========
Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread
instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

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

to

==========
Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread
instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

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

[dominickn, 2016-08-10 05:18:10]

lgtm % nits and a question. Do you have access to a KK device so you can check
the asserts? Assertions aren't triggered on L+ devices. :)

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:133: *
Adds home screen shortcut which opens in a {@link WebappActivity}.
Nit: add a comment that this runs the procedure on a worker thread, and runs
OnWebappDataStored on the callbackPointer when it's finished.

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:178:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
Does this really need to be parallelisable? Just execute() is probably okay
here.

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:25:
#include "content/public/browser/browser_thread.h"
Remove unused browser_thread.h include.

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

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/shor...
chrome/browser/android/shortcut_helper.cc:20: #include
"content/public/browser/browser_thread.h"
Remove unnecessary browser_thread.h include

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:15: #include
"base/memory/ref_counted.h"
Do you need ref_counted.h here?

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

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:17: #include
"content/public/browser/browser_thread.h"
Remove unnecessary browser_thread.h include.

[Yaron, 2016-08-10 14:57:01]

lgtm

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:200:
Context applicationContext = ContextUtils.getApplicationContext();
assert ui thread

[pkotwicz, 2016-08-10 20:25:25]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-08-10 20:28:42]

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

[pkotwicz, 2016-08-10 20:28:43]

dfalcantara@ can you please take a look at the changes in ShortcutHelper.java to
make sure I didn't do anything outrageously wrong?
agrieve@ w.r.t to question about AsyncTask.THREAD_POOL_EXECUTOR

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:178:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
Adding agrieve@. My understanding is that we should always use
AsyncTask.THREAD_POOL_EXECUTOR now except if there is a good reason not ot

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:25:
#include "content/public/browser/browser_thread.h"
On 2016/08/10 05:18:10, dominickn wrote:
> Remove unused browser_thread.h include.

Done.

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

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/shor...
chrome/browser/android/shortcut_helper.cc:20: #include
"content/public/browser/browser_thread.h"
The include is still needed for the DCHECK() in
ShortcutHelper::FinalizeLauncherIconInBackground()

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:15: #include
"base/memory/ref_counted.h"
It is needed for scoped_refptr on line 119

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

https://codereview.chromium.org/2228273002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapps/add_to_homescreen_dialog_helper.cc:17: #include
"content/public/browser/browser_thread.h"
The include is still needed for
AddToHomescreenDialogHelper::FinalizeLauncherIconInBackground()

[pkotwicz, 2016-08-10 20:29:03]

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

[pkotwicz, 2016-08-10 20:29:14]

dfalcantara@ can you please take a look at the changes in ShortcutHelper.java to
make sure I didn't do anything outrageously wrong?
agrieve@ w.r.t to question about AsyncTask.THREAD_POOL_EXECUTOR

[gone, 2016-08-10 22:42:27]

ShortcutHelper/Banners lgtm.  Didn't look at anything else.

https://chromiumcodereview.appspot.com/2228273002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://chromiumcodereview.appspot.com/2228273002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:134: *
homescreen shortcut and registers web app asynchronously. Calls
Pick one.

webapp      / web app
home screen / homescreen

[pkotwicz, 2016-08-11 00:35:21]

https://codereview.chromium.org/2228273002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2228273002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:134: *
homescreen shortcut and registers web app asynchronously. Calls
On 2016/08/10 22:42:26, dfalcantara wrote:
> Pick one.
> 
> webapp      / web app
> home screen / homescreen

Done.

[agrieve, 2016-08-11 00:42:13]

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:178:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2016/08/10 20:28:43, pkotwicz wrote:
> Adding agrieve@. My understanding is that we should always use
> AsyncTask.THREAD_POOL_EXECUTOR now except if there is a good reason not ot
> 

There are certainly trade-offs in either direction, but until we get something
better, I think THREAD_POOL_EXECUTOR is a good default.

.execute() should never be used since it maps to different executors on
different android versions.

[dominickn, 2016-08-11 01:29:18]

On 2016/08/11 00:42:13, agrieve wrote:
>
https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
> File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
> (right):
> 
>
https://codereview.chromium.org/2228273002/diff/1/chrome/android/java/src/org...
> chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:178:
> }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
> On 2016/08/10 20:28:43, pkotwicz wrote:
> > Adding agrieve@. My understanding is that we should always use
> > AsyncTask.THREAD_POOL_EXECUTOR now except if there is a good reason not ot
> > 
> 
> There are certainly trade-offs in either direction, but until we get something
> better, I think THREAD_POOL_EXECUTOR is a good default.
> 
> .execute() should never be used since it maps to different executors on
> different android versions.

Thanks for the advice, duly noted for the future. :)

[pkotwicz, 2016-08-11 13:37:33]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-11 13:37:33]

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

[commit-bot: I haz the power, 2016-08-11 13:37:45]

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-11 14:44:06]

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

[commit-bot: I haz the power, 2016-08-11 14:44:07]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pkotwicz, 2016-08-11 19:05:51]

Yaron can you please take another look? The changes in
BackgroundShadowAsyncTask.java and the tests in particular.

[pkotwicz, 2016-08-11 19:05:52]

Yaron can you please take another look? The changes in
BackgroundShadowAsyncTask.java and the tests in particular.

[Yaron, 2016-08-12 16:18:07]

lgtm

Noticed a few nits but totally ignorable if you want.

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/browsing_data/BrowsingDataRemoverIntegrationTest.java
(right):

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/browsing_data/BrowsingDataRemoverIntegrationTest.java:63:
AsyncTask<Void, Void, Intent> shortcutIntentTask = new AsyncTask<Void, Void,
Intent>() {
if this were prod code, I'd instead say to pass the full map into the asynctask
process all in the doInBackground but the perf should be inconsequential for
tests

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java
(right):

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java:156:
final Intent shortcutIntent = shortcutIntentTask.execute().get();
i just remembered that technically you're supposed to only launch asynctasks
from the ui thread (which this is not - it's the test thread). Then again, that
only fails on ICS (JB+ internally will post to ui thread before kicking off the
asynctask) so this works just fine. This is more for background info :)

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:753:
private Intent createShortcutIntent(final String url) throws Exception {
nit: given how intimately this knows how to use ShortcutHelper (three successive
calls) it might make more sense to add createShortcutIntentForTesting to
ShortcutHelper. Up to you.

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:763:
.get();
nit: move to previous line

[pkotwicz, 2016-08-12 18:44:27]

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java
(right):

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:753:
private Intent createShortcutIntent(final String url) throws Exception {
I have added ShortcutHelper#createWebappShortcutIntentForTesting()

https://codereview.chromium.org/2228273002/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java:763:
.get();
On 2016/08/12 16:18:07, Yaron (limited availability) wrote:
> nit: move to previous line

Done.

[pkotwicz, 2016-08-12 19:18:17]

Patchset #5 (id:100001) has been deleted

[pkotwicz, 2016-08-12 19:19:42]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-12 19:19:43]

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

[commit-bot: I haz the power, 2016-08-12 19:20:31]

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-12 19:57:00]

Description was changed from

==========
Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread
instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

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

to

==========
Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread
instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

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

[commit-bot: I haz the power, 2016-08-12 19:57:01]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-08-12 19:58:35]

Description was changed from

==========
Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread
instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

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

to

==========
Run ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on Worker thread
instead of IO thread

This CL runs ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() on a
worker thread instead of the IO thread. According to
https://www.chromium.org/developers/design-documents/threading expensive tasks
should not be done on the IO thread. This CL also changes
ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() so that only the
expensive parts are run on the worker thread instead of the entire process
started by ShortcutHelper::AddToLauncherInBackgroundWithSkBitmap() running on
the worker thread.

BUG=635347

Committed: https://crrev.com/c67e6ac6fb0cc0b0ba6582ea1a9e8e62caf9748a
Cr-Commit-Position: refs/heads/master@{#411743}
==========

[commit-bot: I haz the power, 2016-08-12 19:58:36]

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