Add a footer at the bottom of the menu for custom tabs

Add a footer at the bottom of the menu for custom tabs

This adds a footer with Chrome branding at the bottom of the menu
for custom tabs to prompt the user the content they are looking at
belongs to a Chrome Tab.

BUG=507522
Committed: https://crrev.com/047ad603c025597829bb6e0482752ad95b2e431c
Cr-Commit-Position: refs/heads/master@{#338366}

[Yusuf, 2015-07-10 01:03:54]

yusufo@chromium.org changed reviewers:
+ newt@chromium.org

[Yusuf, 2015-07-10 01:03:55]

I dont know why but rietveld thinks I have updated some old xmls rather than
adding new ones. I promise I didn't! The xml files are all brand new!

[newt (away), 2015-07-10 16:08:38]

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/bg_menu_chrome_branding.xml (right):

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/bg_menu_chrome_branding.xml:9:
android:width="1px"
1px will be almost invisible on fancy new xxhdpi and xxxhdpi phones. I'd
probably just use 1dp. (Alternatively, you could look at how wide list dividers
in various screen resolution and define a new dimen called divider_width that's,
say, 1px on devices up to xhdpi, and 2px on larger devices. I think list
dividers might just be 1dp, though.)

Also, this will produce a border on all four sides, which is not what we want.
Could you upload a screenshot on the bug? To fix that, you could a) use <inset>
element with insetRight, insetLeft, and insetBottom set to "-1dp", or b) instead
of using a fancy background, change chrome_branding_footer.xml to be a
FrameLayout that contains a View with height 1dp, and a TextView below that.

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/chrome_branding_footer.xml (right):

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/chrome_branding_footer.xml:1: <?xml version="1.0"
encoding="utf-8"?>
I'd suggest calling this powered_by_chrome.xml or powered_by_chrome_footer.xml.
"chrome_branding" is a big vaguer and could mean a lot of things. (If you change
the name, please update the other resource names too)

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/chrome_branding_footer.xml:8:
android:layout_height="@dimen/chrome_branding_footer_height"
Does this look OK when the font size is set to Huge? If not, you might want to
use wrap_content.

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/chrome_branding_footer.xml:14:
android:textColor="#616161"
Another shade of gray! (Not your fault. But please make the designer feel guilty
for this.)

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:140:
Rect visibleDisplayFrame, int screenHeight, int promptViewResourceId) {
The name "promptView" doesn't make any sense to me. How about
"footerResourceId"?

(Okay, after reading on, I understand where "promptView" came from. I still
think it should be changed to "footerResourceId" though :)

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:149:
mPopup.setPromptView(
What happens if the list scrolls? Does the footer view stay in a fixed position
at the bottom, or does it scroll? What behavior do we want?

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

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:22:
boolean shouldContainBrandingFooter();
How about changing this to getFooterResourceId(), and allowing the return value
to be 0 or a valid resource? That would keep all knowledge of the "powered by
chrome" footer inside of CustomTabAppMenuPropertiesDelegate.java.

[Yusuf, 2015-07-10 18:43:47]

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/dra...
File chrome/android/java/res/drawable/bg_menu_chrome_branding.xml (right):

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/dra...
chrome/android/java/res/drawable/bg_menu_chrome_branding.xml:9:
android:width="1px"
On 2015/07/10 16:08:38, newt wrote:
> 1px will be almost invisible on fancy new xxhdpi and xxxhdpi phones. I'd
> probably just use 1dp. (Alternatively, you could look at how wide list
dividers
> in various screen resolution and define a new dimen called divider_width
that's,
> say, 1px on devices up to xhdpi, and 2px on larger devices. I think list
> dividers might just be 1dp, though.)
> 
> Also, this will produce a border on all four sides, which is not what we want.
> Could you upload a screenshot on the bug? To fix that, you could a) use
<inset>
> element with insetRight, insetLeft, and insetBottom set to "-1dp", or b)
instead
> of using a fancy background, change chrome_branding_footer.xml to be a
> FrameLayout that contains a View with height 1dp, and a TextView below that.

Done.Opted for the less cool FrameLayout option with 1 dp.

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/chrome_branding_footer.xml (right):

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/chrome_branding_footer.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2015/07/10 16:08:38, newt wrote:
> I'd suggest calling this powered_by_chrome.xml or
powered_by_chrome_footer.xml.
> "chrome_branding" is a big vaguer and could mean a lot of things. (If you
change
> the name, please update the other resource names too)

Done.

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/chrome_branding_footer.xml:8:
android:layout_height="@dimen/chrome_branding_footer_height"
On 2015/07/10 16:08:38, newt wrote:
> Does this look OK when the font size is set to Huge? If not, you might want to
> use wrap_content.

I think it does. Screenshot uploaded to bug.

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/chrome_branding_footer.xml:14:
android:textColor="#616161"
On 2015/07/10 16:08:38, newt wrote:
> Another shade of gray! (Not your fault. But please make the designer feel
guilty
> for this.)

