[Home] Add a Chrome Home specific incognito NTP

[Home] Add a Chrome Home specific incognito NTP

Adds a ChromeHomeIncognitoNewTabPage displayed behind the BottomSheet
when an incognito NTP is selected and an IncognitoBottomSheetContent
that is displayed inside the "Home" tab of the BottomSheet in incognito
mode. Also updates ChromeHomeNewTabPageTest.

BUG=695973
Review-Url: https://codereview.chromium.org/2809923003
Cr-Commit-Position: refs/heads/master@{#465036}
Committed: https://chromium.googlesource.com/chromium/src/+/c72acf085d8c0977f28f3e54f4e99cc2cbd55554

[Theresa, 2017-04-11 23:21:05]

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

[commit-bot: I haz the power, 2017-04-11 23:21:41]

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-04-11 23:25:54]

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

[commit-bot: I haz the power, 2017-04-11 23:25:55]

Dry run: 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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Theresa, 2017-04-11 23:40:15]

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

[commit-bot: I haz the power, 2017-04-11 23:41:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-04-11 23:47:49]

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

[Theresa, 2017-04-11 23:47:50]

Matt, will you please do a first pass? Then I'll add the NTP folks.

[commit-bot: I haz the power, 2017-04-12 01:30:20]

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

[commit-bot: I haz the power, 2017-04-12 01:30:21]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-04-12 16:51:36]

