[Home] Ensure incognito tab model is created when NTP opened

[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setIsPendingTabAdd() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setIsPendingTabAdd() allows the incognito tab model to destroy the
incognito profile if necessary.

Additionally, TabWindowManager has a new #canDestroyIncognitoProfile()
method that checks selectors for pending tab adds. This method is used
to guard incognito profile destruction.

BUG=725399, 695973, 725413
Review-Url: https://codereview.chromium.org/2899053004
Cr-Commit-Position: refs/heads/master@{#474521}
Committed: https://chromium.googlesource.com/chromium/src/+/4e98ce65239c3022133064637835602cb3d458c2

[Theresa, 2017-05-23 21:21:18]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 21:22:12]

Dry run: 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-23 22:04:06]

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

[commit-bot: I haz the power, 2017-05-23 22:04:07]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Theresa, 2017-05-23 22:30:12]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 22:30:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-05-23 22:34:36]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 22:35:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-05-23 22:35:24]

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

[Theresa, 2017-05-23 22:35:25]

ptal

[Ted C, 2017-05-23 23:50:18]

seems ok but I worry that multi-window might be giving us a headache here

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:247:
public void setMayAddTab(boolean mayAddTab) {
What happens if you are multi-window, you open two incognito tabs in one.  Open
the incognito chrome home panel in the other, then click the close all incognito
notification?

I think it might leave this tab model with a reference to a destroyed profile. 
I think we need to expose this boolean and add a new function to
TabWindowManager (canDestroyIncognitoProfile) that would check this.

From a UI standpoint, should clicking the incognito notification should it close
any open incognito chrome home panels?

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:218:
public void setMayAddTab(boolean mayAddTab);
bikesheading...I like something like setIsPendingTabAdd or something like
that...for no good reason

[Theresa, 2017-05-24 01:02:32]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 01:04:20]

Dry run: 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-24 07:34:37]

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

[commit-bot: I haz the power, 2017-05-24 07:34:37]

Dry run: 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...)