Done. Well not done, but put it on my list. :)

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:140:
Rect visibleDisplayFrame, int screenHeight, int promptViewResourceId) {
On 2015/07/10 16:08:38, newt wrote:
> The name "promptView" doesn't make any sense to me. How about
> "footerResourceId"?
> 
> (Okay, after reading on, I understand where "promptView" came from. I still
> think it should be changed to "footerResourceId" though :)

Done.

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:149:
mPopup.setPromptView(
On 2015/07/10 16:08:38, newt wrote:
> What happens if the list scrolls? Does the footer view stay in a fixed
position
> at the bottom, or does it scroll? What behavior do we want?

It stays in a fixed position at the bottom and that is the behavior we want.

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

https://codereview.chromium.org/1229223002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:22:
boolean shouldContainBrandingFooter();
On 2015/07/10 16:08:38, newt wrote:
> How about changing this to getFooterResourceId(), and allowing the return
value
> to be 0 or a valid resource? That would keep all knowledge of the "powered by
> chrome" footer inside of CustomTabAppMenuPropertiesDelegate.java.

Done.

[newt (away), 2015-07-10 18:55:46]

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

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:8:
android:layout_height="wrap_content"
Technically, this height should be powered_by_chrome_footer_height, otherwise,
the layout calculations in AppMenu will be off by 1dp, right?

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:16:
android:layout_height="@dimen/powered_by_chrome_footer_height"
Then, this can simply be match_parent. You could also add
layout_marginTop="1dp".

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:23:
android:singleLine="true" />
maxLines="1" is recommended over singleLine="true" for its simpler and more
performant behavior

http://stackoverflow.com/questions/17124985/why-does-androidsingleline-true-m...

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

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:137: * 
                           Can be 0 if no such view is required.
nit: indentation

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:149:
mPopup.setPromptView(
fits on one line?

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:152:
R.dimen.powered_by_chrome_footer_height);
maybe should call this menu_footer_height, since AppMenu doesn't "know" about
the "powered by chrome" footer anymore; just about the general concept of a menu
footer.

[Yusuf, 2015-07-10 19:12:19]

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

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:8:
android:layout_height="wrap_content"
On 2015/07/10 18:55:46, newt wrote:
> Technically, this height should be powered_by_chrome_footer_height, otherwise,
> the layout calculations in AppMenu will be off by 1dp, right?

Yes, to the technicality. No to the calculations. Since this is a framelayout
the total height ends up being 28dp anyway, right?

For some reason if I change the height here the parent layout ends up overriding
that and squishing things to wrap_content on the textview only. Any suggestions?

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:23:
android:singleLine="true" />
On 2015/07/10 18:55:46, newt wrote:
> maxLines="1" is recommended over singleLine="true" for its simpler and more
> performant behavior
> 
>
http://stackoverflow.com/questions/17124985/why-does-androidsingleline-true-m...

Done.

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

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:137: * 
                           Can be 0 if no such view is required.
On 2015/07/10 18:55:46, newt wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:149:
mPopup.setPromptView(
On 2015/07/10 18:55:46, newt wrote:
> fits on one line?

Done.

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:152:
R.dimen.powered_by_chrome_footer_height);
On 2015/07/10 18:55:46, newt wrote:
> maybe should call this menu_footer_height, since AppMenu doesn't "know" about
> the "powered by chrome" footer anymore; just about the general concept of a
menu
> footer.

Done.

[newt (away), 2015-07-10 19:18:39]

lgtm after comment

Also, can we start adding "Powered by Chrome" to our email signatures? :)

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

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:8:
android:layout_height="wrap_content"
On 2015/07/10 19:12:19, Yusuf wrote:
> On 2015/07/10 18:55:46, newt wrote:
> > Technically, this height should be powered_by_chrome_footer_height,
otherwise,
> > the layout calculations in AppMenu will be off by 1dp, right?
> 
> Yes, to the technicality. No to the calculations. Since this is a framelayout
> the total height ends up being 28dp anyway, right?
> 
> For some reason if I change the height here the parent layout ends up
overriding
> that and squishing things to wrap_content on the textview only. Any
suggestions?

That probably happens because you're not passing a parent when you inflate
powered_by_chrome_footer.xml. If you don't pass a parent when inflating a view,
then the view will end with up default layout params instead of the very
carefully chosen layout params that you specified in the layout file.

Regardless, I agree that since this is a FrameLayout, it's fine to make its
height wrap_content. You should probably also add paddingTop="1dp" to the
TextView.

[Yusuf, 2015-07-10 19:23:56]

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

https://codereview.chromium.org/1229223002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/powered_by_chrome_footer.xml:8:
android:layout_height="wrap_content"
On 2015/07/10 19:18:38, newt wrote:
> On 2015/07/10 19:12:19, Yusuf wrote:
> > On 2015/07/10 18:55:46, newt wrote:
> > > Technically, this height should be powered_by_chrome_footer_height,
> otherwise,
> > > the layout calculations in AppMenu will be off by 1dp, right?
> > 
> > Yes, to the technicality. No to the calculations. Since this is a
framelayout
> > the total height ends up being 28dp anyway, right?
> > 
> > For some reason if I change the height here the parent layout ends up
> overriding
> > that and squishing things to wrap_content on the textview only. Any
> suggestions?
> 
> That probably happens because you're not passing a parent when you inflate
> powered_by_chrome_footer.xml. If you don't pass a parent when inflating a
view,
> then the view will end with up default layout params instead of the very
> carefully chosen layout params that you specified in the layout file.
> 
> Regardless, I agree that since this is a FrameLayout, it's fine to make its
> height wrap_content. You should probably also add paddingTop="1dp" to the
> TextView.

Ah. I see. OK added the padding to the top.

[Yusuf, 2015-07-10 19:24:04]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-07-10 19:24:04]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1229223002/#ps60001
(title: "Added padding to the top")

[commit-bot: I haz the power, 2015-07-10 19:25:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229223002/60001

[commit-bot: I haz the power, 2015-07-10 19:40:34]

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

[commit-bot: I haz the power, 2015-07-10 19:40:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

(exceeded global retry quota)

[Yusuf, 2015-07-10 20:48:54]

The CQ bit was checked by yusufo@chromium.org

[Yusuf, 2015-07-10 20:48:54]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1229223002/#ps80001
(title: "Added new interface call to chrome shell")

[commit-bot: I haz the power, 2015-07-10 20:49:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1229223002/80001

[commit-bot: I haz the power, 2015-07-10 21:32:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-10 21:33:05]

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