Mostly looks good, just a few questions.

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:198:
if (mTabModelSelector.isIncognitoSelected() && navItemId == R.id.action_home) {
Why is this special cased? Is the assumption that, since there's little content,
the user won't be returning?

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageTest.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageTest.java:180:
getActivity().getLayoutManager().getActiveLayout().finishAnimationsForTests();
Does waitForCallback timeout without this?

[Theresa, 2017-04-12 21:32:03]

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:198:
if (mTabModelSelector.isIncognitoSelected() && navItemId == R.id.action_home) {
On 2017/04/12 16:51:35, mdjones wrote:
> Why is this special cased? Is the assumption that, since there's little
content,
> the user won't be returning?

The map storing all of the other BottomSheetContent objects is keyed off from
navigation item id. R.id.action_home is overloaded (it can be
IncognitobottomSheetContent or SuggestionsBottomSheetContent). Instead of
introducing special code here, I could manipulate the navItemId used to get the
BottomSheetContent out of the map, but that seemed more complex.

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageTest.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageTest.java:180:
getActivity().getLayoutManager().getActiveLayout().finishAnimationsForTests();
On 2017/04/12 16:51:35, mdjones wrote:
> Does waitForCallback timeout without this?

When I initially wrote testCloseNTP_OneTab I incorrectly asserted that overview
mode shouldn't be visible after closing the tab. If there's only one tab and
it's closed then the overview should be visible. Without this call to finish
animations the tab switcher animation was still in progress when
assertTrue("Overview mode should be showing", ...) on line 84 was called so the
check failed.

The new close tab callback technically isn't needed for the tests to pass, but I
think it's good to verify that a tab was in fact closed after pressing the close
button on the chrome home NTP.

[dgn, 2017-04-12 22:27:59]

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

[dgn, 2017-04-12 22:28:00]

drive-by

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageBase.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageBase.java:128:
public void destroy() {
nit: adding the @CallSuper annotation will let the linter know it should enforce
subclasses calling it in redefinitions

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:54:
@IntDef({TYPE_SUGGESTIONS, TYPE_DOWNLOADS, TYPE_BOOKMARKS, TYPE_HISTORY})
add incognito_home here

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:198:
if (mTabModelSelector.isIncognitoSelected() && navItemId == R.id.action_home) {
On 2017/04/12 21:32:03, Theresa wrote:
> On 2017/04/12 16:51:35, mdjones wrote:
> > Why is this special cased? Is the assumption that, since there's little
> content,
> > the user won't be returning?
> 
> The map storing all of the other BottomSheetContent objects is keyed off from
> navigation item id. R.id.action_home is overloaded (it can be
> IncognitobottomSheetContent or SuggestionsBottomSheetContent). Instead of
> introducing special code here, I could manipulate the navItemId used to get
the
> BottomSheetContent out of the map, but that seemed more complex.

why is introducing a special code a bad thing? According to
https://developer.android.com/reference/android/view/View.html#setId(int) Ids
are positive ints, so you could use -1 without risking conflicts?

[Theresa, 2017-04-12 23:12:12]

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:198:
if (mTabModelSelector.isIncognitoSelected() && navItemId == R.id.action_home) {
On 2017/04/12 22:28:00, dgn (in PST timezone) wrote:
> On 2017/04/12 21:32:03, Theresa wrote:
> > On 2017/04/12 16:51:35, mdjones wrote:
> > > Why is this special cased? Is the assumption that, since there's little
> > content,
> > > the user won't be returning?
> > 
> > The map storing all of the other BottomSheetContent objects is keyed off
from
> > navigation item id. R.id.action_home is overloaded (it can be
> > IncognitobottomSheetContent or SuggestionsBottomSheetContent). Instead of
> > introducing special code here, I could manipulate the navItemId used to get
> the
> > BottomSheetContent out of the map, but that seemed more complex.
> 
> why is introducing a special code a bad thing? According to
> https://developer.android.com/reference/android/view/View.html#setId(int) Ids
> are positive ints, so you could use -1 without risking conflicts? 

Changing the id isn't a bad thing. The IncognitoBottomSheetContent has to be
handled separately regardless of whether it's in the map or stored as a separate
variable. Storing it as a separate variable introduces a branch in the code
here. Storing it in the map introduces an if statement to change the navItemId
here and another check in BottomSheetObserver#onSheetClosed() to avoid
destroying the IncognitoBottomSheetContent. I think both approaches are valid,
so I'm happy to put it in the map if you and Matt feel strongly about the
approach.

[mdjones, 2017-04-13 20:34:03]

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:198:
if (mTabModelSelector.isIncognitoSelected() && navItemId == R.id.action_home) {
On 2017/04/12 23:12:11, Theresa wrote:
> On 2017/04/12 22:28:00, dgn (in PST timezone) wrote:
> > On 2017/04/12 21:32:03, Theresa wrote:
> > > On 2017/04/12 16:51:35, mdjones wrote:
> > > > Why is this special cased? Is the assumption that, since there's little
> > > content,
> > > > the user won't be returning?
> > > 
> > > The map storing all of the other BottomSheetContent objects is keyed off
> from
> > > navigation item id. R.id.action_home is overloaded (it can be
> > > IncognitobottomSheetContent or SuggestionsBottomSheetContent). Instead of
> > > introducing special code here, I could manipulate the navItemId used to
get
> > the
> > > BottomSheetContent out of the map, but that seemed more complex.
> > 
> > why is introducing a special code a bad thing? According to
> > https://developer.android.com/reference/android/view/View.html#setId(int)
Ids
> > are positive ints, so you could use -1 without risking conflicts? 
> 
> Changing the id isn't a bad thing. The IncognitoBottomSheetContent has to be
> handled separately regardless of whether it's in the map or stored as a
separate
> variable. Storing it as a separate variable introduces a branch in the code
> here. Storing it in the map introduces an if statement to change the navItemId
> here and another check in BottomSheetObserver#onSheetClosed() to avoid
> destroying the IncognitoBottomSheetContent. I think both approaches are valid,
> so I'm happy to put it in the map if you and Matt feel strongly about the
> approach.

I think we only need to special-case the suggestions content; the incognito view
is quite a bit smaller and can be created much more quickly. If you agree on
that point, I'd rather have the incognito view live in the map.

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageTest.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageTest.java:180:
getActivity().getLayoutManager().getActiveLayout().finishAnimationsForTests();
On 2017/04/12 21:32:03, Theresa wrote:
> On 2017/04/12 16:51:35, mdjones wrote:
> > Does waitForCallback timeout without this?
> 
> When I initially wrote testCloseNTP_OneTab I incorrectly asserted that
overview
> mode shouldn't be visible after closing the tab. If there's only one tab and
> it's closed then the overview should be visible. Without this call to finish
> animations the tab switcher animation was still in progress when
> assertTrue("Overview mode should be showing", ...) on line 84 was called so
the
> check failed.
> 
> The new close tab callback technically isn't needed for the tests to pass, but
I
> think it's good to verify that a tab was in fact closed after pressing the
close
> button on the chrome home NTP.

Acknowledged.

[Theresa, 2017-04-13 21:23:55]

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

[commit-bot: I haz the power, 2017-04-13 21:24:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-04-13 21:26:06]

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

[commit-bot: I haz the power, 2017-04-13 21:26:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-04-13 21:28:13]

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

[Theresa, 2017-04-13 21:28:14]

Screenshots attached to the bug.

mdjones@ - please take another look.
dgn@ - will you please do a full review?
+holte@ - ptal at actions.xml

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageBase.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/ChromeHomeNewTabPageBase.java:128:
public void destroy() {
On 2017/04/12 22:28:00, dgn (in PST timezone) wrote:
> nit: adding the @CallSuper annotation will let the linter know it should
enforce
> subclasses calling it in redefinitions

Done.

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:54:
@IntDef({TYPE_SUGGESTIONS, TYPE_DOWNLOADS, TYPE_BOOKMARKS, TYPE_HISTORY})
On 2017/04/12 22:28:00, dgn (in PST timezone) wrote:
> add incognito_home here

Done.

https://codereview.chromium.org/2809923003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:198:
if (mTabModelSelector.isIncognitoSelected() && navItemId == R.id.action_home) {
On 2017/04/13 20:34:03, mdjones wrote:
> On 2017/04/12 23:12:11, Theresa wrote:
> > On 2017/04/12 22:28:00, dgn (in PST timezone) wrote:
> > > On 2017/04/12 21:32:03, Theresa wrote:
> > > > On 2017/04/12 16:51:35, mdjones wrote:
> > > > > Why is this special cased? Is the assumption that, since there's
little
> > > > content,
> > > > > the user won't be returning?
> > > > 
> > > > The map storing all of the other BottomSheetContent objects is keyed off
> > from
> > > > navigation item id. R.id.action_home is overloaded (it can be
> > > > IncognitobottomSheetContent or SuggestionsBottomSheetContent). Instead
of
> > > > introducing special code here, I could manipulate the navItemId used to
> get
> > > the
> > > > BottomSheetContent out of the map, but that seemed more complex.
> > > 
> > > why is introducing a special code a bad thing? According to
> > > https://developer.android.com/reference/android/view/View.html#setId(int)
> Ids
> > > are positive ints, so you could use -1 without risking conflicts? 
> > 
> > Changing the id isn't a bad thing. The IncognitoBottomSheetContent has to be
> > handled separately regardless of whether it's in the map or stored as a
> separate
> > variable. Storing it as a separate variable introduces a branch in the code
> > here. Storing it in the map introduces an if statement to change the
navItemId
> > here and another check in BottomSheetObserver#onSheetClosed() to avoid
> > destroying the IncognitoBottomSheetContent. I think both approaches are
valid,
> > so I'm happy to put it in the map if you and Matt feel strongly about the
> > approach.
> 
> I think we only need to special-case the suggestions content; the incognito
view
> is quite a bit smaller and can be created much more quickly. If you agree on
> that point, I'd rather have the incognito view live in the map.

Done.

When the bottom sheet is closed, this class resets the bottom sheet content to
R.id.action_home. In addition to showing the right content the next time the
panel is opened, that logic is needed to ensure the regular toolbar starts
showing again when the sheet is closed. I reworked the logic in onSheetClosed()
to support destroying the IncognitoBottomSheetContent in the map while still
ensuring that the regular toolbar is shown.

[commit-bot: I haz the power, 2017-04-13 21:31:01]

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

[commit-bot: I haz the power, 2017-04-13 21:31:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mdjones, 2017-04-13 21:57:25]

lgtm % suggestion

https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:137:
if (!newModel.isIncognito()) mBottomSheetContents.remove(INCOGNITO_HOME_ID);
As discussed offline, this is a bit scary since bottom sheet still has a
reference until it is opened again. Long term, we should support setting the
bottom sheet content to null in BottomSheet. For now, I think you could just set
the content to TYPE_SUGGESTIONS.

[Theresa, 2017-04-13 22:03:47]

https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:137:
if (!newModel.isIncognito()) mBottomSheetContents.remove(INCOGNITO_HOME_ID);
On 2017/04/13 21:57:24, mdjones wrote:
> As discussed offline, this is a bit scary since bottom sheet still has a
> reference until it is opened again. Long term, we should support setting the
> bottom sheet content to null in BottomSheet. For now, I think you could just
set
> the content to TYPE_SUGGESTIONS.

The content is set to TYPE_SUGGESTIONS two lines above, so when the tab model is
switched BottomSheet actually doesn't have a reference to
IncognitoBottomSheetContent. I should be calling destroy() before removing the
IncognitoBottomSheetContent from the map, though.

BottomSheet does still have a reference in this scenario:

1) Open incognito NTP
2) Close panel - at this point we call IncognitoBottomSheetContent#destroy() and
remove it from the map but BottomSheet still has a handle to it.

I think that we have two choices:
1) Skip destroying the IncognitoBottomSheetContent when the sheet is closed and
only destroy when the tab model is switched. This adds a little bit of memory
overhead but the IncognitoBottomSheetContent is really small.
2) destroy when the sheet is closed and switch to TYPE_SUGGESTIONS, with new
logic in onSheetOpened() to reset the content to IncognitoBottomSheetContent.

[mdjones, 2017-04-13 22:31:06]

https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:137:
if (!newModel.isIncognito()) mBottomSheetContents.remove(INCOGNITO_HOME_ID);
On 2017/04/13 22:03:46, Theresa wrote:
> On 2017/04/13 21:57:24, mdjones wrote:
> > As discussed offline, this is a bit scary since bottom sheet still has a
> > reference until it is opened again. Long term, we should support setting the
> > bottom sheet content to null in BottomSheet. For now, I think you could just
> set
> > the content to TYPE_SUGGESTIONS.
> 
> The content is set to TYPE_SUGGESTIONS two lines above, so when the tab model
is
> switched BottomSheet actually doesn't have a reference to
> IncognitoBottomSheetContent. I should be calling destroy() before removing the
> IncognitoBottomSheetContent from the map, though.
> 
> BottomSheet does still have a reference in this scenario:
> 
> 1) Open incognito NTP
> 2) Close panel - at this point we call IncognitoBottomSheetContent#destroy()
and
> remove it from the map but BottomSheet still has a handle to it.
> 
> I think that we have two choices:
> 1) Skip destroying the IncognitoBottomSheetContent when the sheet is closed
and
> only destroy when the tab model is switched. This adds a little bit of memory
> overhead but the IncognitoBottomSheetContent is really small.
> 2) destroy when the sheet is closed and switch to TYPE_SUGGESTIONS, with new
> logic in onSheetOpened() to reset the content to IncognitoBottomSheetContent.