[Theresa, 2017-05-24 16:45:20]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 16:46:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-05-24 16:52:09]

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:247:
public void setMayAddTab(boolean mayAddTab) {
On 2017/05/23 23:50:17, Ted C wrote:
> What happens if you are multi-window, you open two incognito tabs in one. 
Open
> the incognito chrome home panel in the other, then click the close all
incognito
> notification?

That path was causing issues (crashes on a segfault in bookmarks code). I added
the suggested TabWindowManager#canDestroyIncognitoProfile and now the incognito
profile isn't destructed when still in use.

> I think it might leave this tab model with a reference to a destroyed profile.

> I think we need to expose this boolean and add a new function to
> TabWindowManager (canDestroyIncognitoProfile) that would check this.
> From a UI standpoint, should clicking the incognito notification should it
close
> any open incognito chrome home panels?

Yes, I think that any open CH panels on top of incognito tabs should be closed.
I added some code to IncognitoNotificationService to close the bottom sheet if
the incognito ntp is showing.

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModel.java:218:
public void setMayAddTab(boolean mayAddTab);
On 2017/05/23 23:50:17, Ted C wrote:
> bikesheading...I like something like setIsPendingTabAdd or something like
> that...for no good reason

Done.

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(left):

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1041:
getBottomSheet().onProcessUrlViewIntent();
BottomSheetNewTabController hides the new tab UI when the sheet is closed (which
happens on 1048), so there's no need for this call.

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:260:
return mIsPendingTabAdd;
Ted, should this also return true if mIsAddingTab is true? I think in practice
it would be hard to have one activity trying to destroy incognito while another
is in the middle of #addTab() but perhaps it's possible.

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java
(left):

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheet.java:524:
setSheetState(SHEET_STATE_PEEK, true);
This was intended to handle the case where all incognito tabs were closed (e.g.
through the notification), but IncognitoNotificationService handles that
directly now.

[Theresa, 2017-05-24 16:53:28]

Description was changed from

==========
[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setMayAddTab() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setMayAddTab() allows the incognito tab model to destroy the
incognito profile if necessary.

BUG=725399,695973
==========

to

==========
[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setMayAddTab() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setMayAddTab() allows the incognito tab model to destroy the
incognito profile if necessary.

BUG=725399,695973,725413
==========

[Theresa, 2017-05-24 16:54:52]

Description was changed from

==========
[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setMayAddTab() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setMayAddTab() allows the incognito tab model to destroy the
incognito profile if necessary.

BUG=725399,695973,725413
==========

to

==========
[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setIsPendingTabAdd() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setIsPendingTabAdd() allows the incognito tab model to destroy the
incognito profile if necessary.

Additionally, TabWindowManager has a new #canDestroyIncognitoProfile()
method that checks selectors for pending tab adds. This method is used
to guard incognito profile destruction.

BUG=725399,695973,725413
==========

[Theresa, 2017-05-24 17:13:13]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 17:13:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2017-05-24 17:56:08]

lgtm

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:87:
&& TabWindowManager.getInstance().canDestroyIncognitoProfile()) {
I think this should go into the delegate doIncognitoTabsExist, which is where
the other TabWindowManager call is

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:260:
return mIsPendingTabAdd;
On 2017/05/24 16:52:08, Theresa wrote:
> Ted, should this also return true if mIsAddingTab is true? I think in practice
> it would be hard to have one activity trying to destroy incognito while
another
> is in the middle of #addTab() but perhaps it's possible.

I don't think we need to.  The mIsAddingTab is done within a single execution
block on the UI thread, so there definitely shouldn't be a way for the deletion
to happen in the middle (if we do, it is a threading violation in some other
part of the code)

[Theresa, 2017-05-24 18:12:23]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 18:13:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-05-24 18:35:46]

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java
(right):

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:87:
&& TabWindowManager.getInstance().canDestroyIncognitoProfile()) {
On 2017/05/24 17:56:08, Ted C wrote:
> I think this should go into the delegate doIncognitoTabsExist, which is where
> the other TabWindowManager call is

Done.

https://codereview.chromium.org/2899053004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/IncognitoTabModel.java:260:
return mIsPendingTabAdd;
On 2017/05/24 17:56:08, Ted C wrote:
> On 2017/05/24 16:52:08, Theresa wrote:
> > Ted, should this also return true if mIsAddingTab is true? I think in
practice
> > it would be hard to have one activity trying to destroy incognito while
> another
> > is in the middle of #addTab() but perhaps it's possible.
> 
> I don't think we need to.  The mIsAddingTab is done within a single execution
> block on the UI thread, so there definitely shouldn't be a way for the
deletion
> to happen in the middle (if we do, it is a threading violation in some other
> part of the code)

Excellent, thanks!

https://codereview.chromium.org/2899053004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java
(right):

https://codereview.chromium.org/2899053004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationService.java:203:
// animation.
I added this comment to make sure someone doesn't change it to animate without
understanding the repercussions. I think closing with no animation is fine from
a UX perspective since we don't animate the tabs closing either.

https://codereview.chromium.org/2899053004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2899053004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:136:
public int getIncognitoTabCount() {
This method is now only used on canDestroyIncognitoProfile and in a
MultiWindowIntegrationTest. Perhaps it should be marked as @VisibleForTesting
and get an updated comment noting that canDestroyIncognitoProfile() is more
appropriate for most uses?

[Ted C, 2017-05-24 18:57:15]

lgtm

https://codereview.chromium.org/2899053004/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2899053004/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:136:
public int getIncognitoTabCount() {
On 2017/05/24 18:35:46, Theresa (OOO) wrote:
> This method is now only used on canDestroyIncognitoProfile and in a
> MultiWindowIntegrationTest. Perhaps it should be marked as @VisibleForTesting
> and get an updated comment noting that canDestroyIncognitoProfile() is more
> appropriate for most uses?

sgtm

[Theresa, 2017-05-24 19:27:49]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[Theresa, 2017-05-24 19:28:48]

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

[Theresa, 2017-05-24 19:28:48]

Matt, will you please take a quick look at the changes to BottomSheet#loadUrl()?

[commit-bot: I haz the power, 2017-05-24 19:29:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mdjones, 2017-05-24 21:25:47]

BottomSheet lgtm

[Theresa, 2017-05-24 21:28:24]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2017-05-24 21:28:25]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-05-24 21:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2899053004/#ps120001
(title: "Close bottom sheet after loading URL/opening new tab")

[commit-bot: I haz the power, 2017-05-24 21:29:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-05-24 23:46:21]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2017-05-24 23:46:21]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-05-24 23:46:42]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2017-05-24 23:47:30]

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-25 01:57:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495669602500350,
"parent_rev": "d19ecfda89977aef11f98c9ca28547975dbdc8fe", "commit_rev":
"4e98ce65239c3022133064637835602cb3d458c2"}

[commit-bot: I haz the power, 2017-05-25 01:57:39]

Description was changed from

==========
[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setIsPendingTabAdd() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setIsPendingTabAdd() allows the incognito tab model to destroy the
incognito profile if necessary.

Additionally, TabWindowManager has a new #canDestroyIncognitoProfile()
method that checks selectors for pending tab adds. This method is used
to guard incognito profile destruction.

BUG=725399,695973,725413
==========

to

==========
[Home] Ensure incognito tab model is created when NTP opened

With the new Chrome Home NTP design, a new tab object is not created
until the user selects content in the bottom sheet. Previously the
incognito profile was not created until a new tab object was created.

This change introduces a TabModel#setIsPendingTabAdd() method that allows
the BottomSheetNewTabController to tell the incognito tab model when
a new tab may be created soon. This is used to ensure the incognito
profile is created and used for things like the autocomplete controller,
bookmarks bridge, etc. When the bottom sheet is closed, another call
to #setIsPendingTabAdd() allows the incognito tab model to destroy the
incognito profile if necessary.

Additionally, TabWindowManager has a new #canDestroyIncognitoProfile()
method that checks selectors for pending tab adds. This method is used
to guard incognito profile destruction.

BUG=725399,695973,725413

Review-Url: https://codereview.chromium.org/2899053004
Cr-Commit-Position: refs/heads/master@{#474521}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e98ce65239c3022133064637835...
==========

[commit-bot: I haz the power, 2017-05-25 01:57:40]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/4e98ce65239c3022133064637835...