Fix chrome home crash from new incognito NTP launcher icons.

Fix chrome home crash from new incognito NTP launcher icons.

Need to handle a few other cases for opening the bottom
sheet instead of creating a tab.

BUG=725380
Review-Url: https://codereview.chromium.org/2912473003
Cr-Commit-Position: refs/heads/master@{#475160}
Committed: https://chromium.googlesource.com/chromium/src/+/f81292c48283cc884c56e22c755e7bdaf91741a9

[Ted C, 2017-05-26 17:54:25]

tedchoc@chromium.org changed reviewers:
+ mdjones@chromium.org

[Ted C, 2017-05-26 17:54:26]

mdjones - PTAL
twellington - FYI

[mdjones, 2017-05-26 18:25:57]

lgtm % nit

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

https://codereview.chromium.org/2912473003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:333:
private boolean openNtpBottomSheet(String url) {
nit: doc

https://codereview.chromium.org/2912473003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:337:
mDelayedInitialTabBehaviorDuringUiInit = new Runnable() {
If I understand correctly, this is simply to avoid adding the above checks in
initializeUI right? The reason I ask is because it feels a bit weird to use a
runnable outside the context of a separate thread.

[Ted C, 2017-05-26 18:29:24]

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

https://codereview.chromium.org/2912473003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:337:
mDelayedInitialTabBehaviorDuringUiInit = new Runnable() {
On 2017/05/26 18:25:56, mdjones wrote:
> If I understand correctly, this is simply to avoid adding the above checks in
> initializeUI right? The reason I ask is because it feels a bit weird to use a
> runnable outside the context of a separate thread.

I added it initially when I still had the logic in createInitialTab, but
that wasn't needed anymore.

The reason I kept it was that you would need to keep two booleans otherwise.

One would be that you need to create the NTP, the second is the model to create
the tab into (incognito or normal).

I'm not against the two booleans though if you prefer it.

[mdjones, 2017-05-26 18:54:20]

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

https://codereview.chromium.org/2912473003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:337:
mDelayedInitialTabBehaviorDuringUiInit = new Runnable() {
On 2017/05/26 18:29:23, Ted C wrote:
> On 2017/05/26 18:25:56, mdjones wrote:
> > If I understand correctly, this is simply to avoid adding the above checks
in
> > initializeUI right? The reason I ask is because it feels a bit weird to use
a
> > runnable outside the context of a separate thread.
> 
> I added it initially when I still had the logic in createInitialTab, but
> that wasn't needed anymore.
> 
> The reason I kept it was that you would need to keep two booleans otherwise.
> 
> One would be that you need to create the NTP, the second is the model to
create
> the tab into (incognito or normal).
> 
> I'm not against the two booleans though if you prefer it.

What you have is fine, it will probably be a bit easier to track this way.

[Ted C, 2017-05-26 19:13:14]

The CQ bit was checked by tedchoc@chromium.org

[Ted C, 2017-05-26 19:13:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/2912473003/#ps20001
(title: "Add documentation to bottom sheet method")

[commit-bot: I haz the power, 2017-05-26 19:13:41]

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-05-26 19:21:35]

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

[commit-bot: I haz the power, 2017-05-26 19:21:37]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ted C, 2017-05-26 20:23:16]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2017-05-26 20:23:43]

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-05-26 22:19:16]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495830196784500,
"parent_rev": "6a5692489564312b0ef27ad53e81289f8d3092d6", "commit_rev":
"f81292c48283cc884c56e22c755e7bdaf91741a9"}

[commit-bot: I haz the power, 2017-05-26 22:19:26]

Description was changed from

==========
Fix chrome home crash from new incognito NTP launcher icons.

Need to handle a few other cases for opening the bottom
sheet instead of creating a tab.

BUG=725380
==========

to

==========
Fix chrome home crash from new incognito NTP launcher icons.

Need to handle a few other cases for opening the bottom
sheet instead of creating a tab.

BUG=725380

Review-Url: https://codereview.chromium.org/2912473003
Cr-Commit-Position: refs/heads/master@{#475160}
Committed:
https://chromium.googlesource.com/chromium/src/+/f81292c48283cc884c56e22c755e...
==========

[commit-bot: I haz the power, 2017-05-26 22:19:28]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f81292c48283cc884c56e22c755e...

[Theresa, 2017-06-07 17:07:11]

twellington@chromium.org changed reviewers:
+ twellington@chromium.org

[Theresa, 2017-06-07 17:07:12]

https://codereview.chromium.org/2912473003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2912473003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:343:
assert mDelayedInitialTabBehaviorDuringUiInit == null;
I get crashes on this on a local build after:

1) Clear all data
2) Enable chrome home in chrome://flags
3) Close all tabs
4) Restart Chrome

[Theresa, 2017-06-09 17:46:37]

https://codereview.chromium.org/2912473003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2912473003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:343:
assert mDelayedInitialTabBehaviorDuringUiInit == null;
On 2017/06/07 17:07:11, Theresa wrote:
> I get crashes on this on a local build after:
> 
> 1) Clear all data
> 2) Enable chrome home in chrome://flags
> 3) Close all tabs
> 4) Restart Chrome

Filed as crbug.com/731790 against myself to track.