I think 2 is going to be closer to what we need when we eventually work out the
eviction logic.

[Theresa, 2017-04-13 22:50:32]

On 2017/04/13 22:31:06, mdjones wrote:
>
https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
> (right):
> 
>
https://codereview.chromium.org/2809923003/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:137:
> if (!newModel.isIncognito()) mBottomSheetContents.remove(INCOGNITO_HOME_ID);
> On 2017/04/13 22:03:46, Theresa wrote:
> > On 2017/04/13 21:57:24, mdjones wrote:
> > > As discussed offline, this is a bit scary since bottom sheet still has a
> > > reference until it is opened again. Long term, we should support setting
the
> > > bottom sheet content to null in BottomSheet. For now, I think you could
just
> > set
> > > the content to TYPE_SUGGESTIONS.
> > 
> > The content is set to TYPE_SUGGESTIONS two lines above, so when the tab
model
> is
> > switched BottomSheet actually doesn't have a reference to
> > IncognitoBottomSheetContent. I should be calling destroy() before removing
the
> > IncognitoBottomSheetContent from the map, though.
> > 
> > BottomSheet does still have a reference in this scenario:
> > 
> > 1) Open incognito NTP
> > 2) Close panel - at this point we call IncognitoBottomSheetContent#destroy()
> and
> > remove it from the map but BottomSheet still has a handle to it.
> > 
> > I think that we have two choices:
> > 1) Skip destroying the IncognitoBottomSheetContent when the sheet is closed
> and
> > only destroy when the tab model is switched. This adds a little bit of
memory
> > overhead but the IncognitoBottomSheetContent is really small.
> > 2) destroy when the sheet is closed and switch to TYPE_SUGGESTIONS, with new
> > logic in onSheetOpened() to reset the content to
IncognitoBottomSheetContent.
> 
> I think 2 is going to be closer to what we need when we eventually work out
the
> eviction logic.

