🏡 Show correct BottomSheetContent toolbar when contents swapped

[Home] Show correct BottomSheetContent toolbar when contents swapped

 * Detaches the BottomSheetContent toolbars and attaches them to a new
   toolbar holder FrameLayout when the user switches between panes in
   Chrome Home.
 * Hides/shows the normal toolbar when the default BottomSheetContent
   is hidden/visible.
 * Attaches the full download/history/bookmark UI rather than detaching
   and reattaching the content views. Attaching the full UI to the view
   hiearchy is necessary for numerous features to function.
 * Initializes SuggestionsBottomSheetContent when the sheet is opened
   if it hasn't yet been initialized.

BUG=699598
Review-Url: https://codereview.chromium.org/2751583002
Cr-Commit-Position: refs/heads/master@{#457591}
Committed: https://chromium.googlesource.com/chromium/src/+/23587869aa25a885015ceabeeb63f44274f6ae26

[Theresa, 2017-03-13 23:06:00]

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

[commit-bot: I haz the power, 2017-03-13 23:06:44]

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

[Theresa, 2017-03-13 23:09:12]

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

[Theresa, 2017-03-13 23:09:13]

Not quite ready for review, but here for reference.

[commit-bot: I haz the power, 2017-03-14 00:44:35]

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

[commit-bot: I haz the power, 2017-03-14 00:44:35]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-03-14 16:25:02]

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

[Theresa, 2017-03-14 16:25:42]

Description was changed from

==========
[Home] Show correct BottomSheetContent toolbar when contents swapped

 * Detaches the BottomSheetContent toolbars and attaches them to a new
   toolbar holder FrameLayout when the user switches between panes in
   Chrome Home.
 * Hides/shows the normal toolbar when the default BottomSheetContent
   is hidden/visible.
 * Attaches the full download/history/bookmark UI rather than detaching
   and reattaching the content views. Attaching the full UI to the view
   hiearchy is necessary for numerous features to function.
 * Initializes SuggestionsBottomSheetContent when the sheet is opened
   if it hasn't yet been initialized.

BUG=699598
==========

to

==========
[Home] Show correct BottomSheetContent toolbar when contents swapped

 * Detaches the BottomSheetContent toolbars and attaches them to a new
   toolbar holder FrameLayout when the user switches between panes in
   Chrome Home.
 * Hides/shows the normal toolbar when the default BottomSheetContent
   is hidden/visible.
 * Attaches the full download/history/bookmark UI rather than detaching
   and reattaching the content views. Attaching the full UI to the view
   hiearchy is necessary for numerous features to function.
 * Initializes SuggestionsBottomSheetContent when the sheet is opened
   if it hasn't yet been initialized.

BUG=699598
==========

[commit-bot: I haz the power, 2017-03-14 16:25:50]

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

[Theresa, 2017-03-14 16:29:43]

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

[Theresa, 2017-03-14 16:29:44]

ptal

I'm likely going to wait to land this until after the pull handle lands. The
changes needed to make this patchset work well with that in-flight CL are pretty
minimal (noted in comments).

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:35:
android:layout_height="@dimen/toolbar_height_no_shadow"
This will change to bottom_control_container_height after the pull handle CL
lands.

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:43:
toolbar.getResources().getDimensionPixelOffset(R.dimen.toolbar_height_no_shadow);
This will also change to bottom_control_container_height after the pull handle
CL lands.

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:47:
toolbar.getResources().getDimensionPixelOffset(R.dimen.toolbar_height_no_shadow));
The toolbar will get bottom_toolbar_top_margin as extra top padding to push it
below the pull handle. The minimum height will also be adjusted to
bottom_control_container_height - bottom_toolbar_top_margin.