I started implementing #2 and it's going to be far more complex, requiring extra
logic in getSheetContentForId() to determine whether content is being retrieved
while the sheet is opened (get incognito) or closed (get TYPE_SUGGESTIONS). I
think that destroying the SuggestionsBottomSheetContent when we enter incognito
mode is more likely long-term than setting the bottom sheet content to
TYPE_SUGGESTIONS when the sheet is closed is incognito mode.

I'm going to go with #1 unless there is strong dissent. We can revisit when we
develop the destruction policy for TYPE_SUGGESTIONS and/or when we're heap
profiling Chrome Home.

[Theresa, 2017-04-13 22:54:51]

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

[commit-bot: I haz the power, 2017-04-13 22:55:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-04-13 23:51:15]

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:139:
if (!newModel.isIncognito()) mBottomSheetContents.remove(INCOGNITO_HOME_ID);
I still need to call mBottomSheetContengs.get(INCOGNITO_HOME_ID).destroy() here

[commit-bot: I haz the power, 2017-04-14 01:23:11]

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

[commit-bot: I haz the power, 2017-04-14 01:23:13]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-04-15 12:37:05]

lgtm is mdjones@ is happy

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_incognito.xml (right):

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_incognito.xml:69:
android:textColor="#03a9f4" />
nit: you're setting the background in the style, why not the text color too?
It's not the same as the one selected for Home, but you changed the incognito
primary color anyway...

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:64:
private static final int INCOGNITO_HOME_ID = -1;
nit: maybe document how the -1 value was selected?