[Theresa, 2017-03-14 17:12:33]

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

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetContentController.java:96:
public void onSheetOpened() {
A note for myself -- mTabModelSelector.getCurrentTab() can return null if the
sheet is opened before the first tab is created -- if that's the case we should
return early rather than trying to initialize the default content to avoid
crashing.

[commit-bot: I haz the power, 2017-03-14 17:36:38]

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

[commit-bot: I haz the power, 2017-03-14 17:36:39]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-03-14 20:08:37]

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:206:
if (FeatureUtilities.isChromeHomeEnabled()) {
Can you make sure this works in custom tabs?

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java
(right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/BottomToolbarPhone.java:43:
toolbar.getResources().getDimensionPixelOffset(R.dimen.toolbar_height_no_shadow);
On 2017/03/14 16:29:43, Theresa wrote:
> This will also change to bottom_control_container_height after the pull handle
> CL lands.

I'm going to try to land a patch that adds getHeight to toolbar. If I get that
in, you shouldn't need to specify this.

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:151:
* The last non-default toolbar view that was attached to mToolbarHolder.
nit: comment can be made single-line

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:498:
if (mDefaultToolbarView == null) {
It looks like it should be possible to swap the initialization code blocks in
ChromeActivity; I think the current order is arbitrary since they didn't depend
on each other.

[Theresa, 2017-03-14 20:39:56]

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

[Theresa, 2017-03-14 20:40:02]

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:206:
if (FeatureUtilities.isChromeHomeEnabled()) {
On 2017/03/14 20:08:36, mdjones wrote:
> Can you make sure this works in custom tabs?

None of these menu items are shown in custom tabs -- that menu is controlled by
CustomTabAppMenuPropertiesDelegate:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

I did move this inside of the if (isPageMenu..) block since that is the only
group these are valid for. And I tested in custom tabs as a sanity check.

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java
(right):

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:151:
* The last non-default toolbar view that was attached to mToolbarHolder.
On 2017/03/14 20:08:36, mdjones wrote:
> nit: comment can be made single-line

Done.

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheet.java:498:
if (mDefaultToolbarView == null) {
On 2017/03/14 20:08:36, mdjones wrote:
> It looks like it should be possible to swap the initialization code blocks in
> ChromeActivity; I think the current order is arbitrary since they didn't
depend
> on each other.

Done.

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

https://codereview.chromium.org/2751583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/BottomSheetContentController.java:96:
public void onSheetOpened() {
On 2017/03/14 17:12:33, Theresa wrote:
> A note for myself -- mTabModelSelector.getCurrentTab() can return null if the
> sheet is opened before the first tab is created -- if that's the case we
should
> return early rather than trying to initialize the default content to avoid
> crashing.

Done.

[commit-bot: I haz the power, 2017-03-14 20:40:34]

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

[mdjones, 2017-03-14 21:22:13]

lgtm

[commit-bot: I haz the power, 2017-03-14 22:14:31]

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

[commit-bot: I haz the power, 2017-03-14 22:14:32]

Dry run: This issue passed the CQ dry run.

[gone, 2017-03-15 17:21:16]

lgtm

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:36:
android:layout_marginTop="@dimen/toolbar_shadow_height" >
newlines before and after ViewStub

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:677: 
no newline

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:190:
if (FeatureUtilities.isChromeHomeEnabled()) {
All the cool kids are writing lengthy comments explaining why they hide things. 
Do you want to be cool?  It's cool if you don't because the conditional is
explanatory enough.

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
(right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:81:
* @param isFullscreenActivity Whether the history UI will be shown in a
fullscreen activity.
Ambiguous... Chrome Home is still inside a full screen activity

[Theresa, 2017-03-16 18:15:16]

please take another look, specifically at BottomToolbarPhone.java and the
*SheetContent.java classes. I made the static method to set toolbar style a
class method so that it can use getHeight() and getExtraTopMargin().

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/bottom_control_container.xml (right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/bottom_control_container.xml:36:
android:layout_marginTop="@dimen/toolbar_shadow_height" >
On 2017/03/15 17:21:15, dfalcantara (load balance plz) wrote:
> newlines before and after ViewStub

Done.

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/values-v17/styles.xml (right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/res...
chrome/android/java/res/values-v17/styles.xml:677: 
On 2017/03/15 17:21:15, dfalcantara (load balance plz) wrote:
> no newline

I added a "Miscellaneous" comment above this style rule. All of the above styles
are part of the <!-- Download Home --> block.

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:190:
if (FeatureUtilities.isChromeHomeEnabled()) {
On 2017/03/15 17:21:15, dfalcantara (load balance plz) wrote:
> All the cool kids are writing lengthy comments explaining why they hide
things. 
> Do you want to be cool?  It's cool if you don't because the conditional is
> explanatory enough.

I want to be cool.

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
(right):

https://codereview.chromium.org/2751583002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:81:
* @param isFullscreenActivity Whether the history UI will be shown in a
fullscreen activity.
On 2017/03/15 17:21:15, dfalcantara (load balance plz) wrote:
> Ambiguous... Chrome Home is still inside a full screen activity

Done. Changed this to "separate activity".

https://codereview.chromium.org/2751583002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2751583002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:191:
// History, downloads, and and bookmarks are shown in the Chrome Home bottom
sheet.
nit: remove the second "and"

[Theresa, 2017-03-16 18:15:19]

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

[commit-bot: I haz the power, 2017-03-16 18:16:18]

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-03-16 18:57:35]

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

[commit-bot: I haz the power, 2017-03-16 18:57:36]

Dry run: This issue passed the CQ dry run.

[gone, 2017-03-16 20:35:18]

still lgtm

[mdjones, 2017-03-16 21:51:05]

still lgtm as well

[Theresa, 2017-03-16 21:55:44]

https://codereview.chromium.org/2751583002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2751583002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:191:
// History, downloads, and and bookmarks are shown in the Chrome Home bottom
sheet.
On 2017/03/16 18:15:16, Theresa wrote:
> nit: remove the second "and"

Done.

[Theresa, 2017-03-16 21:55:49]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-03-16 21:55:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2751583002/#ps80001
(title: "Remove duplicate "and"")

[commit-bot: I haz the power, 2017-03-16 21:56:14]

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-03-16 22:51:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489701349659420,
"parent_rev": "85ec3b10817d2158c5c04db9b243408cc8c44e74", "commit_rev":
"23587869aa25a885015ceabeeb63f44274f6ae26"}

[commit-bot: I haz the power, 2017-03-16 22:52:29]

Description was changed from

==========
[Home] Show correct BottomSheetContent toolbar when contents swapped

 * Detaches the BottomSheetContent toolbars and attaches them to a new
   toolbar holder FrameLayout when the user switches between panes in
   Chrome Home.
 * Hides/shows the normal toolbar when the default BottomSheetContent
   is hidden/visible.
 * Attaches the full download/history/bookmark UI rather than detaching
   and reattaching the content views. Attaching the full UI to the view
   hiearchy is necessary for numerous features to function.
 * Initializes SuggestionsBottomSheetContent when the sheet is opened
   if it hasn't yet been initialized.

BUG=699598
==========

to

==========
[Home] Show correct BottomSheetContent toolbar when contents swapped

 * Detaches the BottomSheetContent toolbars and attaches them to a new
   toolbar holder FrameLayout when the user switches between panes in
   Chrome Home.
 * Hides/shows the normal toolbar when the default BottomSheetContent
   is hidden/visible.
 * Attaches the full download/history/bookmark UI rather than detaching
   and reattaching the content views. Attaching the full UI to the view
   hiearchy is necessary for numerous features to function.
 * Initializes SuggestionsBottomSheetContent when the sheet is opened
   if it hasn't yet been initialized.

BUG=699598

Review-Url: https://codereview.chromium.org/2751583002
Cr-Commit-Position: refs/heads/master@{#457591}
Committed:
https://chromium.googlesource.com/chromium/src/+/23587869aa25a885015ceabeeb63...
==========

[commit-bot: I haz the power, 2017-03-16 22:52:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/23587869aa25a885015ceabeeb63...