[mdjones, 2017-04-17 15:37:57]

still lgtm

[Theresa, 2017-04-17 15:38:09]

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_incognito.xml (right):

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_incognito.xml:69:
android:textColor="#03a9f4" />
On 2017/04/15 12:37:04, dgn (in PST timezone) wrote:
> nit: you're setting the background in the style, why not the text color too?
> It's not the same as the one selected for Home, but you changed the incognito
> primary color anyway...

R.color.incognito_primary_color is used for the incognito toolbar color, as well
as the background color for the IncognitoBottomSheetContent. It is not, however,
used as the background color for the non-Chrome Home NTP. The difference in text
color is due to this difference in background color. In the non-Chrome Home
incognito NTP the text and link are displayed on #222
(R.color.ntp_bg_incognito). In the Chrome Home IncognitoBottomSheetContent the
text and link are displayed on #424242 (R.color.incognito_primary_color).

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java
(right):

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:64:
private static final int INCOGNITO_HOME_ID = -1;
On 2017/04/15 12:37:04, dgn (in PST timezone) wrote:
> nit: maybe document how the -1 value was selected?

Done.

https://codereview.chromium.org/2809923003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetContentController.java:139:
if (!newModel.isIncognito()) mBottomSheetContents.remove(INCOGNITO_HOME_ID);
On 2017/04/13 23:51:15, Theresa wrote:
> I still need to call mBottomSheetContengs.get(INCOGNITO_HOME_ID).destroy()
here

Done.

[Theresa, 2017-04-17 15:38:35]

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

[commit-bot: I haz the power, 2017-04-17 15:38:47]

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-04-17 16:57:51]

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

[commit-bot: I haz the power, 2017-04-17 16:57:52]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-04-17 21:44:50]

lgtm

[Theresa, 2017-04-17 21:50:07]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-04-17 21:50:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2809923003/#ps100001
(title: "[Home] Add a Chrome Home specific incognito NTP")

[commit-bot: I haz the power, 2017-04-17 21:50: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, 2017-04-17 21:57:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492465807869300,
"parent_rev": "a8d3f52b4314919506265b3f6c6d93fe79b1d902", "commit_rev":
"c72acf085d8c0977f28f3e54f4e99cc2cbd55554"}

[commit-bot: I haz the power, 2017-04-17 21:58:14]

Description was changed from

==========
[Home] Add a Chrome Home specific incognito NTP

Adds a ChromeHomeIncognitoNewTabPage displayed behind the BottomSheet
when an incognito NTP is selected and an IncognitoBottomSheetContent
that is displayed inside the "Home" tab of the BottomSheet in incognito
mode. Also updates ChromeHomeNewTabPageTest.

BUG=695973
==========

to

==========
[Home] Add a Chrome Home specific incognito NTP

Adds a ChromeHomeIncognitoNewTabPage displayed behind the BottomSheet
when an incognito NTP is selected and an IncognitoBottomSheetContent
that is displayed inside the "Home" tab of the BottomSheet in incognito
mode. Also updates ChromeHomeNewTabPageTest.

BUG=695973

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

[commit-bot: I haz the power, 2017-04-17 21:58:18]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c72acf085d8c0977f28f3e54f4e9...